On Fri, Mar 24, 2023 at 12:23:39AM +0800, Roger Pau Monné wrote: > 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)?
Yes, dom0 works well. But they don't work on passthrough devices in domU whatever with PV or PVH. So I would like to implement the gsi firstly, then continue checking the MSI(-X) issues. > > > 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. > Thanks, Ray
