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 <jbeul...@suse.com>
>> ---
>> 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 ...
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.
>> 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?
>> RFC: _setup_hwdom_pci_devices()' loop may want splitting: For
>> modify_bars() to consistently respect BARs of hidden devices while
>> setting up "normal" ones (i.e. to avoid as much as possible the
>> "continue" path introduced here), setting up of the former may want
>> doing first.
>
> But BARs of hidden devices should be mapped into dom0 physmap?
Yes.
> 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.
>> RFC: vpci_write()'s check of the r/o map may want moving out of mainline
>> code, into the case dealing with !pdev->vpci.
>
> Indeed.
Will extend the patch accordingly then
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
>> struct vpci_header *header = &pdev->vpci->header;
>> struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>> struct pci_dev *tmp, *dev = NULL;
>> + const struct domain *d;
>> const struct vpci_msix *msix = pdev->vpci->msix;
>> unsigned int i;
>> int rc;
>> @@ -285,9 +286,11 @@ static int modify_bars(const struct pci_
>>
>> /*
>> * Check for overlaps with other BARs. Note that only BARs that are
>> - * currently mapped (enabled) are checked for overlaps.
>> + * currently mapped (enabled) are checked for overlaps. Note also that
>> + * for Dom0 we also need to include hidden, i.e. DomXEN's, devices.
>> */
>> - for_each_pdev ( pdev->domain, tmp )
>> +for ( d = pdev->domain; ; d = dom_xen ) {//todo
>> + for_each_pdev ( d, tmp )
>> {
>> if ( tmp == pdev )
>> {
>> @@ -304,6 +307,7 @@ static int modify_bars(const struct pci_
>> */
>> continue;
>> }
>> +if ( !tmp->vpci ) { ASSERT(d == dom_xen); continue; }//todo
>>
>> for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>> {
>> @@ -330,6 +334,7 @@ static int modify_bars(const struct pci_
>> }
>> }
>> }
>> +if ( !is_hardware_domain(d) ) break; }//todo
>
> AFAICT in order to support hidden devices there's one bit missing in
> vpci_{write,read}(): the call to pci_get_pdev() should be also done
> against dom_xen when handling accesses originated from the hardware
> domain.
Hmm, right. Without that we'd always take the vpci_{read,write}_hw()
path.
> 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)
Jan
