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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 10:02, Chen, Jiqian wrote:
> 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?

I'd prefer the other call to be dropped if it's provably redundant. But Roger
has the final say here anyway.

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

Yes. The struct doesn't require bigger alignment afaics. (In fact in principle
no alignment should need specifying there, except that this would require
keeping the section separate in the final image. Which I don't think we want.)

> Since I encountered errors that the values of __start_vpci_array are not 
> right when I use them in vpci_init_capabilities().

Details please.

Jan