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

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

On 2025/6/20 14:38, Jan Beulich wrote:
> 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?
Sorry, I misunderstood you in my last email.
After here's change, it seems the call in modify_decoding() is redundant.
What's your taste? Removing the call in modify_decoding() or adding a code 
comment?

>> 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.
Got it.
One more question, if change to be that, then how should I modify the 
definition of VPCI_ARRAY?
Is POINTER_ALIGN still right?
Since I encountered errors that the values of __start_vpci_array are not right 
when I use them in vpci_init_capabilities().

> 
> Jan

-- 
Best regards,
Jiqian Chen.