Re: [PATCH v3 05/11] vpci: Refactor REGISTER_VPCI_INIT

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

On 2025/5/7 16:04, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 05:59:52AM +0000, Chen, Jiqian wrote:
>> On 2025/5/6 22:37, Roger Pau Monné wrote:
>>> On Mon, Apr 21, 2025 at 02:18:57PM +0800, Jiqian Chen wrote:
>>>>
>>>> +        if ( !is_ext )
>>>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>>>> +        else
>>>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
>>>> +
>>>> +        if ( !pos || !capability->init )
>>>
>>> Isn't it bogus to have a vpci_capability_t entry with a NULL init
>>> function?
>> Since I don't add fini_x() function for capabilities and also add check "if 
>> ( capability->fini )" below,
>> so I add this NULL check here.
>> I will remove it if you think it is unnecessary.
>> Should I also remove the NULL check for fini?
> 
> I think `fini` is fine to be NULL, but I don't see a case for `init`
> being NULL?
> 
> Maybe I'm missing some use-case, but I expect capabilities will always
> need some kind of initialization (iow: setting up handlers) otherwise
> it's just a no-op.
Got it. I will just remove the check of init.

> 
>>>> +        if ( rc )
>>>> +            return rc;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  void vpci_deassign_device(struct pci_dev *pdev)
>>>>  {
>>>>      unsigned int i;
>>>> @@ -128,7 +158,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>>>  
>>>>  int vpci_assign_device(struct pci_dev *pdev)
>>>>  {
>>>> -    unsigned int i;
>>>>      const unsigned long *ro_map;
>>>>      int rc = 0;
>>>>  
>>>> @@ -159,14 +188,13 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>>          goto out;
>>>>  #endif
>>>>  
>>>> -    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>>>> -    {
>>>> -        rc = __start_vpci_array[i](pdev);
>>>> -        if ( rc )
>>>> -            break;
>>>> -    }
>>>> +    rc = vpci_init_header(pdev);
>>>> +    if ( rc )
>>>> +        goto out;
>>>> +
>>>> +    rc = vpci_init_capabilities(pdev);
>>>>  
>>>> - out: __maybe_unused;
>>>> + out:
>>>>      if ( rc )
>>>>          vpci_deassign_device(pdev);
>>>>  
>>>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>>>> index 9d47b8c1a50e..8e815b418b7d 100644
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev 
>>>> *pdev, unsigned int reg,
>>>>  typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
>>>>                            uint32_t val, void *data);
>>>>  
>>>> -typedef int vpci_register_init_t(struct pci_dev *dev);
>>>> -
>>>> -#define VPCI_PRIORITY_HIGH      "1"
>>>> -#define VPCI_PRIORITY_MIDDLE    "5"
>>>> -#define VPCI_PRIORITY_LOW       "9"
>>>> +typedef struct {
>>>> +    unsigned int id;
>>>> +    bool is_ext;
>>>> +    int (*init)(struct pci_dev *pdev);
>>>> +    void (*fini)(struct pci_dev *pdev);
>>>> +} vpci_capability_t;
>>>>  
>>>>  #define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
>>>>  
>>>> @@ -29,9 +30,20 @@ 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_CAP(cap, x, y, ext) \
>>>
>>> x and y are not very helpful identifier names, better use some more
>>> descriptive naming, init and fini?  Same below.
>> init and fini seems not good. They are conflict with the hook name of below 
>> vpci_capability_t.
>> Maybe init_func and fini_func ?
> 
> Oh, I see.  Can I recommend to name fields init and destroy or cleanup
> (instead of fini), and then use finit and fdestroy/fclean as macro
> parameters?
> 
> I don't think it's common in Xen to name cleanup functions 'fini'.  I
> understand this is a question of taste, it's mostly for coherence with
> the rest of the code base.
OK, will change to "init    cleanup" and "finit     fclean"

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.