Re: [PATCH v2 1/2] xen/vpci: header: filter PCI capabilities

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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