Re: [PATCH RFC v2] vPCI: account for hidden devices

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

On Wed, May 24, 2023 at 04:44:49PM +0200, Jan Beulich wrote:
> On 24.05.2023 16:22, Roger Pau Monné wrote:
> > On Wed, May 24, 2023 at 03:45:58PM +0200, Jan Beulich wrote:
> >> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> >> console) are associated with DomXEN, not Dom0. This means that while
> >> looking for overlapping BARs such devices cannot be found on Dom0's list
> >> of devices; DomXEN's list also needs to be scanned.
> >>
> >> Suppress vPCI init altogether for r/o devices (which constitute a subset
> >> of hidden ones).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> RFC: The modify_bars() change is intentionally mis-formatted, as the
> >>      necessary re-indentation would make the diff difficult to read. At
> >>      this point I'd merely like to gather input towards possible better
> >>      approaches to solve the issue (not the least because quite possibly
> >>      there are further places needing changing).
> > 
> > I think we should also handle the case of !pdev->vpci for the hardware
> > doamin, as it's allowed for the vpci_add_handlers() call in
> > setup_one_hwdom_device() to fail and the device wold still be assigned
> > to the hardware domain.
> > 
> > I can submit that as a separate bugfix, as it's already an issue
> > without taking r/o or hidden devices into account.
> 
> Yeah, I think that wants dealing with separately. I'm not actually sure
> though that "is allowed to fail" is proper behavior ...

One better option would be to mark the device r/o if the
vpci_add_handlers() call fails in setup_one_hwdom_device(), as that
would prevent dom0 from accessing native MSI(-X) capabilities.

> But anyway - I take this as you agreeing to go that route, which is the
> prereq for me to actually make a well-formed patch. Please shout soon
> if that's a misunderstanding of mine.

Sure, will send the fix later today or tomorrow so that you can
rebase.

> >> RFC: Whether to actually suppress vPCI init is up for debate; adding the
> >>      extra logic is following Roger's suggestion (I'm not convinced it is
> >>      useful to have). If we want to keep that, a 2nd question would be
> >>      whether to keep it in vpci_add_handlers(): Both of its callers (can)
> >>      have a struct pci_seg readily available, so checking ->ro_map at the
> >>      call sites would be easier.
> > 
> > But that would duplicate the logic into the callers, which doesn't
> > seem very nice to me, and makes it easier to make mistakes if further
> > callers are added and r/o is not checked there.
> 
> Right, hence why I didn't do it the alternative way from the beginning.
> Both approaches have a pro and a con.
> 
> But prior to answering the 2nd question, what about the 1st one? Is it
> really worth to have the extra logic?

Why would we want to do all the vPCI initialization for r/o devices?
None of the handlers setup will get called, so I see it the other way
around: not short-circuiting vpci_add_handlers() for r/o devices is a
waste of time and resources because none of the setup state would be
used anyway.

> >  And
> > hence doing those before or after normal devices will lead to the same
> > result.  The loop in modify_bars() is there to avoid attempting to map
> > the same range twice, or to unmap a range while there are devices
> > still using it, but the unmap is never done during initial device
> > setup.
> 
> Okay, so maybe indeed there's no effect on the final result. Yet still
> the anomaly bothered me when seeing it in the logs (it actually mislead
> me initially in my conclusions as to what was actually going on), when
> I added a printk() to that new "continue" path. We would skip hidden
> devices up until them getting initialized themselves. There would be
> less skipping if all (there aren't going to be many) DomXEN devices
> were initialized first.

I think that just makes the logic more complicated for no reason, the
only reason you don't see this with devices assigned to dom0 is
because device addition is interleaved with calls to
vpci_add_handlers().  However it would also be valid to add all
devices to dom0 and then call vpci_add_handlers() for each one of them.

> > One further question is whether we want to map BARs of r/o devices
> > into the hardware domain physmap.  Not sure that's very helpful, as
> > dom0 won't be allowed to modify any of the config space bits of those
> > devices, so even attempts to size the BARs will fail.  I wonder what
> > kind of issues this can cause to dom0 OSes.
> 
> This is what Linux (6.3) says:
> 
> pci 0000:02:00.1: [Firmware Bug]: reg 0x10: invalid BAR (can't size)
> pci 0000:02:00.1: [Firmware Bug]: reg 0x14: invalid BAR (can't size)
> pci 0000:02:00.1: [Firmware Bug]: reg 0x24: invalid BAR (can't size)

OK, seems fine then.  There's no point in mapping the BARs of r/o
devices to the dom0 physmap, as the domain is unable to size them in
the first place.

Thanks, Roger.