Re: [PATCH v1 3/3] xen/vpci: header: filter PCI capabilities

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2023 23:11, Stewart Hildebrand wrote:
> On 8/14/23 10:58, Jan Beulich wrote:
>> On 10.08.2023 21:12, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -513,6 +513,36 @@ static void cf_check rom_write(
>>>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>>>  }
>>>
>>> +static uint8_t vpci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos)
>>> +{
>>> +    uint8_t id;
>>> +    int ttl;
>>> +
>>> +    if ( pos < 0x40 )
>>> +        pos = pci_conf_read8(sbdf, PCI_CAPABILITY_LIST);
>>> +    else
>>> +        pos = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_NEXT);
>>
>> How about avoiding the if/else by having the caller pass in a useful
>> value, rather than PCI_CAPABILITY_LIST? I.e.
>>
>> #define PCI_CAP_LIST_FIRST (PCI_CAPABILITY_LIST - PCI_CAP_LIST_NEXT)
> 
> OK, yes, I will eliminate the if/else.
> 
>>
>>> +    for ( ttl = 48; ttl > 0; ttl-- )
>>> +    {
>>> +        if ( pos < 0x40 )
>>> +            break;
>>> +
>>> +        pos &= ~3;
>>> +        id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
>>> +
>>> +        if ( id == 0xff )
>>> +            break;
>>> +
>>> +        if ( id == PCI_CAP_ID_MSI ||
>>> +             id == PCI_CAP_ID_MSIX )
>>> +            return pos;
>>
>> Can this please start out as switch() right away?
> 
> Yes, certainly.
> 
>>> +        pos = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_NEXT);
>>> +    }
>>> +    return 0;
>>> +}
>>
>> Nit: Blank line please ahead of main function return point.
>>
>> I also notice that the function isn't really vPCI-specific in any way
>> (except for the specific PCI_CAP_ID_* compared against). Would it
>> perhaps make sense to have it be a general utility function, living in
>> xen/drivers/pci/pci.c next to its relatives?
> 
> Yes. The the PCI_CAP_ID_* comparisons were the only reason I initially 
> decided not to use the existing pci_find_next_cap() function, which performs 
> an almost identical task. I just noticed that the existing 
> pci_find_next_cap() doesn't appear to have any callers. Given this, I'd 
> prefer to modify the existing pci_find_next_cap() to suit our needs.

Please modify the function only if it then remains (easily) usable for
its original purpose. Even if right now without callers, it exists for
a reason. (How to deal with the Misra unreachable code aspect in such
cases remains to be decided.)

Jan