Re: [PATCH v6 2/8] vpci: Refactor REGISTER_VPCI_INIT

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

On 2025/6/18 22:05, Jan Beulich wrote:
> On 12.06.2025 11:29, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -703,9 +703,13 @@ static int cf_check init_msix(struct pci_dev *pdev)
>>      pdev->vpci->msix = msix;
>>      list_add(&msix->next, &d->arch.hvm.msix_tables);
>>  
>> -    return 0;
>> +    spin_lock(&pdev->vpci->lock);
>> +    rc = vpci_make_msix_hole(pdev);
>> +    spin_unlock(&pdev->vpci->lock);
> 
> If you add a call to vpci_make_msix_hole() here, doesn't it need (or at
> least want) removing somewhere else? Otherwise maybe a code comment is
> warranted next to the new call site?
The removing operation in modify_bars() and vpci_deassign_device() is not 
enough?

> 
>> @@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>>   */
>>  #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
>>  
>> -#define REGISTER_VPCI_INIT(x, p)                \
>> -  static vpci_register_init_t *const x##_entry  \
>> -               __used_section(".data.vpci." p) = (x)
>> +#define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
>> +    static const vpci_capability_t finit##_t = { \
>> +        .id = (cap), \
>> +        .init = (finit), \
>> +        .cleanup = (fclean), \
>> +        .is_ext = (ext), \
>> +    }; \
>> +    static const vpci_capability_t *const finit##_entry  \
>> +        __used_section(".data.rel.ro.vpci") = &finit##_t
> 
> Could you remind me why the extra level of indirection is necessary here?
> That is, why can't .data.rel.ro.vpci be an array of vpci_capability_t?
You mean I should change to be:
#define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
    static const vpci_capability_t finit##_t \
        __used_section(".data.rel.ro.vpci") = { \
        .id = (cap), \
        .init = (finit), \
        .cleanup = (fclean), \
        .is_ext = (ext), \
    }

Right?

> 
> Jan

-- 
Best regards,
Jiqian Chen.