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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2021 10:33, Oleksandr Andrushchenko wrote:
> 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.

Well, okay, but please make this explicit in the description then.

Jan