Re: [PATCH V7 09/11] vpci: add initial support for virtual PCI bus topology

[Oleksandr <olekstysh@xxxxxxxxx>]

On 28.07.22 17:26, Jan Beulich wrote:

Hello Jan

> On 28.07.2022 16:16, Oleksandr wrote:
> > On 27.07.22 13:32, Jan Beulich wrote:
> > > On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > >
> > > > Assign SBDF to the PCI devices being passed through with bus 0.
> > > > The resulting topology is where PCIe devices reside on the bus 0 of the
> > > > root complex itself (embedded endpoints).
> > > > This implementation is limited to 32 devices which are allowed on
> > > > a single PCI bus.
> > > >
> > > > Please note, that at the moment only function 0 of a multifunction
> > > > device can be passed through.
> > > I've not been able to spot where this restriction is being enforced -
> > > can you please point me at the respective code?
> > Nor have I found the respective code.
> >
> > Could you please suggest a place where to put such enforcement (I guess,
> > this should be present in the toolstack)?
> Such check should be in the tool stack primarily to give a sensible
> error message to the user. Yet the hypervisor needs to check itself
> nevertheless. You know the code you're adding much better than I do,
> so I guess I'm a little puzzled by you asking me to suggest a place.
> (And for the tool stack I guess asking tool stack folks would get
> you better mileage.)
Thanks for the clarification.


I am still getting used to the changes which that patch series makes (I 
didn't write that code).
Asking for suggestion I didn't mean to point an exact place in the code, 
but rather a subsystem/software layer,
sorry if I was unclear.


> > > > @@ -124,6 +191,7 @@ void vpci_deassign_device(struct pci_dev *pdev)
> > > >        if ( !has_vpci(pdev->domain) )
> > > >            return;
> > > >    
> > > > +    vpci_remove_virtual_device(pdev);
> > > >        vpci_remove_device(pdev);
> > > >    }
> > > And other call sites of vpci_remove_device() do not have a need of
> > > cleaning up guest_sbdf / vpci_dev_assigned_map?
> > I am not 100% sure, but it looks like they don't need. On the other
> > hand, even if they don't need that, doing the cleaning won't be an issue
> > at all,
> >
> > there is a check before cleaning (which will be extended as I proposed
> > above), so ...
> >
> >
> > > IOW I wonder if it
> > > wouldn't be better to have vpci_remove_device() do this as well
> > > (retaining - see my comment on the earlier patch) the simple aliasing
> > > of vpci_deassign_device() to vpci_remove_device()).
> >      ... maybe yes. Shall I do that change?
> Well - yes please, afaic.

ok, will do


> Jan
--
Regards,

Oleksandr Tyshchenko