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.
