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

[Jan Beulich <jbeulich@suse.com>]

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.

>> 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.

>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -99,6 +99,33 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>>   
>>>       return rc;
>>>   }
>>> +
>>> +#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;
>>> +
>>> +    if ( !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    rc = vpci_add_handlers(pdev);
>>> +    if ( rc )
>>> +        vpci_deassign_device(d, pdev);
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +/* Notify vPCI that device is de-assigned from guest. */
>>> +void vpci_deassign_device(struct domain *d, struct pci_dev *pdev)
>>> +{
>>> +    if ( !has_vpci(d) )
>>> +        return;
>>> +
>>> +    vpci_remove_device(pdev);
>>> +}
>>> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>> While for the latter function you look to need two parameters, do you
>> really need them also in the former one?
> Do you mean instead of passing d we could just use pdev->domain?
> int vpci_assign_device(struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return 0;

Yes.

> Yes, we probably can, but the rest of functions called from assign_device
> are accepting both d and pdev, so not sure why would we want these
> two be any different. Any good reason not to change others as well then?

Yes: Prior to the call of the ->assign_device() hook, d != pdev->domain.
It is the _purpose_ of this function to change ownership of the device.

Jan