On 8/31/23 08:11, Jan Beulich wrote: > On 28.08.2023 19:56, Stewart Hildebrand wrote: >> Currently, Xen vPCI only supports virtualizing the MSI and MSI-X >> capabilities. >> Hide all other PCI capabilities (including extended capabilities) from domUs >> for >> now, even though there may be certain devices/drivers that depend on being >> able >> to discover certain capabilities. >> >> We parse the physical PCI capabilities linked list and add vPCI register >> handlers for the next elements, inserting our own next value, thus >> presenting a >> modified linked list to the domU. >> >> Introduce helper functions vpci_hw_read8 and vpci_read_val. The vpci_read_val >> helper function returns a fixed value, which may be used for RAZ registers, >> or >> registers whose value doesn't change. >> >> Introduce pci_find_next_cap_ttl() helper while adapting the logic from >> pci_find_next_cap() to suit our needs, and implement the existing >> pci_find_next_cap() in terms of the new helper. >> >> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com> > > Reviewed-by: Jan Beulich <jbeul...@suse.com>> > Nevertheless a couple of remarks: > >> --- a/xen/drivers/pci/pci.c >> +++ b/xen/drivers/pci/pci.c >> @@ -39,31 +39,44 @@ unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, >> unsigned int cap) >> return 0; >> } >> >> -unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos, >> - unsigned int cap) >> +unsigned int pci_find_next_cap_ttl(pci_sbdf_t sbdf, unsigned int pos, >> + bool (*is_match)(unsigned int, unsigned >> int), >> + unsigned int userdata, unsigned int *ttl) >> { >> - u8 id; >> - int ttl = 48; >> + unsigned int id; >> >> - while ( ttl-- ) >> + while ( (*ttl)-- ) >> { >> pos = pci_conf_read8(sbdf, pos); >> if ( pos < 0x40 ) >> break; >> >> - pos &= ~3; >> - id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID); >> + id = pci_conf_read8(sbdf, (pos & ~3) + PCI_CAP_LIST_ID); >> >> if ( id == 0xff ) >> break; >> - if ( id == cap ) >> + if ( is_match(id, userdata) ) >> return pos; >> >> - pos += PCI_CAP_LIST_NEXT; >> + pos = (pos & ~3) + PCI_CAP_LIST_NEXT; >> } >> + >> return 0; >> } >> >> +static bool cf_check is_cap_match(unsigned int id1, unsigned int id2) >> +{ >> + return id1 == id2; >> +} > > Personally I would have preferred to get away without yet another hook > function here, by ... > >> +unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos, >> + unsigned int cap) >> +{ >> + unsigned int ttl = 48; >> + >> + return pci_find_next_cap_ttl(sbdf, pos, is_cap_match, cap, &ttl) & ~3; > > ... passing NULL here and then suitably handling the case in that > common helper.
Thinking in terms of reducing the amount of dead code, I'll change it >> @@ -561,6 +573,71 @@ static int cf_check init_bars(struct pci_dev *pdev) >> if ( rc ) >> return rc; >> >> + if ( !is_hardware_domain(pdev->domain) ) >> + { >> + if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & >> PCI_STATUS_CAP_LIST) ) >> + { >> + /* RAZ/WI */ >> + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, >> + PCI_CAPABILITY_LIST, 1, (void *)0); >> + if ( rc ) >> + return rc; >> + } >> + else >> + { >> + /* Only expose capabilities to the guest that vPCI can handle. >> */ >> + uint8_t next; > > If this was "unsigned long", ... > >> + unsigned int ttl = 48; >> + >> + next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST, >> + vpci_cap_supported, 0, &ttl); >> + >> + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, >> + PCI_CAPABILITY_LIST, 1, >> + (void *)(uintptr_t)next); > > ... you'd avoid the need for the double cast here and again below. Yet > then I realize that Misra would take offence at us doing so ... As ugly as that double cast is, I think I prefer the next and pos declarations have consistent types (which I had intended to be unsigned int to match the prior patches, not uint8_t). As well as not making the MISRA situation any worse. The casts, after all, make it excruciatingly obvious what we're doing here, I hope. Stew