Re: [RFC XEN PATCH 2/6] vpci: accept BAR writes if dom0 is PVH

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

On Thu, Mar 23, 2023 at 09:34:40PM +0800, Huang Rui wrote:
> On Thu, Mar 23, 2023 at 06:43:53PM +0800, Roger Pau Monné wrote:
> > On Wed, Mar 22, 2023 at 01:48:30PM +0100, Jan Beulich wrote:
> > > On 22.03.2023 13:33, Huang Rui wrote:
> > > > I traced that while we do pci-assignable-add, we will follow below 
> > > > trace to
> > > > bind the passthrough device.
> > > > 
> > > > pciassignable_add()->libxl_device_pci_assignable_add()->libxl__device_pci_assignable_add()->pciback_dev_assign()
> > > > 
> > > > Then kernel xen-pciback driver want to add virtual configuration 
> > > > spaces. In
> > > > this phase, the bar_write() in xen hypervisor will be called. I still 
> > > > need
> > > > a bit more time to figure the exact reason. May I know where the
> > > > xen-pciback driver would trigger a hvm_io_intercept to xen hypervisor?
> > > 
> > > Any config space access would. And I might guess ...
> > > 
> > > > [  309.719049] xen_pciback: wants to seize 0000:03:00.0
> > > > [  462.911251] pciback 0000:03:00.0: xen_pciback: probing...
> > > > [  462.911256] pciback 0000:03:00.0: xen_pciback: seizing device
> > > > [  462.911257] pciback 0000:03:00.0: xen_pciback: pcistub_device_alloc
> > > > [  462.911261] pciback 0000:03:00.0: xen_pciback: initializing...
> > > > [  462.911263] pciback 0000:03:00.0: xen_pciback: initializing config
> > > > [  462.911265] pciback 0000:03:00.0: xen-pciback: initializing virtual 
> > > > configuration space
> > > > [  462.911268] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x00
> > > > [  462.911271] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x02
> > > > [  462.911284] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x04
> > > > [  462.911286] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x3c
> > > > [  462.911289] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x3d
> > > > [  462.911291] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x0c
> > > > [  462.911294] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x0d
> > > > [  462.911296] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x0f
> > > > [  462.911301] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x10
> > > > [  462.911306] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x14
> > > > [  462.911309] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x18
> > > > [  462.911313] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x1c
> > > > [  462.911317] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x20
> > > > [  462.911321] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x24
> > > > [  462.911325] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x30
> > > > [  462.911358] pciback 0000:03:00.0: Found capability 0x1 at 0x50
> > > > [  462.911361] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x50
> > > > [  462.911363] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x52
> > > > [  462.911368] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x54
> > > > [  462.911371] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x56
> > > > [  462.911373] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0x57
> > > > [  462.911386] pciback 0000:03:00.0: Found capability 0x5 at 0xa0
> > > > [  462.911388] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0xa0
> > > > [  462.911391] pciback 0000:03:00.0: xen-pciback: added config field at 
> > > > offset 0xa2
> > > > [  462.911405] pciback 0000:03:00.0: xen_pciback: enabling device
> > > > [  462.911412] pciback 0000:03:00.0: enabling device (0006 -> 0007)
> > > > [  462.911658] Already setup the GSI :28
> > > > [  462.911668] Already map the GSI :28 and IRQ: 115
> > > > [  462.911684] pciback 0000:03:00.0: xen_pciback: save state of device
> > > > [  462.912154] pciback 0000:03:00.0: xen_pciback: resetting (FLR, D3, 
> > > > etc) the device
> > > > [  463.954998] pciback 0000:03:00.0: xen_pciback: reset device
> > > 
> > > ... it is actually the reset here, saving and then restoring config space.
> > > If e.g. that restore was done "blindly" (i.e. simply writing fields low to
> > > high), then memory decode would be re-enabled before the BARs are written.
> > 
> > The problem is also that we don't tell vPCI that the device has been
> > reset, so the current cached state in pdev->vpci is all out of date
> > with the real device state.
> > 
> > I didn't hit this on my test because the device I was using had no
> > reset support.
> > 
> > I don't think it's feasible for Xen to detect all the possible reset
> > methods dom0 might use, as some of those are device specific for
> > example.
> 
> OK.
> 
> > 
> > We would have to introduce a new hypercall that clears all vPCI device
> > state, PHYSDEVOP_pci_device_reset for example.  This will involve
> > adding proper cleanup functions, as the current code in
> > vpci_remove_device() only deals with allocated memory (because so far
> > devices where not deassigned) but we now also need to make sure
> > MSI(-X) interrupts are torn down and freed, and will also require
> > removing any mappings of BARs into the dom0 physmap.
> > 
> 
> Thanks for the suggestion. Let me make the new PHYSDEVOP_pci_device_reset
> in the next version instead of current workaround.
> 
> The MSI(-X) interrupts doesn't work in our platform, I don't figure the
> root cause yet.

Do MSI-X interrupts work when the device is in use by dom0 (both Pv
and PVH)?

> Could you please elaborate where we should require removing
> any mappings of BARs into the dom0 physmap here?

I think you can just use `modify_bars(pdev, 0, 0)`, as that will
effectively remove any BARs from the memory map.  That should also
take care of preemption, so you should be good to go.

Regards, Roger.