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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2023 17:33, Roger Pau Monné wrote:
> 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.

Perhaps, but again in a separate patch.

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

Hmm, yes, that's a valid way of looking at (and justifying) it.

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

Okay, I'll leave that alone than, at least as long as we don't see any
reason because of actual behavioral differences.

Jan