Re: [PATCH v3 04/11] vpci/header: Emulate extended capability list for dom0

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/5/6 22:14, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:18:56PM +0800, Jiqian Chen wrote:
>> Add a new function to emulate extended capability list for dom0,
>> and call it in init_header(). So that it will be easy to hide a
>> extended capability whose initialization fails.
>>
>> As for the extended capability list of domU, just move the logic
>> into above function and keep hiding it for domU.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> v2->v3 changes:
>> * In vpci_init_ext_capability_list(), when domain is domU, directly return 
>> after adding a handler(hiding all extended capability for domU).
>> * In vpci_init_ext_capability_list(), change condition to be "while ( pos >= 
>> 0x100U && ttl-- )" instead of "while ( pos && ttl-- )".
>> * Add new function vpci_hw_write32, and pass it to extended capability 
>> handler for dom0.
>>
>> v1->v2 changes:
>> new patch
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/header.c | 36 ++++++++++++++++++++++++++++--------
>>  xen/drivers/vpci/vpci.c   |  6 ++++++
>>  xen/include/xen/vpci.h    |  2 ++
>>  3 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index c98cd211d9d7..ee94ad8e5037 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -817,6 +817,31 @@ static int vpci_init_capability_list(struct pci_dev 
>> *pdev)
>>                                    PCI_STATUS_RSVDZ_MASK);
>>  }
>>  
>> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
>> +{
>> +    unsigned int pos = PCI_CFG_SPACE_SIZE, ttl = 480;
>> +
>> +    if ( !is_hardware_domain(pdev->domain) )
>> +        /* Extended capabilities read as zero, write ignore for guest */
>> +        return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                 pos, 4, (void *)0);
>> +
>> +    while ( pos >= PCI_CFG_SPACE_SIZE && ttl-- )
>> +    {
>> +        uint32_t header = pci_conf_read32(pdev->sbdf, pos);
>> +        int rc;
> 
> I'm thinking it might be helpful to avoid setting the handler for the
> last capability on the list, or simply for devices that have no
> extended capabilities at all:
> 
> if ( PCI_EXT_CAP_NEXT(header) >= PCI_CFG_SPACE_SIZE )
> {
>     int rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
>                                pos, 4, (void *)(uintptr_t)header);
> 
>     if ( rc )
>         return rc;
> }
But if adding this check, there is a problem, think about this situation:
a device only has one extended capability, then under your check, it does not 
add handler for it,
if the initialization of that extended capability fails, we can't hide it by 
removing handler from vpci.
If you want to avoid adding handler for devices that have no extended 
capabilities.
I think adding check
If ( header == 0 )
    return 0;
is enough.

> 
> Otherwise on systems with a lot of devices it can be quite wasteful to
> set a handler to just return the next capability as 0.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.