Re: [PATCH v3 04/11] vpci/header: Add and remove register handlers dynamically

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 02.11.21 12:03, Roger Pau Monné wrote:
> On Mon, Nov 01, 2021 at 09:18:17AM +0000, Oleksandr Andrushchenko wrote:
>>>> +    if ( rc )
>>>> +        gdprintk(XENLOG_ERR,
>>>> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
>>>> +                 &pdev->sbdf, d, rc);
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev 
>>>> *pdev)
>>>> +{
>>>> +    /* Remove previously added registers. */
>>>> +    vpci_remove_device_registers(pdev);
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>    /*
>>>>     * Local variables:
>>>>     * mode: C
>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>> index 0fe86cb30d23..702f7b5d5dda 100644
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct 
>>>> pci_dev *dev)
>>>>        if ( is_system_domain(d) || !has_vpci(d) )
>>>>            return 0;
>>>>    
>>>> -    return 0;
>>>> +    return vpci_bar_add_handlers(d, dev);
>>>>    }
>>>>    
>>>>    /* Notify vPCI that device is de-assigned from guest. */
>>>> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const 
>>>> struct pci_dev *dev)
>>>>        if ( is_system_domain(d) || !has_vpci(d) )
>>>>            return 0;
>>>>    
>>>> -    return 0;
>>>> +    return vpci_bar_remove_handlers(d, dev);
>>> I think it would be better to use something similar to
>>> REGISTER_VPCI_INIT here, otherwise this will need to be modified every
>>> time a new capability is handled by Xen.
>>>
>>> Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
>>> to be used for guest initialization?
>>>
>>>>    }
>>>>    #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>>>    
>>>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>>>> index ecc08f2c0f65..fd822c903af5 100644
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, 
>>>> unsigned int reg,
>>>>     */
>>>>    bool __must_check vpci_process_pending(struct vcpu *v);
>>>>    
>>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>> +/* Add/remove BAR handlers for a domain. */
>>>> +int vpci_bar_add_handlers(const struct domain *d,
>>>> +                          const struct pci_dev *pdev);
>>>> +int vpci_bar_remove_handlers(const struct domain *d,
>>>> +                             const struct pci_dev *pdev);
>>>> +#endif
>>> This would then go away if we implement a mechanism similar to
>>> REGISTER_VPCI_INIT.
>>>
>>> Thanks, Roger.
>> Ok, so I can extend REGISTER_VPCI_INIT with an action parameter:
>>
>> "There are number of actions to be taken while first initializing vPCI
>> for a PCI device or when the device is assigned to a guest or when it
>> is de-assigned and so on.
>> Every time a new action is needed during these steps we need to call some
>> relevant function to handle that. Make it is easier to track the required
>> steps by extending REGISTER_VPCI_INIT machinery with an action parameter
>> which shows which exactly step/action is being performed."
>>
>> So, we have
>>
>> -typedef int vpci_register_init_t(struct pci_dev *dev);
>> +enum VPCI_INIT_ACTION {
>> +  VPCI_INIT_ADD,
>> +  VPCI_INIT_ASSIGN,
>> +  VPCI_INIT_DEASSIGN,
>> +};
>> +
>> +typedef int vpci_register_init_t(struct pci_dev *dev,
>> +                                 enum VPCI_INIT_ACTION action);
>>
>> and, for example,
>>
>> @@ -452,6 +452,9 @@ static int init_bars(struct pci_dev *pdev)
>>        struct vpci_bar *bars = header->bars;
>>        int rc;
>>
>> +    if ( action != VPCI_INIT_ADD )
>> +        return 0;
>> +
>>
>> I was thinking about adding dedicated machinery similar to 
>> REGISTER_VPCI_INIT,
>> e.g. REGISTER_VPCI_{ASSIGN|DEASSIGN} + dedicated sections in the linker 
>> scripts,
>> but it seems not worth it: these steps are only executed at device 
>> init/assign/deassign,
>> so extending the existing approach doesn't seem to hurt performance much.
>>
>> Please let me know if this is what you mean, so I can re-work the relevant 
>> code.
> I'm afraid I'm still unsure whether we need an explicit helper to
> execute when assigning a device, rather than just using the current
> init helpers (init_bars &c).
>
> You said that sizing the BARs when assigning to a domU was not
> possible [0], but I'm missing an explanation of why it's not possible,
> as I think that won't be an issue on x86 [1].
I am in the process of re-working this and the relevant patches.
At the moment I have those helpers, but it seems I can remove them.
Once I finish the series I (most probably) will remove those.
>
> Thanks, Roger.
>
> [0] 
> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/368bf4b5-f9fd-76a6-294e-dbb93a18e73f@xxxxxxxx/__;!!GF_29dbcQIUBPA!mGz2uzJKNZsMr3R8awokkSOjo8ETjOS9N-JVkTIOJW5BYxvKgtZrKamPJq59I5u2GCDnsY4dQQ$
>  [lore[.]kernel[.]org]
> [1] 
> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/YXlxmdYdwptakDDK@Air-de-Roger/__;!!GF_29dbcQIUBPA!mGz2uzJKNZsMr3R8awokkSOjo8ETjOS9N-JVkTIOJW5BYxvKgtZrKamPJq59I5u2GCAHHkrD1g$
>  [lore[.]kernel[.]org]