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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.02.22 11:44, Jan Beulich wrote:
> On 08.02.2022 10:27, Oleksandr Andrushchenko wrote:
>> On 08.02.22 11:13, Jan Beulich wrote:
>>> On 08.02.2022 09:32, Oleksandr Andrushchenko wrote:
>>>> On 07.02.22 18:28, Jan Beulich wrote:
>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>> @@ -1507,6 +1511,8 @@ static int assign_device(struct domain *d, u16 
>>>>>> seg, u8 bus, u8 devfn, u32 flag)
>>>>>>                             pci_to_dev(pdev), flag);
>>>>>>         }
>>>>>>     
>>>>>> +    rc = vpci_assign_device(d, pdev);
>>>>>> +
>>>>>>      done:
>>>>>>         if ( rc )
>>>>>>             printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
>>>>> There's no attempt to undo anything in the case of getting back an
>>>>> error. ISTR this being deemed okay on the basis that the tool stack
>>>>> would then take whatever action, but whatever it is that is supposed
>>>>> to deal with errors here wants spelling out in the description.
>>>> Why? I don't change the previously expected decision and implementation
>>>> of the assign_device function: I use error paths as they were used before
>>>> for the existing code. So, I see no clear reason to stress that the 
>>>> existing
>>>> and new code relies on the toolstack
>>> Saying half a sentence on this is helping review.
>> Ok
>>>>> What's important is that no caller up the call tree may be left with
>>>>> the impression that the device is still owned by the original
>>>>> domain. With how you have it, the device is going to be owned by the
>>>>> new domain, but not really usable.
>>>> This is not true: vpci_assign_device will call vpci_deassign_device
>>>> internally if it fails. So, the device won't be assigned in this case
>>> No. The device is assigned to whatever pdev->domain holds. Calling
>>> vpci_deassign_device() there merely makes sure that the device will
>>> have _no_ vPCI data and hooks in place, rather than something
>>> partial.
>> So, this patch is only dealing with vpci assign/de-assign
>> And it rolls back what it did in case of a failure
>> It also returns rc in assign_device to signal it has failed
>> What else is expected from this patch??
> Until now if assign_device() returns an error, this tells the caller
> that the device did not change ownership;
Not sure this is the case:
     if ( (rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
                           pci_to_dev(pdev), flag)) )
iommu_call can leave the new ownership even now without
vpci_assign_device. My understanding is that the roll-back is
expected to be performed by the toolstack and vpci_assign_device
doesn't prevent that by returning rc. Even more, before we discussed
that it would be good for vpci_assign_device to try recovering from
a possible error early which is done by calling vpci_deassign_device
internally.

So, if you want the things to be clearly handled without relying on the
toolstack then it is not vpci_assign_device introduced issue, but the
existing one, which needs (if there is a good reason) to be fixed
separately.
I think that new code doesn't make things worse. At least

>   in the worst case it either
> only moved to the quarantine domain, or the new owner may have been
> crashed. In no case is the device owned by an alive DomU. You're
> changing this property, and hence you need to make clear/sure that
> this isn't colliding with assumptions made elsewhere.
>
> Jan
>
>