| Message ID | 20221101135749.4477-1-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pci: Abort if pci_add_capability fails | expand |
On 1/11/22 14:57, Akihiko Odaki wrote: > pci_add_capability() checks whether capabilities overlap, and notifies > its caller so that it can properly handle the case. However, in the > most cases, the capabilities actually never overlap, and the interface > incurred extra error handling code, which is often incorrect or > suboptimal. For such cases, pci_add_capability() can simply abort the > execution if the capabilities actually overlap since it should be a > programming error. > > This change handles the other cases: hw/vfio/pci depends on the check to > decide MSI and MSI-X capabilities overlap with another. As they are > quite an exceptional and hw/vfio/pci knows much about PCI capabilities, > adding code specific to the cases to hw/vfio/pci still results in less > code than having error handling code everywhere in total. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/pci/pci.c | 34 ++++++++++++++++++++++------------ > hw/vfio/pci.c | 15 ++++++++++++++- > include/hw/pci/pci.h | 3 +++ > 3 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 2f450f6a72..b53649d1fd 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2512,6 +2512,25 @@ static void pci_del_option_rom(PCIDevice *pdev) > pdev->has_rom = false; > } > > +bool pci_check_capability_overlap(PCIDevice *pdev, uint8_t cap_id, > + uint8_t offset, uint8_t size, Error **errp) > +{ > + int i; > + > + for (i = offset; i < offset + size; i++) { > + if (pdev->used[i]) { > + error_setg(errp, > + "%s:%02x:%02x.%x PCI capability %x at offset %x overlaps existing capability %x at offset %x", > + pci_root_bus_path(pdev), pci_dev_bus_num(pdev), > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > + cap_id, offset, pci_find_capability_at_offset(pdev, i), i); > + return true; > + } > + } > + > + return false; > +} I apologize for jumping at v8 :/ Per the Error API, function taking an Error** as last argument should return TRUE on success; or FALSE on error and setting the *errp argument. Your function return 'true' on error. The confusion might come from its name 'pci_check_capability_overlap'. > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index b54b6ef88f..77b264c17e 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -390,6 +390,9 @@ void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem, > void pci_unregister_vga(PCIDevice *pci_dev); > pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num); > Please document function prototype of public APIs. > +bool pci_check_capability_overlap(PCIDevice *pdev, uint8_t cap_id, > + uint8_t offset, uint8_t size, Error **errp); > + > int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > uint8_t offset, uint8_t size, > Error **errp); Also, consider configuring scripts/git.orderfile :) Regards, Phil.