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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2025 08:39, Chen, Jiqian wrote:
> 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?

I fear I don't understand this reply of yours. Which suggests that the patch
description may want extending as to this part of the change.

>>> @@ -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?

Yes, subject to the earlier comments on the identifier choice.

Jan