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

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

On Wed, Jul 26, 2023 at 01:38:30AM +0000, Volodymyr Babchuk wrote:
> 
> Hi Roger,
> 
> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
> 
> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >> @@ -1509,6 +1517,19 @@ static int assign_device(struct domain *d, u16 seg, 
> >> u8 bus, u8 devfn, u32 flag)
> >>          rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
> >>                          pci_to_dev(pdev), flag);
> >>      }
> >> +    if ( rc )
> >> +        goto done;
> >> +
> >> +    devfn = pdev->devfn;
> >> +    write_lock(&pdev->domain->pci_lock);
> >> +    rc = vpci_assign_device(pdev);
> >> +    write_unlock(&pdev->domain->pci_lock);
> >> +    if ( rc && deassign_device(d, seg, bus, devfn) )
> >> +    {
> >> +        printk(XENLOG_ERR "%pd: %pp was left partially assigned\n",
> >> +               d, &PCI_SBDF(seg, bus, devfn));
> >
> > &pdev->sbdf?  Then you can get of the devfn usage above.
> 
> Yes, thanks.
> 
> >> +        domain_crash(d);
> >
> > This seems like a bit different from the other error paths in the
> > function, isn't it fine to return an error and let the caller handle
> > the deassign?
> 
> I believe, intention was to not leave device in an unknown state: we
> failed both assign_device() and deassign_device() call, so what to do
> now? But yes, I think you are right and it is better to let caller to
> decide what to do next.

I don't think it would be a security risk to leave the device in that
state.  For domUs the guest won't get access to the device registers
anyway as we use an allow list approach.  Also deassign_device() is
not called when we fail to assign one of the phantom functions.

We don't seem to undo any of the work in assign_device() on error so
it should be fine to not do the call to deassign_device() on error to
initialize vPCI.

> >
> > Also, if we really need to call deassign_device() we must do so for
> > all possible phantom devices, see the above loop around
> > iommu_call(..., assing_device, ...);
> 
> But deassign_device() has the loop for all phantom devices that already
> does all the work. Unless I miss something, of course.

No, you are right, a single call is fine.

Thanks, Roger.