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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 01, 2022 at 08:56:49AM +0000, Oleksandr Andrushchenko wrote:
> 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.

We discussed this on IRC, and we have agreed that it's possible to
free pdev->vpci on deassign since in any case we need to call
init_bars (and other capability init functions) when the device is
assigned to setup the register traps and fetch the required
information in order to fill pdev->vpci.

Roger.