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

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

On 2025/6/24 16:05, Jan Beulich wrote:
> 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.
OK, let's wait Roger's input.

> 
>>>> 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.
After changing __start_vpci_array to be vpci_capability_t array, codes will be 
(maybe I modified wrong somewhere):

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index c51bbb8abb19..9f2f438b4fdd 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -36,8 +36,8 @@ struct vpci_register {
 };

 #ifdef __XEN__
-extern const vpci_capability_t *const __start_vpci_array[];
-extern const vpci_capability_t *const __end_vpci_array[];
+extern vpci_capability_t __start_vpci_array[];
+extern vpci_capability_t __end_vpci_array[];
 #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)

 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
@@ -255,7 +255,7 @@ static int vpci_init_capabilities(struct pci_dev *pdev)
 {
     for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
     {
-        const vpci_capability_t *capability = __start_vpci_array[i];
+        const vpci_capability_t *capability = &__start_vpci_array[i];
         const unsigned int cap = capability->id;
         const bool is_ext = capability->is_ext;
         int rc;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index f4ec1c25922d..77750dd4131a 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -31,14 +31,13 @@ typedef struct {
 #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)

 #define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
-    static const vpci_capability_t finit##_t = { \
+    static vpci_capability_t finit##_entry \
+        __used_section(".data.rel.ro.vpci") = { \
         .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
+    }

 #define REGISTER_VPCI_CAP(cap, finit, fclean) \
     REGISTER_VPCI_CAPABILITY(cap, finit, fclean, false)

I print the value of NUM_VPCI_INIT, it is a strange number 
(6148914691236517209).


> 
> Jan

-- 
Best regards,
Jiqian Chen.