Re: [PATCH v5 05/14] vpci: add hooks for PCI device assign/de-assign

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 31.01.22 10:45, Oleksandr Andrushchenko wrote:
> Hi, Roger!
>
> On 13.01.22 13:40, Roger Pau Monné wrote:
>> On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>> +/* Notify vPCI that device is assigned to guest. */
>>> +int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>>> +{
>>> +    int rc;
>>> +
>>> +    /* It only makes sense to assign for hwdom or guest domain. */
>>> +    if ( is_system_domain(d) || !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    spin_lock(&pdev->vpci_lock);
>>> +    rc = run_vpci_init(pdev);
>> Following my comment below, this will likely need to call
>> vpci_add_handlers in order to allocate the pdev->vpci field.
>>
>> It's not OK to carry the contents of pdev->vpci across domain
>> assignations, as the device should be reset, and thus the content of
>> pdev->vpci would be stale.
>>
>>> +    spin_unlock(&pdev->vpci_lock);
>>> +    if ( rc )
>>> +        vpci_deassign_device(d, pdev);
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +/* Notify vPCI that device is de-assigned from guest. */
>>> +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev)
>>> +{
>>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
>>> +    if ( is_system_domain(d) || !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    spin_lock(&pdev->vpci_lock);
>>> +    vpci_remove_device_handlers_locked(pdev);
>> You need to free the pdev->vpci structure on deassign. I would expect
>> the device to be reset on deassign, so keeping the pdev->vpci contents
>> would be wrong.
> Sure, I will re-allocate pdev->vpci then
After thinking a bit more on this I have realized that we cannot free
pdev->vpci on de-assign. The reason for that is the fact that vpci
structure contains vital data which is collected and managed at different
stages: for example, BAR types are collected while we run for the
hardware domain and in init_bars we collect the types of the BARS etc.
This is then used while assigning device to construct guest's representation
of the device. Freeing vpci will lead to that data is lost and the required
data is not populated into vpci.
So, it is no possible to free vpci structure and I am about to leave the
approach as it is.
>> Thanks, Roger.
> Thank you,
> Oleksandr
Thank you,
Oleksandr