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

[Huang Rui <ray.huang@amd.com>]

On Wed, Mar 22, 2023 at 05:34:41PM +0800, Roger Pau Monné wrote:
> On Wed, Mar 22, 2023 at 03:28:58PM +0800, Huang Rui wrote:
> > On Tue, Mar 21, 2023 at 09:03:58PM +0800, Huang Rui wrote:
> > > On Tue, Mar 21, 2023 at 08:27:21PM +0800, Jan Beulich wrote:
> > > > On 21.03.2023 12:49, Huang Rui wrote:
> > > > > Thanks, but we found if dom0 is PV domain, the passthrough device will
> > > > > access this function to write the real bar.
> > > > 
> > > > Can you please be quite a bit more detailed about this? The specific code
> > > > paths taken (in upstream software) to result in such would of of interest.
> > > > 
> > > 
> > > yes, please wait for a moment. let me capture a trace dump in my side.
> > > 
> > 
> > Sorry, we are wrong that if Xen PV dom0, bar_write() won't be called,
> > please ignore above information.
> > 
> > While xen is on initialization on PVH dom0, it will add all PCI devices in
> > the real bus including 0000:03:00.0 (VGA device: GPU) and 0000:03:00.1
> > (Audio device).
> > 
> > Audio is another function in the pcie device, but we won't use it here. So
> > we will remove it after that.
> > 
> > Please see below xl dmesg:
> > 
> > (XEN) PCI add device 0000:03:00.0
> > (XEN) d0v0 bar_write Ray line 391 0000:03:00.1 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:03:00.1 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:03:00.1 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:03:00.1 bar->enabled 0
> > (XEN) PCI add device 0000:03:00.1
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 391 0000:04:00.0 bar->enabled 0
> > (XEN) d0v0 bar_write Ray line 406 0000:04:00.0 bar->enabled 0
> > (XEN) PCI add device 0000:04:00.0
> > 
> > ...
> > 
> > (XEN) PCI add device 0000:07:00.7
> > (XEN) arch/x86/hvm/svm/svm.c:2017:d0v0 RDMSR 0xc0010058 unimplemented
> > (XEN) arch/x86/hvm/svm/svm.c:2017:d0v0 RDMSR 0xc0011020 unimplemented
> > (XEN) PCI remove device 0000:03:00.1
> > 
> > We run below script to remove audio
> > 
> > echo -n "1" > /sys/bus/pci/devices/0000:03:00.1/remove
> > 
> > (XEN) arch/x86/hvm/svm/svm.c:2017:d0v0 RDMSR 0xc001029b unimplemented
> > (XEN) arch/x86/hvm/svm/svm.c:2017:d0v0 RDMSR 0xc001029a unimplemented
> > 
> > Then we will run "xl pci-assignable-add 03:00.0" to assign GPU as
> > passthrough. At this moment, the real bar is trying to be written.
> > 
> > (XEN) d0v7 bar_write Ray line 391 0000:03:00.0 bar->enabled 1
> > (XEN) d0v7 bar_write Ray line 406 0000:03:00.0 bar->enabled 1
> > (XEN) Xen WARN at drivers/vpci/header.c:408
> > (XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Not tainted ]----
> > (XEN) CPU:    8
> > (XEN) RIP:    e008:[<ffff82d040263cb9>] drivers/vpci/header.c#bar_write+0xc0/0x1ce
> > (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v7)
> > (XEN) rax: ffff8303fc36d06c   rbx: ffff8303f90468b0   rcx: 0000000000000010
> > (XEN) rdx: 0000000000000002   rsi: ffff8303fc36a020   rdi: ffff8303fc36a018
> > (XEN) rbp: ffff8303fc367c18   rsp: ffff8303fc367be8   r8:  0000000000000001
> > (XEN) r9:  ffff8303fc36a010   r10: 0000000000000001   r11: 0000000000000001
> > (XEN) r12: 00000000d0700000   r13: ffff8303fc6d9230   r14: ffff8303fc6d9270
> > (XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 00000000003506e0
> > (XEN) cr3: 00000003fc3c4000   cr2: 00007f180f6371e8
> > (XEN) fsb: 00007fce655edbc0   gsb: ffff88822f3c0000   gss: 0000000000000000
> > (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
> > (XEN) Xen code around <ffff82d040263cb9> (drivers/vpci/header.c#bar_write+0xc0/0x1ce):
> > (XEN)  b6 53 14 f6 c2 02 74 02 <0f> 0b 48 8b 03 45 84 ff 0f 85 ec 00 00 00 48 b9
> > (XEN) Xen stack trace from rsp=ffff8303fc367be8:
> > (XEN)    00000024fc367bf8 ffff8303f9046a50 0000000000000000 0000000000000004
> > (XEN)    0000000000000004 0000000000000024 ffff8303fc367ca0 ffff82d040263683
> > (XEN)    00000300fc367ca0 d070000003003501 00000024d0700000 ffff8303fc6d9230
> > (XEN)    0000000000000000 0000000000000000 0000002400000004 0000000000000000
> > (XEN)    0000000000000000 0000000000000000 0000000000000004 00000000d0700000
> > (XEN)    0000000000000024 0000000000000000 ffff82d040404bc0 ffff8303fc367cd0
> > (XEN)    ffff82d0402c60a8 0000030000000001 ffff8303fc367d88 0000000000000000
> > (XEN)    ffff8303fc610800 ffff8303fc367d30 ffff82d0402c54da ffff8303fc367ce0
> > (XEN)    ffff8303fc367fff 0000000000000004 ffff830300000004 00000000d0700000
> > (XEN)    ffff8303fc610800 ffff8303fc367d88 0000000000000001 0000000000000000
> > (XEN)    0000000000000000 ffff8303fc367d58 ffff82d0402c5570 0000000000000004
> > (XEN)    ffff8304065ea000 ffff8303fc367e28 ffff8303fc367dd0 ffff82d0402b5357
> > (XEN)    0000000000000cfc ffff8303fc621000 0000000000000000 0000000000000000
> > (XEN)    0000000000000cfc 00000000d0700000 0000000400000001 0001000000000000
> > (XEN)    0000000000000004 0000000000000004 0000000000000000 ffff8303fc367e44
> > (XEN)    ffff8304065ea000 ffff8303fc367e10 ffff82d0402b56d6 0000000000000000
> > (XEN)    ffff8303fc367e44 0000000000000004 0000000000000cfc ffff8304065e6000
> > (XEN)    0000000000000000 ffff8303fc367e30 ffff82d0402b6bcc ffff8303fc367e44
> > (XEN)    0000000000000001 ffff8303fc367e70 ffff82d0402c5e80 d070000040203490
> > (XEN)    000000000000007b ffff8303fc367ef8 ffff8304065e6000 ffff8304065ea000
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d040263cb9>] R drivers/vpci/header.c#bar_write+0xc0/0x1ce
> > (XEN)    [<ffff82d040263683>] F vpci_write+0x123/0x26c
> > (XEN)    [<ffff82d0402c60a8>] F arch/x86/hvm/io.c#vpci_portio_write+0xa0/0xa7
> > (XEN)    [<ffff82d0402c54da>] F hvm_process_io_intercept+0x203/0x26f
> > (XEN)    [<ffff82d0402c5570>] F hvm_io_intercept+0x2a/0x4c
> > (XEN)    [<ffff82d0402b5357>] F arch/x86/hvm/emulate.c#hvmemul_do_io+0x29b/0x5eb
> > (XEN)    [<ffff82d0402b56d6>] F arch/x86/hvm/emulate.c#hvmemul_do_io_buffer+0x2f/0x6a
> > (XEN)    [<ffff82d0402b6bcc>] F hvmemul_do_pio_buffer+0x33/0x35
> > (XEN)    [<ffff82d0402c5e80>] F handle_pio+0x70/0x1b7
> > (XEN)    [<ffff82d04029dc7f>] F svm_vmexit_handler+0x10ba/0x18aa
> > (XEN)    [<ffff82d0402034e5>] F svm_stgi_label+0x8/0x18
> > (XEN)
> > (XEN) d0v7 bar_write Ray line 391 0000:03:00.0 bar->enabled 1
> > (XEN) d0v7 bar_write Ray line 406 0000:03:00.0 bar->enabled 1
> 

Hi Jan, Roger,

> As said by Jan, it's hard to figure out where are the printks placed without a
> diff of your changes.

I attached the diff of my prints below, and I want to figure out why the
Bar_write() is called while we use pci-assignable-add to assign passthrough device in PVH dom0.


diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 918d11fbce..35447aff2a 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -388,12 +388,14 @@ static void cf_check bar_write(
     else
         val &= PCI_BASE_ADDRESS_MEM_MASK;
 
+    gprintk(XENLOG_WARNING, "%s Ray line %d %pp bar->enabled %d\n", __func__, __LINE__, &pdev->sbdf , bar->enabled);
     /*
      * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
      * writes as long as the BAR is not mapped into the p2m.
      */
     if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
     {
+        gprintk(XENLOG_WARNING, "%s Ray line %d %pp bar->enabled %d\n", __func__, __LINE__, &pdev->sbdf , bar->enabled);
         /* If the value written is the current one avoid printing a warning. */
         if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
             gprintk(XENLOG_WARNING,
@@ -401,7 +403,9 @@ static void cf_check bar_write(
                     &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
         return;
     }
-
+    gprintk(XENLOG_WARNING, "%s Ray line %d %pp bar->enabled %d\n", __func__, __LINE__, &pdev->sbdf , bar->enabled);
+    if (bar->enabled)
+	WARN_ON(1);
 
     /*
      * Update the cached address, so that when memory decoding is enabled

> 
> So far the above seems to be expected, as we currently don't handle BAR
> register writes with memory decoding enabled.
> 
> Given the change proposed in this patch, can you check whether `bar->enabled ==
> true` but the PCI command register has the memory decoding bit unset?


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?

[  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


> 
> If so it would mean Xen state got out-of-sync with the hardware state, and we
> would need to figure out where it happened.  Is there any backdoor in the AMD
> GPU that allows to disable memory decoding without using the PCI command
> register?
> 

I don't think we have any backdoor.

Thanks,
Ray