| Message ID | 6dd6719f-a05d-3f90-95b5-0ce9b5b0a2da@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMU/PCI: assorted follow-on to XSA-400 | expand |
On 21/04/2022 15:26, 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> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Reviewed-by: Paul Durrant <paul@xen.org>
On 21.04.2022 16:26, 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> > 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. > --- > v2: Arm build fix (for those who actually have ways to build the Arm > code being changed here). May I please get an Arm side ack (or otherwise) here? Especially the 2nd, dependent patch better wouldn't remain pending for too long, or else there's a fair risk for it to go stale. Thanks, Jan > --- 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.devfn << 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; > >
Hi Jan, > On 21 Apr 2022, at 15:26, 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> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Sorry I missed it. Cheers Bertrand > --- > 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. > --- > v2: Arm build fix (for those who actually have ways to build the Arm > code being changed here). > > --- 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.devfn << 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.devfn << 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;