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