[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/2] xen/vpci: header: filter PCI capabilities
On 8/17/23 08:57, Jan Beulich wrote: > On 16.08.2023 20:50, Stewart Hildebrand wrote: >> If there are no capabilities to be exposed to the guest, a future status >> register handler likely would want to mask the PCI_STATUS_CAP_LIST bit. See >> [1] >> for a suggestion of how this might be tracked in struct vpci_header. > > Can we actually get away without doing this right away? I'm not sure > consumers are required to range check what they read from PCI_CAPABILITY_LIST. I think you're right, this should be done right away. I'll add a status register handler with PCI_STATUS_CAP_LIST bit masking in the next version of the series. This will include a modification to vpci_write_helper() be able to handle rw1c registers properly. >> --- a/xen/drivers/pci/pci.c >> +++ b/xen/drivers/pci/pci.c >> @@ -39,27 +39,27 @@ int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 >> func, u8 cap) >> return 0; >> } >> >> -int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap) >> +int pci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos, bool >> (*is_match)(uint8_t), >> + int *ttl) > > Why plain int? When values can't go negative, respective variables generally > want to be of unsigned types. I'll change to unsigned int. >> { >> - u8 id; >> - int ttl = 48; >> + uint8_t id; >> >> - while ( ttl-- ) >> + while ( (*ttl)-- > 0 ) > > I don't see why you add "> 0" here. I'll remove it. >> { >> - pos = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos); >> + pos = pci_conf_read8(sbdf, pos); >> if ( pos < 0x40 ) >> break; >> >> - pos &= ~3; >> - id = pci_conf_read8(PCI_SBDF(seg, bus, devfn), 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) ) >> return pos; >> >> - pos += PCI_CAP_LIST_NEXT; >> + pos = (pos & ~3) + PCI_CAP_LIST_NEXT; >> } >> + >> return 0; >> } > > As said in context of v1, this function should remain usable for its > original purpose. That, to me, includes the caller not needing to care about > ttl. I could see you convert the original function the way you do, but under > a new name, and then implement the original one simply in terms of this more > general purpose function. Will do. > Also, while I appreciate the sbdf conversion, that wants to be a separate > patch, which would then want to take care of the sibling functions as well. OK, will do. To be clear, this means converting the following 4 functions to use pci_sbdf_t, along with all the call sites: pci_find_cap_offset pci_find_next_cap pci_find_ext_capability pci_find_next_ext_capability >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -513,6 +513,18 @@ static void cf_check rom_write( >> rom->addr = val & PCI_ROM_ADDRESS_MASK; >> } >> >> +static bool cf_check vpci_cap_supported(uint8_t id) >> +{ >> + switch ( id ) >> + { >> + case PCI_CAP_ID_MSI: >> + case PCI_CAP_ID_MSIX: >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> static int cf_check init_bars(struct pci_dev *pdev) >> { >> uint16_t cmd; >> @@ -544,6 +556,60 @@ 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) >> + == 0 ) > > This fits on a single line when written this more commonly used way: > > if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) > ) I'll do this. > Otherwise it needs wrapping differently - binary operators at a wrapping > point belong on the earlier line in our style. > >> + { >> + /* RAZ/WI */ >> + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, >> + PCI_CAPABILITY_LIST, 1, NULL); > > This last NULL is likely misleading to readers: It does not obviously > represent the value 0 cast to void *. (Same then for the extended cap > handler at the end.) I'll change to (void *)0 >> + if ( rc ) >> + return rc; >> + } >> + else >> + { >> + /* Only expose capabilities to the guest that vPCI can handle. >> */ >> + uint8_t next; >> + int ttl = 48; >> + >> + next = pci_find_next_cap(pdev->sbdf, PCI_CAPABILITY_LIST, >> + vpci_cap_supported, &ttl); >> + >> + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, >> + PCI_CAPABILITY_LIST, 1, >> + (void *)(uintptr_t)next); >> + if ( rc ) >> + return rc; >> + >> + while ( next && (ttl > 0) ) > > Don't you need to mask off the low two bits first (rather than [only] ... Yes, I'll fix this. > >> + { >> + uint8_t pos = next; >> + >> + next = pci_find_next_cap(pdev->sbdf, pos + >> PCI_CAP_LIST_NEXT, >> + vpci_cap_supported, &ttl); >> + >> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL, >> + pos + PCI_CAP_LIST_ID, 1, NULL); >> + if ( rc ) >> + return rc; >> + >> + rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, >> + pos + PCI_CAP_LIST_NEXT, 1, >> + (void *)(uintptr_t)next); >> + if ( rc ) >> + return rc; >> + >> + next &= ~3; > > ... here)? > > Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |