| Message ID | 33824f67-d50d-f7ac-f3a6-305eb8fe3bc1@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,01/10] AMD/IOMMU: miscellaneous DTE handling adjustments | expand |
On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote: > Rather than doing this every time we set up interrupts for a device > anew (and then in two distinct places) fill this invariant field > right after allocating struct arch_msix. > > While at it also obtain the MSI-X capability structure position just > once, in msix_capability_init(), rather than in each caller. > > Furthermore take the opportunity and eliminate the multi_msix_capable() > alias of msix_table_size(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v5: New. > > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -821,10 +821,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 > * requested MSI-X entries with allocated irqs or non-zero for otherwise. > **/ > static int msix_capability_init(struct pci_dev *dev, > - unsigned int pos, > struct msi_info *msi, > - struct msi_desc **desc, > - unsigned int nr_entries) > + struct msi_desc **desc) > { > struct arch_msix *msix = dev->msix; > struct msi_desc *entry = NULL; > @@ -838,6 +836,11 @@ static int msix_capability_init(struct p > u8 slot = PCI_SLOT(dev->devfn); > u8 func = PCI_FUNC(dev->devfn); > bool maskall = msix->host_maskall; > + unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, > + PCI_CAP_ID_MSIX); > + > + if ( !pos ) > + return -ENODEV; > ASSERT(pcidevs_locked()); > @@ -912,10 +915,9 @@ static int msix_capability_init(struct p > u64 pba_paddr; > u32 pba_offset; > - msix->nr_entries = nr_entries; > msix->table.first = PFN_DOWN(table_paddr); > msix->table.last = PFN_DOWN(table_paddr + > - nr_entries * PCI_MSIX_ENTRY_SIZE - 1); > + msix->nr_entries * PCI_MSIX_ENTRY_SIZE - 1); > WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->table.first, > msix->table.last)); > @@ -927,7 +929,7 @@ static int msix_capability_init(struct p > msix->pba.first = PFN_DOWN(pba_paddr); > msix->pba.last = PFN_DOWN(pba_paddr + > - BITS_TO_LONGS(nr_entries) - 1); > + BITS_TO_LONGS(msix->nr_entries) - 1); > WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first, > msix->pba.last)); > } > @@ -999,7 +1001,6 @@ static int msix_capability_init(struct p > /* XXX How to deal with existing mappings? */ > } > } > - WARN_ON(msix->nr_entries != nr_entries); > WARN_ON(msix->table.first != (table_paddr >> PAGE_SHIFT)); > ++msix->used_entries; > @@ -1093,22 +1094,17 @@ static void __pci_disable_msi(struct msi > **/ > static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc) > { > - int pos, nr_entries; > struct pci_dev *pdev; > - u16 control; > u8 slot = PCI_SLOT(msi->devfn); > u8 func = PCI_FUNC(msi->devfn); > struct msi_desc *old_desc; Missing newline. > ASSERT(pcidevs_locked()); > pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn); > - pos = pci_find_cap_offset(msi->seg, msi->bus, slot, func, PCI_CAP_ID_MSIX); > - if ( !pdev || !pos ) > + if ( !pdev || !pdev->msix ) > return -ENODEV; > - control = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); > - nr_entries = multi_msix_capable(control); > - if ( msi->entry_nr >= nr_entries ) > + if ( msi->entry_nr >= pdev->msix->nr_entries ) > return -EINVAL; > old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSIX); > @@ -1127,7 +1123,7 @@ static int __pci_enable_msix(struct msi_ > __pci_disable_msi(old_desc); > } > - return msix_capability_init(pdev, pos, msi, desc, nr_entries); > + return msix_capability_init(pdev, msi, desc); > } > static void _pci_cleanup_msix(struct arch_msix *msix) > @@ -1187,16 +1183,10 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 > { > int rc; > struct pci_dev *pdev; > - u8 slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); > - unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, > - PCI_CAP_ID_MSIX); You seem to be missing an empty newline here? > if ( !use_msi ) > return 0; Same here. > - if ( !pos ) > - return -ENODEV; > - > pcidevs_lock(); > pdev = pci_get_pdev(seg, bus, devfn); > if ( !pdev ) > @@ -1209,13 +1199,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 > rc = 0; > } > else > - { > - uint16_t control = pci_conf_read16(PCI_SBDF3(seg, bus, devfn), > - msix_control_reg(pos)); > - > - rc = msix_capability_init(pdev, pos, NULL, NULL, > - multi_msix_capable(control)); > - } > + rc = msix_capability_init(pdev, NULL, NULL); > pcidevs_unlock(); > return rc; > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -324,6 +324,7 @@ static void apply_quirks(struct pci_dev > static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) > { > struct pci_dev *pdev; > + unsigned int pos; This chunk doesn't seem to match my current code, as there's an empty newline between the declarations and list_for_each_entry. > list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) > if ( pdev->bus == bus && pdev->devfn == devfn ) > @@ -339,10 +340,12 @@ static struct pci_dev *alloc_pdev(struct > pdev->domain = NULL; > INIT_LIST_HEAD(&pdev->msi_list); > - if ( pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > - PCI_CAP_ID_MSIX) ) > + pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + PCI_CAP_ID_MSIX); > + if ( pos ) > { > struct arch_msix *msix = xzalloc(struct arch_msix); > + uint16_t ctrl; > if ( !msix ) > { > @@ -350,6 +353,10 @@ static struct pci_dev *alloc_pdev(struct > return NULL; > } > spin_lock_init(&msix->table_lock); > + > + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); > + msix->nr_entries = msix_table_size(ctrl); Since you store the number of entries here, why not also store the position of the MSI-X capability since it's also immutable? That would prevent having to fetch it again in msix_capability_init. Thanks, Roger.
On 06.08.2019 16:25, Roger Pau Monné wrote: > On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote: >> @@ -1093,22 +1094,17 @@ static void __pci_disable_msi(struct msi >> **/ >> static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc) >> { >> - int pos, nr_entries; >> struct pci_dev *pdev; >> - u16 control; >> u8 slot = PCI_SLOT(msi->devfn); >> u8 func = PCI_FUNC(msi->devfn); >> struct msi_desc *old_desc; > > Missing newline. For one this is patch context only anyway. And then I'm afraid this is an artifact of the ongoing email issue mentioned in the cover letter. In the list archives this also reflects itself by ... >> ASSERT(pcidevs_locked()); ... this line not having any indentation at all. Then again, looking at the copy I have received back from xen-devel, I can't see any issue there at all. >> @@ -1187,16 +1183,10 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 >> { >> int rc; >> struct pci_dev *pdev; >> - u8 slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); >> - unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, >> - PCI_CAP_ID_MSIX); > > You seem to be missing an empty newline here? > >> if ( !use_msi ) >> return 0; > > Same here. Same as above. >> @@ -1209,13 +1199,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 >> rc = 0; >> } >> else >> - { >> - uint16_t control = pci_conf_read16(PCI_SBDF3(seg, bus, devfn), >> - msix_control_reg(pos)); >> - >> - rc = msix_capability_init(pdev, pos, NULL, NULL, >> - multi_msix_capable(control)); >> - } >> + rc = msix_capability_init(pdev, NULL, NULL); >> pcidevs_unlock(); >> return rc; >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -324,6 +324,7 @@ static void apply_quirks(struct pci_dev >> static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) >> { >> struct pci_dev *pdev; >> + unsigned int pos; > > This chunk doesn't seem to match my current code, as there's an empty > newline between the declarations and list_for_each_entry. Same issue as above. >> @@ -350,6 +353,10 @@ static struct pci_dev *alloc_pdev(struct >> return NULL; >> } >> spin_lock_init(&msix->table_lock); >> + >> + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); >> + msix->nr_entries = msix_table_size(ctrl); > > Since you store the number of entries here, why not also store the > position of the MSI-X capability since it's also immutable? > > That would prevent having to fetch it again in msix_capability_init. I do consider this as something worthwhile to do in the future, but not here: The field to store this doesn't exist in struct arch_msix (yet), and hence would likely want moving from struct msi_attrib. This is beyond the scope of this patch. Jan
On Tue, Aug 06, 2019 at 04:47:28PM +0200, Jan Beulich wrote: > On 06.08.2019 16:25, Roger Pau Monné wrote: > > On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote: > > > --- a/xen/drivers/passthrough/pci.c > > > +++ b/xen/drivers/passthrough/pci.c > > > @@ -324,6 +324,7 @@ static void apply_quirks(struct pci_dev > > > static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) > > > { > > > struct pci_dev *pdev; > > > + unsigned int pos; > > > > This chunk doesn't seem to match my current code, as there's an empty > > newline between the declarations and list_for_each_entry. > > Same issue as above. Sorry for the noise. I jumped straight into this patch. > > > @@ -350,6 +353,10 @@ static struct pci_dev *ºalloc_pdev(struct > > > return NULL; > > > } > > > spin_lock_init(&msix->table_lock); > > > + > > > + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); > > > + msix->nr_entries = msix_table_size(ctrl); > > > > Since you store the number of entries here, why not also store the > > position of the MSI-X capability since it's also immutable? > > > > That would prevent having to fetch it again in msix_capability_init. > > I do consider this as something worthwhile to do in the future, but > not here: The field to store this doesn't exist in struct arch_msix > (yet), and hence would likely want moving from struct msi_attrib. > This is beyond the scope of this patch. Oh I see. So the position it's actually stored in msi_attrib, and is used by both MSI and MSI-X, in which case what I'm proposing would be worse, since the field would only be used by MSI-X. Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
On 06.08.2019 17:16, Roger Pau Monné wrote: > On Tue, Aug 06, 2019 at 04:47:28PM +0200, Jan Beulich wrote: >> On 06.08.2019 16:25, Roger Pau Monné wrote: >>> On Tue, Aug 06, 2019 at 03:10:40PM +0200, Jan Beulich wrote: >>>> @@ -350,6 +353,10 @@ static struct pci_dev *ºalloc_pdev(struct >>>> return NULL; >>>> } >>>> spin_lock_init(&msix->table_lock); >>>> + >>>> + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); >>>> + msix->nr_entries = msix_table_size(ctrl); >>> >>> Since you store the number of entries here, why not also store the >>> position of the MSI-X capability since it's also immutable? >>> >>> That would prevent having to fetch it again in msix_capability_init. >> >> I do consider this as something worthwhile to do in the future, but >> not here: The field to store this doesn't exist in struct arch_msix >> (yet), and hence would likely want moving from struct msi_attrib. >> This is beyond the scope of this patch. > > Oh I see. So the position it's actually stored in msi_attrib, and is > used by both MSI and MSI-X, in which case what I'm proposing would be > worse, since the field would only be used by MSI-X. Right, if we wanted to store it, we'd want to cover both MSI and MSI-X (and hence it would need to be a field outside of struct arch_msix). > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
On 06/08/2019 14:10, Jan Beulich wrote: > Rather than doing this every time we set up interrupts for a device > anew (and then in two distinct places) fill this invariant field > right after allocating struct arch_msix. > > While at it also obtain the MSI-X capability structure position just > once, in msix_capability_init(), rather than in each caller. > > Furthermore take the opportunity and eliminate the multi_msix_capable() > alias of msix_table_size(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -821,10 +821,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 * requested MSI-X entries with allocated irqs or non-zero for otherwise. **/ static int msix_capability_init(struct pci_dev *dev, - unsigned int pos, struct msi_info *msi, - struct msi_desc **desc, - unsigned int nr_entries) + struct msi_desc **desc) { struct arch_msix *msix = dev->msix; struct msi_desc *entry = NULL; @@ -838,6 +836,11 @@ static int msix_capability_init(struct p u8 slot = PCI_SLOT(dev->devfn); u8 func = PCI_FUNC(dev->devfn); bool maskall = msix->host_maskall; + unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, + PCI_CAP_ID_MSIX); + + if ( !pos ) + return -ENODEV; ASSERT(pcidevs_locked()); @@ -912,10 +915,9 @@ static int msix_capability_init(struct p u64 pba_paddr; u32 pba_offset; - msix->nr_entries = nr_entries; msix->table.first = PFN_DOWN(table_paddr); msix->table.last = PFN_DOWN(table_paddr + - nr_entries * PCI_MSIX_ENTRY_SIZE - 1); + msix->nr_entries * PCI_MSIX_ENTRY_SIZE - 1); WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->table.first, msix->table.last)); @@ -927,7 +929,7 @@ static int msix_capability_init(struct p msix->pba.first = PFN_DOWN(pba_paddr); msix->pba.last = PFN_DOWN(pba_paddr + - BITS_TO_LONGS(nr_entries) - 1); + BITS_TO_LONGS(msix->nr_entries) - 1); WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first, msix->pba.last)); } @@ -999,7 +1001,6 @@ static int msix_capability_init(struct p /* XXX How to deal with existing mappings? */ } } - WARN_ON(msix->nr_entries != nr_entries); WARN_ON(msix->table.first != (table_paddr >> PAGE_SHIFT)); ++msix->used_entries; @@ -1093,22 +1094,17 @@ static void __pci_disable_msi(struct msi **/ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc) { - int pos, nr_entries; struct pci_dev *pdev; - u16 control; u8 slot = PCI_SLOT(msi->devfn); u8 func = PCI_FUNC(msi->devfn); struct msi_desc *old_desc; ASSERT(pcidevs_locked()); pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn); - pos = pci_find_cap_offset(msi->seg, msi->bus, slot, func, PCI_CAP_ID_MSIX); - if ( !pdev || !pos ) + if ( !pdev || !pdev->msix ) return -ENODEV; - control = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); - nr_entries = multi_msix_capable(control); - if ( msi->entry_nr >= nr_entries ) + if ( msi->entry_nr >= pdev->msix->nr_entries ) return -EINVAL; old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSIX); @@ -1127,7 +1123,7 @@ static int __pci_enable_msix(struct msi_ __pci_disable_msi(old_desc); } - return msix_capability_init(pdev, pos, msi, desc, nr_entries); + return msix_capability_init(pdev, msi, desc); } static void _pci_cleanup_msix(struct arch_msix *msix) @@ -1187,16 +1183,10 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 { int rc; struct pci_dev *pdev; - u8 slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn); - unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, - PCI_CAP_ID_MSIX); if ( !use_msi ) return 0; - if ( !pos ) - return -ENODEV; - pcidevs_lock(); pdev = pci_get_pdev(seg, bus, devfn); if ( !pdev ) @@ -1209,13 +1199,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 rc = 0; } else - { - uint16_t control = pci_conf_read16(PCI_SBDF3(seg, bus, devfn), - msix_control_reg(pos)); - - rc = msix_capability_init(pdev, pos, NULL, NULL, - multi_msix_capable(control)); - } + rc = msix_capability_init(pdev, NULL, NULL); pcidevs_unlock(); return rc; --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -324,6 +324,7 @@ static void apply_quirks(struct pci_dev static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) { struct pci_dev *pdev; + unsigned int pos; list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) if ( pdev->bus == bus && pdev->devfn == devfn ) @@ -339,10 +340,12 @@ static struct pci_dev *alloc_pdev(struct pdev->domain = NULL; INIT_LIST_HEAD(&pdev->msi_list); - if ( pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), - PCI_CAP_ID_MSIX) ) + pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), + PCI_CAP_ID_MSIX); + if ( pos ) { struct arch_msix *msix = xzalloc(struct arch_msix); + uint16_t ctrl; if ( !msix ) { @@ -350,6 +353,10 @@ static struct pci_dev *alloc_pdev(struct return NULL; } spin_lock_init(&msix->table_lock); + + ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); + msix->nr_entries = msix_table_size(ctrl); + pdev->msix = msix; } @@ -358,7 +365,6 @@ static struct pci_dev *alloc_pdev(struct /* update bus2bridge */ switch ( pdev->type = pdev_type(pseg->nr, bus, devfn) ) { - int pos; u16 cap; u8 sec_bus, sub_bus; --- a/xen/include/asm-x86/msi.h +++ b/xen/include/asm-x86/msi.h @@ -171,7 +171,6 @@ int msi_free_irq(struct msi_desc *entry) #define msix_enable(control) control |= PCI_MSIX_FLAGS_ENABLE #define msix_disable(control) control &= ~PCI_MSIX_FLAGS_ENABLE #define msix_table_size(control) ((control & PCI_MSIX_FLAGS_QSIZE)+1) -#define multi_msix_capable msix_table_size #define msix_unmask(address) (address & ~PCI_MSIX_VECTOR_BITMASK) #define msix_mask(address) (address | PCI_MSIX_VECTOR_BITMASK)
Rather than doing this every time we set up interrupts for a device anew (and then in two distinct places) fill this invariant field right after allocating struct arch_msix. While at it also obtain the MSI-X capability structure position just once, in msix_capability_init(), rather than in each caller. Furthermore take the opportunity and eliminate the multi_msix_capable() alias of msix_table_size(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v5: New.