| Message ID | 452b42cb-56a5-3f28-989f-c02e53334447@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMU: assorted follow-on to XSA-400 | expand |
On Mon, Apr 11, 2022 at 11:40:24AM +0200, Jan Beulich wrote: > There's no good reason to use these when we already have a pci_sbdf_t > type object available. This extends to the use of PCI_BUS() in > pci_ecam_map_bus() as well. > > No change to generated code (with gcc11 at least, and I have to admit > that I didn't expect compilers to necessarily be able to spot the > optimization potential on the original code). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
Hi Jan, > On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote: > > There's no good reason to use these when we already have a pci_sbdf_t > type object available. This extends to the use of PCI_BUS() in > pci_ecam_map_bus() as well. > > No change to generated code (with gcc11 at least, and I have to admit > that I didn't expect compilers to necessarily be able to spot the > optimization potential on the original code). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Note that the Arm changes are "blind": I haven't been able to spot a way > to at least compile test the changes there; the code looks to be > entirely dead. > > --- a/xen/arch/arm/pci/ecam.c > +++ b/xen/arch/arm/pci/ecam.c > @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc > container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); > unsigned int devfn_shift = ops->bus_shift - 8; > void __iomem *base; > - > - unsigned int busn = PCI_BUS(sbdf.bdf); > + unsigned int busn = sbdf.bus; > > if ( busn < cfg->busn_start || busn > cfg->busn_end ) > return NULL; > @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc > busn -= cfg->busn_start; > base = cfg->win + (busn << ops->bus_shift); > > - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; > + return base + (sbdf.df << devfn_shift) + where; I think this should be sbdf.bdf instead (typo you removed the b). Cheers Bertrand
On 13.04.2022 15:48, Bertrand Marquis wrote: >> On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote: >> --- a/xen/arch/arm/pci/ecam.c >> +++ b/xen/arch/arm/pci/ecam.c >> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc >> container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); >> unsigned int devfn_shift = ops->bus_shift - 8; >> void __iomem *base; >> - >> - unsigned int busn = PCI_BUS(sbdf.bdf); >> + unsigned int busn = sbdf.bus; >> >> if ( busn < cfg->busn_start || busn > cfg->busn_end ) >> return NULL; >> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc >> busn -= cfg->busn_start; >> base = cfg->win + (busn << ops->bus_shift); >> >> - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; >> + return base + (sbdf.df << devfn_shift) + where; > > I think this should be sbdf.bdf instead (typo you removed the b). I don't think so, no - the transformation is to remove the PCI_DEVFN2(), which was another way to drop the bus part of the coordinates. Patch context also shows that the bus part if taken care of by other means. Jan
On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote: > Hi Jan, > > > On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote: > > > > There's no good reason to use these when we already have a pci_sbdf_t > > type object available. This extends to the use of PCI_BUS() in > > pci_ecam_map_bus() as well. > > > > No change to generated code (with gcc11 at least, and I have to admit > > that I didn't expect compilers to necessarily be able to spot the > > optimization potential on the original code). > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > > Note that the Arm changes are "blind": I haven't been able to spot a way > > to at least compile test the changes there; the code looks to be > > entirely dead. > > > > --- a/xen/arch/arm/pci/ecam.c > > +++ b/xen/arch/arm/pci/ecam.c > > @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc > > container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); > > unsigned int devfn_shift = ops->bus_shift - 8; > > void __iomem *base; > > - > > - unsigned int busn = PCI_BUS(sbdf.bdf); > > + unsigned int busn = sbdf.bus; > > > > if ( busn < cfg->busn_start || busn > cfg->busn_end ) > > return NULL; > > @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc > > busn -= cfg->busn_start; > > base = cfg->win + (busn << ops->bus_shift); > > > > - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; > > + return base + (sbdf.df << devfn_shift) + where; > > I think this should be sbdf.bdf instead (typo you removed the b). I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the devfn from sbdf.bdf. That's not needed, as you can just get the devfn directly from sbdf.df. Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be there. Thanks, Roger.
Hi, > On 13 Apr 2022, at 14:58, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> There's no good reason to use these when we already have a pci_sbdf_t >>> type object available. This extends to the use of PCI_BUS() in >>> pci_ecam_map_bus() as well. >>> >>> No change to generated code (with gcc11 at least, and I have to admit >>> that I didn't expect compilers to necessarily be able to spot the >>> optimization potential on the original code). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> Note that the Arm changes are "blind": I haven't been able to spot a way >>> to at least compile test the changes there; the code looks to be >>> entirely dead. >>> >>> --- a/xen/arch/arm/pci/ecam.c >>> +++ b/xen/arch/arm/pci/ecam.c >>> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc >>> container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); >>> unsigned int devfn_shift = ops->bus_shift - 8; >>> void __iomem *base; >>> - >>> - unsigned int busn = PCI_BUS(sbdf.bdf); >>> + unsigned int busn = sbdf.bus; >>> >>> if ( busn < cfg->busn_start || busn > cfg->busn_end ) >>> return NULL; >>> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc >>> busn -= cfg->busn_start; >>> base = cfg->win + (busn << ops->bus_shift); >>> >>> - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; >>> + return base + (sbdf.df << devfn_shift) + where; >> >> I think this should be sbdf.bdf instead (typo you removed the b). > > I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the > devfn from sbdf.bdf. That's not needed, as you can just get the devfn > directly from sbdf.df. > > Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be > there. There is not df field in the sbdf structure so it should be devfn instead. Cheers Bertrand > > Thanks, Roger.
On 13.04.2022 16:13, Bertrand Marquis wrote: >> On 13 Apr 2022, at 14:58, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote: >>>> On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> There's no good reason to use these when we already have a pci_sbdf_t >>>> type object available. This extends to the use of PCI_BUS() in >>>> pci_ecam_map_bus() as well. >>>> >>>> No change to generated code (with gcc11 at least, and I have to admit >>>> that I didn't expect compilers to necessarily be able to spot the >>>> optimization potential on the original code). >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> Note that the Arm changes are "blind": I haven't been able to spot a way >>>> to at least compile test the changes there; the code looks to be >>>> entirely dead. Note this remark. >>>> --- a/xen/arch/arm/pci/ecam.c >>>> +++ b/xen/arch/arm/pci/ecam.c >>>> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc >>>> container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); >>>> unsigned int devfn_shift = ops->bus_shift - 8; >>>> void __iomem *base; >>>> - >>>> - unsigned int busn = PCI_BUS(sbdf.bdf); >>>> + unsigned int busn = sbdf.bus; >>>> >>>> if ( busn < cfg->busn_start || busn > cfg->busn_end ) >>>> return NULL; >>>> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc >>>> busn -= cfg->busn_start; >>>> base = cfg->win + (busn << ops->bus_shift); >>>> >>>> - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; >>>> + return base + (sbdf.df << devfn_shift) + where; >>> >>> I think this should be sbdf.bdf instead (typo you removed the b). >> >> I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the >> devfn from sbdf.bdf. That's not needed, as you can just get the devfn >> directly from sbdf.df. >> >> Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be >> there. > > There is not df field in the sbdf structure so it should be devfn instead. Yes indeed, thanks for noticing. But really (see the remark further up) this is what happens if code in the tree can't even be built-tested. Jan
> From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, April 11, 2022 5:40 PM > > There's no good reason to use these when we already have a pci_sbdf_t > type object available. This extends to the use of PCI_BUS() in > pci_ecam_map_bus() as well. > > No change to generated code (with gcc11 at least, and I have to admit > that I didn't expect compilers to necessarily be able to spot the > optimization potential on the original code). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > Note that the Arm changes are "blind": I haven't been able to spot a way > to at least compile test the changes there; the code looks to be > entirely dead. > > --- a/xen/arch/arm/pci/ecam.c > +++ b/xen/arch/arm/pci/ecam.c > @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc > container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); > unsigned int devfn_shift = ops->bus_shift - 8; > void __iomem *base; > - > - unsigned int busn = PCI_BUS(sbdf.bdf); > + unsigned int busn = sbdf.bus; > > if ( busn < cfg->busn_start || busn > cfg->busn_end ) > return NULL; > @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc > busn -= cfg->busn_start; > base = cfg->win + (busn << ops->bus_shift); > > - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; > + return base + (sbdf.df << devfn_shift) + where; > } > > bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d, > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -839,7 +839,7 @@ static int msix_capability_init(struct p > pbus = dev->info.physfn.bus; > pslot = PCI_SLOT(dev->info.physfn.devfn); > pfunc = PCI_FUNC(dev->info.physfn.devfn); > - vf = PCI_BDF2(dev->bus, dev->devfn); > + vf = dev->sbdf.bdf; > } > > table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf); > --- a/xen/drivers/passthrough/vtd/qinval.c > +++ b/xen/drivers/passthrough/vtd/qinval.c > @@ -267,7 +267,7 @@ int qinval_device_iotlb_sync(struct vtd_ > qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0; > qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev- > >ats.queue_depth; > qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0; > - qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev- > >devfn); > + qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = pdev->sbdf.bdf; > qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0; > > qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
--- a/xen/arch/arm/pci/ecam.c +++ b/xen/arch/arm/pci/ecam.c @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc container_of(bridge->ops, const struct pci_ecam_ops, pci_ops); unsigned int devfn_shift = ops->bus_shift - 8; void __iomem *base; - - unsigned int busn = PCI_BUS(sbdf.bdf); + unsigned int busn = sbdf.bus; if ( busn < cfg->busn_start || busn > cfg->busn_end ) return NULL; @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc busn -= cfg->busn_start; base = cfg->win + (busn << ops->bus_shift); - return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where; + return base + (sbdf.df << devfn_shift) + where; } bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d, --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -839,7 +839,7 @@ static int msix_capability_init(struct p pbus = dev->info.physfn.bus; pslot = PCI_SLOT(dev->info.physfn.devfn); pfunc = PCI_FUNC(dev->info.physfn.devfn); - vf = PCI_BDF2(dev->bus, dev->devfn); + vf = dev->sbdf.bdf; } table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf); --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -267,7 +267,7 @@ int qinval_device_iotlb_sync(struct vtd_ qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0; qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth; qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0; - qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn); + qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = pdev->sbdf.bdf; qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0; qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
There's no good reason to use these when we already have a pci_sbdf_t type object available. This extends to the use of PCI_BUS() in pci_ecam_map_bus() as well. No change to generated code (with gcc11 at least, and I have to admit that I didn't expect compilers to necessarily be able to spot the optimization potential on the original code). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Note that the Arm changes are "blind": I haven't been able to spot a way to at least compile test the changes there; the code looks to be entirely dead.