Re: [PATCH 2/9] vpci: Add hooks for PCI device assign/de-assign

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

Hello, Jan!

On 06.09.21 16:23, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -864,6 +864,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>       if ( ret )
>>           goto out;
>>   
>> +    ret = vpci_deassign_device(d, pdev);
>> +    if ( ret )
>> +        goto out;
>> +
>>       if ( pdev->domain == hardware_domain  )
>>           pdev->quarantine = false;
>>   
>> @@ -1425,6 +1429,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>           rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag);
>>       }
>>   
>> +    if ( rc )
>> +        goto done;
>> +
>> +    rc = vpci_assign_device(d, pdev);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> I have to admit that I'm worried by the further lack of unwinding in
> case of error: We're not really good at this, I agree, but it would
> be quite nice if the problem didn't get worse. At the very least if
> the device was de-assigned from Dom0 and assignment to a DomU failed,
> imo you will want to restore Dom0's settings.

In the current design the error path is handled by the toolstack

via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,

so this is why it is "ok" to have the code structured in the

assign_device as it is, e.g. roll back will be handled on deassign_device.

So, it is up to the toolstack to re-assign the device to Dom0 or DomIO(?)

in case of error and we do rely on the toolstack in Xen.

>
> Also in the latter case don't you need to additionally call
> vpci_deassign_device() for the prior owner of the device?

Even if we wanted to help the toolstack with the roll-back in case of an error

this would IMO make things even worth, e.g. we will de-assign for vPCI, but will

leave IOMMU path untouched which would result in some mess.

So, my only guess here is that we should rely on the toolstack completely as

it was before PCI passthrough on Arm.

>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>>   
>>       return rc;
>>   }
>> +
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct domain *d, struct pci_dev *dev)
>> +{
>> +    /* It only makes sense to assign for hwdom or guest domain. */
>> +    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
> Please don't open-code is_system_domain(). I also think you want to
> flip the two sides of the ||, to avoid evaluating whatever has_vcpi()
> expands to for system domains. (Both again below.)
Good catch, I missed is_system_domain completely, thank you!
>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>>   /* Add vPCI handlers to device. */
>>   int __must_check vpci_add_handlers(struct pci_dev *dev);
>>   
>> +/* Notify vPCI that device is assigned to guest. */
>> +int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);
> Is the expectation that "dev" may get altered? If not, it may want to
> become pointer-to-const. (For "d" there might be the need to acquire
> locks, so I guess it might not be a god idea to constify that one.)
Just checked that and it is indeed possible to constify. Will do
>
> I also think that one comment ought to be enough for the two functions.
Sure
>
> Jan
>
Thank you,

Oleksandr