[PATCH 0/2] Fix Linux dom0 boot on a QEMU/KVM virtual machine

[PATCH 0/2] Fix Linux dom0 boot on a QEMU/KVM virtual machine

[Petr Pavlu]

Fix two problems that prevent booting Linux dom0 on a QEMU/KVM virtual
machine, which is sometimes useful for testing purposes.

Petr Pavlu (2):
  xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent
  xen/virtio: Avoid use of the dom0 backend in dom0

 drivers/xen/grant-dma-ops.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.35.3

[PATCH 1/2] xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent

[Petr Pavlu]

When attempting to run Xen on a QEMU/KVM virtual machine with virtio
devices (all x86_64), function xen_dt_get_node() crashes on accessing
bus->bridge->parent->of_node because a bridge of the PCI root bus has no
parent set:

[    1.694192][    T1] BUG: kernel NULL pointer dereference, address: 0000000000000288
[    1.695688][    T1] #PF: supervisor read access in kernel mode
[    1.696297][    T1] #PF: error_code(0x0000) - not-present page
[    1.696297][    T1] PGD 0 P4D 0
[    1.696297][    T1] Oops: 0000 [#1] PREEMPT SMP NOPTI
[    1.696297][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.7-1-default #1 openSUSE Tumbleweed a577eae57964bb7e83477b5a5645a1781df990f0
[    1.696297][    T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
[    1.696297][    T1] RIP: e030:xen_virtio_restricted_mem_acc+0xd9/0x1c0
[    1.696297][    T1] Code: 45 0c 83 e8 c9 a3 ea ff 31 c0 eb d7 48 8b 87 40 ff ff ff 48 89 c2 48 8b 40 10 48 85 c0 75 f4 48 8b 82 10 01 00 00 48 8b 40 40 <48> 83 b8 88 02 00 00 00 0f 84 45 ff ff ff 66 90 31 c0 eb a5 48 89
[    1.696297][    T1] RSP: e02b:ffffc90040013cc8 EFLAGS: 00010246
[    1.696297][    T1] RAX: 0000000000000000 RBX: ffff888006c75000 RCX: 0000000000000029
[    1.696297][    T1] RDX: ffff888005ed1000 RSI: ffffc900400f100c RDI: ffff888005ee30d0
[    1.696297][    T1] RBP: ffff888006c75010 R08: 0000000000000001 R09: 0000000330000006
[    1.696297][    T1] R10: ffff888005850028 R11: 0000000000000002 R12: ffffffff830439a0
[    1.696297][    T1] R13: 0000000000000000 R14: ffff888005657900 R15: ffff888006e3e1e8
[    1.696297][    T1] FS:  0000000000000000(0000) GS:ffff88804a000000(0000) knlGS:0000000000000000
[    1.696297][    T1] CS:  e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.696297][    T1] CR2: 0000000000000288 CR3: 0000000002e36000 CR4: 0000000000050660
[    1.696297][    T1] Call Trace:
[    1.696297][    T1]  <TASK>
[    1.696297][    T1]  virtio_features_ok+0x1b/0xd0
[    1.696297][    T1]  virtio_dev_probe+0x19c/0x270
[    1.696297][    T1]  really_probe+0x19b/0x3e0
[    1.696297][    T1]  __driver_probe_device+0x78/0x160
[    1.696297][    T1]  driver_probe_device+0x1f/0x90
[    1.696297][    T1]  __driver_attach+0xd2/0x1c0
[    1.696297][    T1]  bus_for_each_dev+0x74/0xc0
[    1.696297][    T1]  bus_add_driver+0x116/0x220
[    1.696297][    T1]  driver_register+0x59/0x100
[    1.696297][    T1]  virtio_console_init+0x7f/0x110
[    1.696297][    T1]  do_one_initcall+0x47/0x220
[    1.696297][    T1]  kernel_init_freeable+0x328/0x480
[    1.696297][    T1]  kernel_init+0x1a/0x1c0
[    1.696297][    T1]  ret_from_fork+0x29/0x50
[    1.696297][    T1]  </TASK>
[    1.696297][    T1] Modules linked in:
[    1.696297][    T1] CR2: 0000000000000288
[    1.696297][    T1] ---[ end trace 0000000000000000 ]---

The PCI root bus is in this case created from ACPI description via
acpi_pci_root_add() -> pci_acpi_scan_root() -> acpi_pci_root_create() ->
pci_create_root_bus() where the last function is called with
parent=NULL. It indicates that no parent is present and then
bus->bridge->parent is NULL too.

Fix the problem by checking bus->bridge->parent in xen_dt_get_node() for
NULL first.

Fixes: ef8ae384b4c9 ("xen/virtio: Handle PCI devices which Host controller is described in DT")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 drivers/xen/grant-dma-ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 9784a77fa3c9..76f6f26265a3 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -303,6 +303,8 @@ static struct device_node *xen_dt_get_node(struct device *dev)
 		while (!pci_is_root_bus(bus))
 			bus = bus->parent;
 
+		if (!bus->bridge->parent)
+			return NULL;
 		return of_node_get(bus->bridge->parent->of_node);
 	}
 
-- 
2.35.3

Re: [PATCH 1/2] xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent

[Oleksandr Tyshchenko]

On 21.06.23 16:12, Petr Pavlu wrote:


Hello Petr


> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> devices (all x86_64), function xen_dt_get_node() crashes on accessing
> bus->bridge->parent->of_node because a bridge of the PCI root bus has no
> parent set:
> 
> [    1.694192][    T1] BUG: kernel NULL pointer dereference, address: 0000000000000288
> [    1.695688][    T1] #PF: supervisor read access in kernel mode
> [    1.696297][    T1] #PF: error_code(0x0000) - not-present page
> [    1.696297][    T1] PGD 0 P4D 0
> [    1.696297][    T1] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [    1.696297][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.7-1-default #1 openSUSE Tumbleweed a577eae57964bb7e83477b5a5645a1781df990f0
> [    1.696297][    T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
> [    1.696297][    T1] RIP: e030:xen_virtio_restricted_mem_acc+0xd9/0x1c0
> [    1.696297][    T1] Code: 45 0c 83 e8 c9 a3 ea ff 31 c0 eb d7 48 8b 87 40 ff ff ff 48 89 c2 48 8b 40 10 48 85 c0 75 f4 48 8b 82 10 01 00 00 48 8b 40 40 <48> 83 b8 88 02 00 00 00 0f 84 45 ff ff ff 66 90 31 c0 eb a5 48 89
> [    1.696297][    T1] RSP: e02b:ffffc90040013cc8 EFLAGS: 00010246
> [    1.696297][    T1] RAX: 0000000000000000 RBX: ffff888006c75000 RCX: 0000000000000029
> [    1.696297][    T1] RDX: ffff888005ed1000 RSI: ffffc900400f100c RDI: ffff888005ee30d0
> [    1.696297][    T1] RBP: ffff888006c75010 R08: 0000000000000001 R09: 0000000330000006
> [    1.696297][    T1] R10: ffff888005850028 R11: 0000000000000002 R12: ffffffff830439a0
> [    1.696297][    T1] R13: 0000000000000000 R14: ffff888005657900 R15: ffff888006e3e1e8
> [    1.696297][    T1] FS:  0000000000000000(0000) GS:ffff88804a000000(0000) knlGS:0000000000000000
> [    1.696297][    T1] CS:  e030 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.696297][    T1] CR2: 0000000000000288 CR3: 0000000002e36000 CR4: 0000000000050660
> [    1.696297][    T1] Call Trace:
> [    1.696297][    T1]  <TASK>
> [    1.696297][    T1]  virtio_features_ok+0x1b/0xd0
> [    1.696297][    T1]  virtio_dev_probe+0x19c/0x270
> [    1.696297][    T1]  really_probe+0x19b/0x3e0
> [    1.696297][    T1]  __driver_probe_device+0x78/0x160
> [    1.696297][    T1]  driver_probe_device+0x1f/0x90
> [    1.696297][    T1]  __driver_attach+0xd2/0x1c0
> [    1.696297][    T1]  bus_for_each_dev+0x74/0xc0
> [    1.696297][    T1]  bus_add_driver+0x116/0x220
> [    1.696297][    T1]  driver_register+0x59/0x100
> [    1.696297][    T1]  virtio_console_init+0x7f/0x110
> [    1.696297][    T1]  do_one_initcall+0x47/0x220
> [    1.696297][    T1]  kernel_init_freeable+0x328/0x480
> [    1.696297][    T1]  kernel_init+0x1a/0x1c0
> [    1.696297][    T1]  ret_from_fork+0x29/0x50
> [    1.696297][    T1]  </TASK>
> [    1.696297][    T1] Modules linked in:
> [    1.696297][    T1] CR2: 0000000000000288
> [    1.696297][    T1] ---[ end trace 0000000000000000 ]---
> 
> The PCI root bus is in this case created from ACPI description via
> acpi_pci_root_add() -> pci_acpi_scan_root() -> acpi_pci_root_create() ->
> pci_create_root_bus() where the last function is called with
> parent=NULL. It indicates that no parent is present and then
> bus->bridge->parent is NULL too.
> 
> Fix the problem by checking bus->bridge->parent in xen_dt_get_node() for
> NULL first >
> Fixes: ef8ae384b4c9 ("xen/virtio: Handle PCI devices which Host controller is described in DT")

Oops, sorry. I have to admit I checked with DT only.


> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>



> ---
>   drivers/xen/grant-dma-ops.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 9784a77fa3c9..76f6f26265a3 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -303,6 +303,8 @@ static struct device_node *xen_dt_get_node(struct device *dev)
>   		while (!pci_is_root_bus(bus))
>   			bus = bus->parent;
>   
> +		if (!bus->bridge->parent)
> +			return NULL;
>   		return of_node_get(bus->bridge->parent->of_node);
>   	}
>   

Re: [PATCH 1/2] xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent

[Stefano Stabellini]

On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
> On 21.06.23 16:12, Petr Pavlu wrote:
> Hello Petr
> 
> 
> > When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> > devices (all x86_64), function xen_dt_get_node() crashes on accessing
> > bus->bridge->parent->of_node because a bridge of the PCI root bus has no
> > parent set:
> > 
> > [    1.694192][    T1] BUG: kernel NULL pointer dereference, address: 0000000000000288
> > [    1.695688][    T1] #PF: supervisor read access in kernel mode
> > [    1.696297][    T1] #PF: error_code(0x0000) - not-present page
> > [    1.696297][    T1] PGD 0 P4D 0
> > [    1.696297][    T1] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [    1.696297][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.7-1-default #1 openSUSE Tumbleweed a577eae57964bb7e83477b5a5645a1781df990f0
> > [    1.696297][    T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
> > [    1.696297][    T1] RIP: e030:xen_virtio_restricted_mem_acc+0xd9/0x1c0
> > [    1.696297][    T1] Code: 45 0c 83 e8 c9 a3 ea ff 31 c0 eb d7 48 8b 87 40 ff ff ff 48 89 c2 48 8b 40 10 48 85 c0 75 f4 48 8b 82 10 01 00 00 48 8b 40 40 <48> 83 b8 88 02 00 00 00 0f 84 45 ff ff ff 66 90 31 c0 eb a5 48 89
> > [    1.696297][    T1] RSP: e02b:ffffc90040013cc8 EFLAGS: 00010246
> > [    1.696297][    T1] RAX: 0000000000000000 RBX: ffff888006c75000 RCX: 0000000000000029
> > [    1.696297][    T1] RDX: ffff888005ed1000 RSI: ffffc900400f100c RDI: ffff888005ee30d0
> > [    1.696297][    T1] RBP: ffff888006c75010 R08: 0000000000000001 R09: 0000000330000006
> > [    1.696297][    T1] R10: ffff888005850028 R11: 0000000000000002 R12: ffffffff830439a0
> > [    1.696297][    T1] R13: 0000000000000000 R14: ffff888005657900 R15: ffff888006e3e1e8
> > [    1.696297][    T1] FS:  0000000000000000(0000) GS:ffff88804a000000(0000) knlGS:0000000000000000
> > [    1.696297][    T1] CS:  e030 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    1.696297][    T1] CR2: 0000000000000288 CR3: 0000000002e36000 CR4: 0000000000050660
> > [    1.696297][    T1] Call Trace:
> > [    1.696297][    T1]  <TASK>
> > [    1.696297][    T1]  virtio_features_ok+0x1b/0xd0
> > [    1.696297][    T1]  virtio_dev_probe+0x19c/0x270
> > [    1.696297][    T1]  really_probe+0x19b/0x3e0
> > [    1.696297][    T1]  __driver_probe_device+0x78/0x160
> > [    1.696297][    T1]  driver_probe_device+0x1f/0x90
> > [    1.696297][    T1]  __driver_attach+0xd2/0x1c0
> > [    1.696297][    T1]  bus_for_each_dev+0x74/0xc0
> > [    1.696297][    T1]  bus_add_driver+0x116/0x220
> > [    1.696297][    T1]  driver_register+0x59/0x100
> > [    1.696297][    T1]  virtio_console_init+0x7f/0x110
> > [    1.696297][    T1]  do_one_initcall+0x47/0x220
> > [    1.696297][    T1]  kernel_init_freeable+0x328/0x480
> > [    1.696297][    T1]  kernel_init+0x1a/0x1c0
> > [    1.696297][    T1]  ret_from_fork+0x29/0x50
> > [    1.696297][    T1]  </TASK>
> > [    1.696297][    T1] Modules linked in:
> > [    1.696297][    T1] CR2: 0000000000000288
> > [    1.696297][    T1] ---[ end trace 0000000000000000 ]---
> > 
> > The PCI root bus is in this case created from ACPI description via
> > acpi_pci_root_add() -> pci_acpi_scan_root() -> acpi_pci_root_create() ->
> > pci_create_root_bus() where the last function is called with
> > parent=NULL. It indicates that no parent is present and then
> > bus->bridge->parent is NULL too.
> > 
> > Fix the problem by checking bus->bridge->parent in xen_dt_get_node() for
> > NULL first >
> > Fixes: ef8ae384b4c9 ("xen/virtio: Handle PCI devices which Host controller is described in DT")
> 
> Oops, sorry. I have to admit I checked with DT only.
> 
> 
> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> 
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

[PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Petr Pavlu]

When attempting to run Xen on a QEMU/KVM virtual machine with virtio
devices (all x86_64), dom0 tries to establish a grant for itself which
eventually results in a hang during the boot.

The backtrace looks as follows, the while loop in __send_control_msg()
makes no progress:

  #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
  #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
  #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
  #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
  #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
  #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
  #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
  #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
  #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
  #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
  #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
  #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
      fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
  #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
  #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
  #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
  #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
  #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
  #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
  #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
  #19 do_initcalls () at ../init/main.c:1335
  #20 do_basic_setup () at ../init/main.c:1354
  #21 kernel_init_freeable () at ../init/main.c:1571
  #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
  #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
  #24 0x0000000000000000 in ?? ()

Fix the problem by preventing xen_grant_init_backend_domid() from
setting dom0 as a backend when running in dom0.

Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 drivers/xen/grant-dma-ops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 76f6f26265a3..29ed27ac450e 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
 	if (np) {
 		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
 		of_node_put(np);
-	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
+	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
+		    xen_pv_domain()) &&
+		   !xen_initial_domain()) {
 		dev_info(dev, "Using dom0 as backend\n");
 		*backend_domid = 0;
 		ret = 0;
-- 
2.35.3

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 21.06.23 16:12, Petr Pavlu wrote:


Hello Petr


> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> devices (all x86_64), dom0 tries to establish a grant for itself which
> eventually results in a hang during the boot.
> 
> The backtrace looks as follows, the while loop in __send_control_msg()
> makes no progress:
> 
>    #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>    #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>    #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>    #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>    #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>    #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>    #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>    #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>    #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>    #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>    #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>    #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>        fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>    #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>    #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>    #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>    #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>    #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>    #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>    #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>    #19 do_initcalls () at ../init/main.c:1335
>    #20 do_basic_setup () at ../init/main.c:1354
>    #21 kernel_init_freeable () at ../init/main.c:1571
>    #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>    #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>    #24 0x0000000000000000 in ?? ()
> 
> Fix the problem by preventing xen_grant_init_backend_domid() from
> setting dom0 as a backend when running in dom0.
> 
> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")


I am not 100% sure whether the Fixes tag points to precise commit. If I 
am not mistaken, the said commit just moves the code in the context 
without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was 
introduced before.


> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>   drivers/xen/grant-dma-ops.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 76f6f26265a3..29ed27ac450e 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>   	if (np) {
>   		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>   		of_node_put(np);
> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> +		    xen_pv_domain()) &&
> +		   !xen_initial_domain()) {

The commit lgtm, just one note:


I would even bail out early in xen_virtio_restricted_mem_acc() instead,
as I assume the same issue could happen on Arm with DT (although there 
we don't guess the backend's domid, we read it from DT and quite 
unlikely we get Dom0 being in Dom0 with correct DT).

Something like:

@@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct 
virtio_device *dev)
  {
         domid_t backend_domid;

+       /* Xen grant DMA ops are not used when running as initial domain */
+       if (xen_initial_domain())
+               return false;
+
         if (!xen_grant_init_backend_domid(dev->dev.parent, 
&backend_domid)) {
                 xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
                 return true;
(END)



If so, that commit subject would need to be updated accordingly.

Let's see what other reviewers will say.





>   		dev_info(dev, "Using dom0 as backend\n");
>   		*backend_domid = 0;
>   		ret = 0;

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Petr Pavlu]

On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
> On 21.06.23 16:12, Petr Pavlu wrote:
>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>> devices (all x86_64), dom0 tries to establish a grant for itself which
>> eventually results in a hang during the boot.
>>
>> The backtrace looks as follows, the while loop in __send_control_msg()
>> makes no progress:
>>
>>    #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>    #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>>    #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>>    #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>    #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>    #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>    #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>    #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>>    #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>    #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>    #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>    #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>        fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>>    #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>>    #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>>    #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>>    #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>>    #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>>    #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>>    #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>>    #19 do_initcalls () at ../init/main.c:1335
>>    #20 do_basic_setup () at ../init/main.c:1354
>>    #21 kernel_init_freeable () at ../init/main.c:1571
>>    #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>>    #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>>    #24 0x0000000000000000 in ?? ()
>>
>> Fix the problem by preventing xen_grant_init_backend_domid() from
>> setting dom0 as a backend when running in dom0.
>>
>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
> 
> 
> I am not 100% sure whether the Fixes tag points to precise commit. If I 
> am not mistaken, the said commit just moves the code in the context 
> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was 
> introduced before.

I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
introduced the original logic to use dom0 as backend.

Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
devices") is relevant in sense that it extended when this logic is
active by adding an OR check for xen_pv_domain().

> 
> 
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> ---
>>   drivers/xen/grant-dma-ops.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index 76f6f26265a3..29ed27ac450e 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>>   	if (np) {
>>   		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>>   		of_node_put(np);
>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>> +		    xen_pv_domain()) &&
>> +		   !xen_initial_domain()) {
> 
> The commit lgtm, just one note:
> 
> 
> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
> as I assume the same issue could happen on Arm with DT (although there 
> we don't guess the backend's domid, we read it from DT and quite 
> unlikely we get Dom0 being in Dom0 with correct DT).
> 
> Something like:
> 
> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct 
> virtio_device *dev)
>   {
>          domid_t backend_domid;
> 
> +       /* Xen grant DMA ops are not used when running as initial domain */
> +       if (xen_initial_domain())
> +               return false;
> +
>          if (!xen_grant_init_backend_domid(dev->dev.parent, 
> &backend_domid)) {
>                  xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>                  return true;
> (END)
> 
> 
> 
> If so, that commit subject would need to be updated accordingly.
> 
> Let's see what other reviewers will say.

Ok, makes sense.

Thanks,
Petr

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 6766 bytes --]

On 26.06.23 15:17, Petr Pavlu wrote:
> On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
>> On 21.06.23 16:12, Petr Pavlu wrote:
>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>> eventually results in a hang during the boot.
>>>
>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>> makes no progress:
>>>
>>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>     #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>     #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>     #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>     #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>>>     #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>     #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>     #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>>>     #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>>>     #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>>>     #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>>>     #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>>>     #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>>>     #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>>>     #19 do_initcalls () at ../init/main.c:1335
>>>     #20 do_basic_setup () at ../init/main.c:1354
>>>     #21 kernel_init_freeable () at ../init/main.c:1571
>>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>>>     #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>>>     #24 0x0000000000000000 in ?? ()
>>>
>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>> setting dom0 as a backend when running in dom0.
>>>
>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
>>
>>
>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>> am not mistaken, the said commit just moves the code in the context
>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>> introduced before.
> 
> I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
> dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
> introduced the original logic to use dom0 as backend.
> 
> Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
> devices") is relevant in sense that it extended when this logic is
> active by adding an OR check for xen_pv_domain().
> 
>>
>>
>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>> ---
>>>    drivers/xen/grant-dma-ops.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>> index 76f6f26265a3..29ed27ac450e 100644
>>> --- a/drivers/xen/grant-dma-ops.c
>>> +++ b/drivers/xen/grant-dma-ops.c
>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>>>    	if (np) {
>>>    		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>>>    		of_node_put(np);
>>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
>>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>> +		    xen_pv_domain()) &&
>>> +		   !xen_initial_domain()) {
>>
>> The commit lgtm, just one note:
>>
>>
>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>> as I assume the same issue could happen on Arm with DT (although there
>> we don't guess the backend's domid, we read it from DT and quite
>> unlikely we get Dom0 being in Dom0 with correct DT).
>>
>> Something like:
>>
>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>> virtio_device *dev)
>>    {
>>           domid_t backend_domid;
>>
>> +       /* Xen grant DMA ops are not used when running as initial domain */
>> +       if (xen_initial_domain())
>> +               return false;
>> +
>>           if (!xen_grant_init_backend_domid(dev->dev.parent,
>> &backend_domid)) {
>>                   xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>>                   return true;
>> (END)
>>
>>
>>
>> If so, that commit subject would need to be updated accordingly.
>>
>> Let's see what other reviewers will say.
> 
> Ok, makes sense.

I think this is okay for a fix of the current problem.

Passing through virtio devices to a PV domU is not covered by this fix, but this
should be a rather rare configuration, which doesn't work today either. So the
suggested patch would fix the current issue without introducing a regression.

Anything else can be done later.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

On Fri, 7 Jul 2023, Juergen Gross wrote:
> On 26.06.23 15:17, Petr Pavlu wrote:
> > On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
> > > On 21.06.23 16:12, Petr Pavlu wrote:
> > > > When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> > > > devices (all x86_64), dom0 tries to establish a grant for itself which
> > > > eventually results in a hang during the boot.
> > > > 
> > > > The backtrace looks as follows, the while loop in __send_control_msg()
> > > > makes no progress:
> > > > 
> > > >     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400,
> > > > len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>)
> > > > at ../drivers/virtio/virtio_ring.c:2326
> > > >     #1  0xffffffff817086b7 in virtqueue_get_buf
> > > > (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at
> > > > ../drivers/virtio/virtio_ring.c:2333
> > > >     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized
> > > > out>, port_id=0xffffffff, event=0x0, value=0x1) at
> > > > ../drivers/char/virtio_console.c:562
> > > >     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized
> > > > out>, port_id=<optimized out>, event=<optimized out>, value=<optimized
> > > > out>) at ../drivers/char/virtio_console.c:569
> > > >     #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800)
> > > > at ../drivers/char/virtio_console.c:2098
> > > >     #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810)
> > > > at ../drivers/virtio/virtio.c:305
> > > >     #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0
> > > > <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>,
> > > > dev=0xffff88800585e810) at ../drivers/base/dd.c:579
> > > >     #7  really_probe (dev=dev@entry=0xffff88800585e810,
> > > > drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > ../drivers/base/dd.c:658
> > > >     #8  0xffffffff8198e58f in __driver_probe_device
> > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
> > > > dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
> > > >     #9  0xffffffff8198e65a in driver_probe_device
> > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
> > > > dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
> > > >     #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810,
> > > > data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
> > > >     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>,
> > > > start=start@entry=0x0 <fixed_percpu_data>,
> > > > data=data@entry=0xffffffff82be40c0 <virtio_console>,
> > > >         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at
> > > > ../drivers/base/bus.c:368
> > > >     #12 0xffffffff8198db65 in driver_attach
> > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > ../drivers/base/dd.c:1233
> > > >     #13 0xffffffff8198d207 in bus_add_driver
> > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > ../drivers/base/bus.c:673
> > > >     #14 0xffffffff8198f550 in driver_register
> > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > ../drivers/base/driver.c:246
> > > >     #15 0xffffffff81706b47 in register_virtio_driver
> > > > (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > ../drivers/virtio/virtio.c:357
> > > >     #16 0xffffffff832cd34b in virtio_console_init () at
> > > > ../drivers/char/virtio_console.c:2258
> > > >     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0
> > > > <virtio_console_init>) at ../init/main.c:1246
> > > >     #18 0xffffffff83277293 in do_initcall_level
> > > > (command_line=0xffff888003e2f900 "root", level=0x6) at
> > > > ../init/main.c:1319
> > > >     #19 do_initcalls () at ../init/main.c:1335
> > > >     #20 do_basic_setup () at ../init/main.c:1354
> > > >     #21 kernel_init_freeable () at ../init/main.c:1571
> > > >     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at
> > > > ../init/main.c:1462
> > > >     #23 0xffffffff81001f49 in ret_from_fork () at
> > > > ../arch/x86/entry/entry_64.S:308
> > > >     #24 0x0000000000000000 in ?? ()
> > > > 
> > > > Fix the problem by preventing xen_grant_init_backend_domid() from
> > > > setting dom0 as a backend when running in dom0.
> > > > 
> > > > Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
> > > > devices")
> > > 
> > > 
> > > I am not 100% sure whether the Fixes tag points to precise commit. If I
> > > am not mistaken, the said commit just moves the code in the context
> > > without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
> > > introduced before.
> > 
> > I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
> > dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
> > introduced the original logic to use dom0 as backend.
> > 
> > Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
> > devices") is relevant in sense that it extended when this logic is
> > active by adding an OR check for xen_pv_domain().
> > 
> > > 
> > > 
> > > > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > > > ---
> > > >    drivers/xen/grant-dma-ops.c | 4 +++-
> > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> > > > index 76f6f26265a3..29ed27ac450e 100644
> > > > --- a/drivers/xen/grant-dma-ops.c
> > > > +++ b/drivers/xen/grant-dma-ops.c
> > > > @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct
> > > > device *dev,
> > > >    	if (np) {
> > > >    		ret = xen_dt_grant_init_backend_domid(dev, np,
> > > > backend_domid);
> > > >    		of_node_put(np);
> > > > -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> > > > xen_pv_domain()) {
> > > > +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> > > > +		    xen_pv_domain()) &&
> > > > +		   !xen_initial_domain()) {
> > > 
> > > The commit lgtm, just one note:
> > > 
> > > 
> > > I would even bail out early in xen_virtio_restricted_mem_acc() instead,
> > > as I assume the same issue could happen on Arm with DT (although there
> > > we don't guess the backend's domid, we read it from DT and quite
> > > unlikely we get Dom0 being in Dom0 with correct DT).
> > > 
> > > Something like:
> > > 
> > > @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
> > > virtio_device *dev)
> > >    {
> > >           domid_t backend_domid;
> > > 
> > > +       /* Xen grant DMA ops are not used when running as initial domain
> > > */
> > > +       if (xen_initial_domain())
> > > +               return false;
> > > +
> > >           if (!xen_grant_init_backend_domid(dev->dev.parent,
> > > &backend_domid)) {
> > >                   xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
> > >                   return true;
> > > (END)
> > > 
> > > 
> > > 
> > > If so, that commit subject would need to be updated accordingly.
> > > 
> > > Let's see what other reviewers will say.
> > 
> > Ok, makes sense.
> 
> I think this is okay for a fix of the current problem.
> 
> Passing through virtio devices to a PV domU is not covered by this fix, but
> this
> should be a rather rare configuration, which doesn't work today either. So the
> suggested patch would fix the current issue without introducing a regression.
> 
> Anything else can be done later.

Why do you say that passing through virtio devices to a PV domU doesn't
work today anyway? Also, as you know many people use Xen outside of
datacenter deployments (laptops, embedded etc.) where drivers domains
and device assignment are very common. You could assign a virtio network
card to a domU and use PV network to share the network with other
guests. Physical virtio devices, especially virtio-net devices, exist. I
could probably repro this problem today in a domU just installing
QubesOS inside QEMU. QubesOS uses network driver domains and if QEMU
provides a virtio-net network card, this would break even with this
patch.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 8454 bytes --]

On 07.07.23 23:02, Stefano Stabellini wrote:
> On Fri, 7 Jul 2023, Juergen Gross wrote:
>> On 26.06.23 15:17, Petr Pavlu wrote:
>>> On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
>>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>>>> eventually results in a hang during the boot.
>>>>>
>>>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>>>> makes no progress:
>>>>>
>>>>>      #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400,
>>>>> len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>)
>>>>> at ../drivers/virtio/virtio_ring.c:2326
>>>>>      #1  0xffffffff817086b7 in virtqueue_get_buf
>>>>> (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at
>>>>> ../drivers/virtio/virtio_ring.c:2333
>>>>>      #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized
>>>>> out>, port_id=0xffffffff, event=0x0, value=0x1) at
>>>>> ../drivers/char/virtio_console.c:562
>>>>>      #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized
>>>>> out>, port_id=<optimized out>, event=<optimized out>, value=<optimized
>>>>> out>) at ../drivers/char/virtio_console.c:569
>>>>>      #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800)
>>>>> at ../drivers/char/virtio_console.c:2098
>>>>>      #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810)
>>>>> at ../drivers/virtio/virtio.c:305
>>>>>      #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0
>>>>> <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>>      #7  really_probe (dev=dev@entry=0xffff88800585e810,
>>>>> drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:658
>>>>>      #8  0xffffffff8198e58f in __driver_probe_device
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>>      #9  0xffffffff8198e65a in driver_probe_device
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>>      #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810,
>>>>> data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>>>>      #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>,
>>>>> start=start@entry=0x0 <fixed_percpu_data>,
>>>>> data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>          fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at
>>>>> ../drivers/base/bus.c:368
>>>>>      #12 0xffffffff8198db65 in driver_attach
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:1233
>>>>>      #13 0xffffffff8198d207 in bus_add_driver
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/bus.c:673
>>>>>      #14 0xffffffff8198f550 in driver_register
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/driver.c:246
>>>>>      #15 0xffffffff81706b47 in register_virtio_driver
>>>>> (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/virtio/virtio.c:357
>>>>>      #16 0xffffffff832cd34b in virtio_console_init () at
>>>>> ../drivers/char/virtio_console.c:2258
>>>>>      #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0
>>>>> <virtio_console_init>) at ../init/main.c:1246
>>>>>      #18 0xffffffff83277293 in do_initcall_level
>>>>> (command_line=0xffff888003e2f900 "root", level=0x6) at
>>>>> ../init/main.c:1319
>>>>>      #19 do_initcalls () at ../init/main.c:1335
>>>>>      #20 do_basic_setup () at ../init/main.c:1354
>>>>>      #21 kernel_init_freeable () at ../init/main.c:1571
>>>>>      #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at
>>>>> ../init/main.c:1462
>>>>>      #23 0xffffffff81001f49 in ret_from_fork () at
>>>>> ../arch/x86/entry/entry_64.S:308
>>>>>      #24 0x0000000000000000 in ?? ()
>>>>>
>>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>>> setting dom0 as a backend when running in dom0.
>>>>>
>>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
>>>>> devices")
>>>>
>>>>
>>>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>>>> am not mistaken, the said commit just moves the code in the context
>>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>>> introduced before.
>>>
>>> I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
>>> dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
>>> introduced the original logic to use dom0 as backend.
>>>
>>> Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
>>> devices") is relevant in sense that it extended when this logic is
>>> active by adding an OR check for xen_pv_domain().
>>>
>>>>
>>>>
>>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>>>> ---
>>>>>     drivers/xen/grant-dma-ops.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>>> --- a/drivers/xen/grant-dma-ops.c
>>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct
>>>>> device *dev,
>>>>>     	if (np) {
>>>>>     		ret = xen_dt_grant_init_backend_domid(dev, np,
>>>>> backend_domid);
>>>>>     		of_node_put(np);
>>>>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>> xen_pv_domain()) {
>>>>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>> +		    xen_pv_domain()) &&
>>>>> +		   !xen_initial_domain()) {
>>>>
>>>> The commit lgtm, just one note:
>>>>
>>>>
>>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>>>> as I assume the same issue could happen on Arm with DT (although there
>>>> we don't guess the backend's domid, we read it from DT and quite
>>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>>
>>>> Something like:
>>>>
>>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>>> virtio_device *dev)
>>>>     {
>>>>            domid_t backend_domid;
>>>>
>>>> +       /* Xen grant DMA ops are not used when running as initial domain
>>>> */
>>>> +       if (xen_initial_domain())
>>>> +               return false;
>>>> +
>>>>            if (!xen_grant_init_backend_domid(dev->dev.parent,
>>>> &backend_domid)) {
>>>>                    xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>>>>                    return true;
>>>> (END)
>>>>
>>>>
>>>>
>>>> If so, that commit subject would need to be updated accordingly.
>>>>
>>>> Let's see what other reviewers will say.
>>>
>>> Ok, makes sense.
>>
>> I think this is okay for a fix of the current problem.
>>
>> Passing through virtio devices to a PV domU is not covered by this fix, but
>> this
>> should be a rather rare configuration, which doesn't work today either. So the
>> suggested patch would fix the current issue without introducing a regression.
>>
>> Anything else can be done later.
> 
> Why do you say that passing through virtio devices to a PV domU doesn't
> work today anyway? Also, as you know many people use Xen outside of
> datacenter deployments (laptops, embedded etc.) where drivers domains
> and device assignment are very common. You could assign a virtio network
> card to a domU and use PV network to share the network with other
> guests. Physical virtio devices, especially virtio-net devices, exist. I
> could probably repro this problem today in a domU just installing
> QubesOS inside QEMU. QubesOS uses network driver domains and if QEMU
> provides a virtio-net network card, this would break even with this
> patch.

I might be wrong, but I don't think all virtio frontends will work in that
scenario. The main reason is the PFN/MFN difference: a frontend using guest
consecutive memory for doing large I/Os will fail miserably. This was the
main reason why I had to add the functionality of consecutive grants for
large I/O buffers. The same goes for multi-page virtio ring pages.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

On Sat, 8 Jul 2023, Juergen Gross wrote:
> On 07.07.23 23:02, Stefano Stabellini wrote:
> > On Fri, 7 Jul 2023, Juergen Gross wrote:
> > > On 26.06.23 15:17, Petr Pavlu wrote:
> > > > On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
> > > > > On 21.06.23 16:12, Petr Pavlu wrote:
> > > > > > When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> > > > > > devices (all x86_64), dom0 tries to establish a grant for itself
> > > > > > which
> > > > > > eventually results in a hang during the boot.
> > > > > > 
> > > > > > The backtrace looks as follows, the while loop in
> > > > > > __send_control_msg()
> > > > > > makes no progress:
> > > > > > 
> > > > > >      #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400,
> > > > > > len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0
> > > > > > <fixed_percpu_data>)
> > > > > > at ../drivers/virtio/virtio_ring.c:2326
> > > > > >      #1  0xffffffff817086b7 in virtqueue_get_buf
> > > > > > (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94)
> > > > > > at
> > > > > > ../drivers/virtio/virtio_ring.c:2333
> > > > > >      #2  0xffffffff8175f6b2 in __send_control_msg
> > > > > > (portdev=<optimized
> > > > > > out>, port_id=0xffffffff, event=0x0, value=0x1) at
> > > > > > ../drivers/char/virtio_console.c:562
> > > > > >      #3  0xffffffff8175f6ee in __send_control_msg
> > > > > > (portdev=<optimized
> > > > > > out>, port_id=<optimized out>, event=<optimized out>,
> > > > > > value=<optimized
> > > > > > out>) at ../drivers/char/virtio_console.c:569
> > > > > >      #4  0xffffffff817618b1 in virtcons_probe
> > > > > > (vdev=0xffff88800585e800)
> > > > > > at ../drivers/char/virtio_console.c:2098
> > > > > >      #5  0xffffffff81707117 in virtio_dev_probe
> > > > > > (_d=0xffff88800585e810)
> > > > > > at ../drivers/virtio/virtio.c:305
> > > > > >      #6  0xffffffff8198e348 in call_driver_probe
> > > > > > (drv=0xffffffff82be40c0
> > > > > > <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>,
> > > > > > dev=0xffff88800585e810) at ../drivers/base/dd.c:579
> > > > > >      #7  really_probe (dev=dev@entry=0xffff88800585e810,
> > > > > > drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/base/dd.c:658
> > > > > >      #8  0xffffffff8198e58f in __driver_probe_device
> > > > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
> > > > > > dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
> > > > > >      #9  0xffffffff8198e65a in driver_probe_device
> > > > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
> > > > > > dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
> > > > > >      #10 0xffffffff8198e832 in __driver_attach
> > > > > > (dev=0xffff88800585e810,
> > > > > > data=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/base/dd.c:1216
> > > > > >      #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized
> > > > > > out>,
> > > > > > start=start@entry=0x0 <fixed_percpu_data>,
> > > > > > data=data@entry=0xffffffff82be40c0 <virtio_console>,
> > > > > >          fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at
> > > > > > ../drivers/base/bus.c:368
> > > > > >      #12 0xffffffff8198db65 in driver_attach
> > > > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/base/dd.c:1233
> > > > > >      #13 0xffffffff8198d207 in bus_add_driver
> > > > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/base/bus.c:673
> > > > > >      #14 0xffffffff8198f550 in driver_register
> > > > > > (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/base/driver.c:246
> > > > > >      #15 0xffffffff81706b47 in register_virtio_driver
> > > > > > (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at
> > > > > > ../drivers/virtio/virtio.c:357
> > > > > >      #16 0xffffffff832cd34b in virtio_console_init () at
> > > > > > ../drivers/char/virtio_console.c:2258
> > > > > >      #17 0xffffffff8100105c in do_one_initcall
> > > > > > (fn=0xffffffff832cd2e0
> > > > > > <virtio_console_init>) at ../init/main.c:1246
> > > > > >      #18 0xffffffff83277293 in do_initcall_level
> > > > > > (command_line=0xffff888003e2f900 "root", level=0x6) at
> > > > > > ../init/main.c:1319
> > > > > >      #19 do_initcalls () at ../init/main.c:1335
> > > > > >      #20 do_basic_setup () at ../init/main.c:1354
> > > > > >      #21 kernel_init_freeable () at ../init/main.c:1571
> > > > > >      #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>)
> > > > > > at
> > > > > > ../init/main.c:1462
> > > > > >      #23 0xffffffff81001f49 in ret_from_fork () at
> > > > > > ../arch/x86/entry/entry_64.S:308
> > > > > >      #24 0x0000000000000000 in ?? ()
> > > > > > 
> > > > > > Fix the problem by preventing xen_grant_init_backend_domid() from
> > > > > > setting dom0 as a backend when running in dom0.
> > > > > > 
> > > > > > Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of
> > > > > > "xen-grant-dma"
> > > > > > devices")
> > > > > 
> > > > > 
> > > > > I am not 100% sure whether the Fixes tag points to precise commit. If
> > > > > I
> > > > > am not mistaken, the said commit just moves the code in the context
> > > > > without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
> > > > > introduced before.
> > > > 
> > > > I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
> > > > dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
> > > > introduced the original logic to use dom0 as backend.
> > > > 
> > > > Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
> > > > devices") is relevant in sense that it extended when this logic is
> > > > active by adding an OR check for xen_pv_domain().
> > > > 
> > > > > 
> > > > > 
> > > > > > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > > > > > ---
> > > > > >     drivers/xen/grant-dma-ops.c | 4 +++-
> > > > > >     1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/xen/grant-dma-ops.c
> > > > > > b/drivers/xen/grant-dma-ops.c
> > > > > > index 76f6f26265a3..29ed27ac450e 100644
> > > > > > --- a/drivers/xen/grant-dma-ops.c
> > > > > > +++ b/drivers/xen/grant-dma-ops.c
> > > > > > @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct
> > > > > > device *dev,
> > > > > >     	if (np) {
> > > > > >     		ret = xen_dt_grant_init_backend_domid(dev, np,
> > > > > > backend_domid);
> > > > > >     		of_node_put(np);
> > > > > > -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> > > > > > xen_pv_domain()) {
> > > > > > +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> > > > > > +		    xen_pv_domain()) &&
> > > > > > +		   !xen_initial_domain()) {
> > > > > 
> > > > > The commit lgtm, just one note:
> > > > > 
> > > > > 
> > > > > I would even bail out early in xen_virtio_restricted_mem_acc()
> > > > > instead,
> > > > > as I assume the same issue could happen on Arm with DT (although there
> > > > > we don't guess the backend's domid, we read it from DT and quite
> > > > > unlikely we get Dom0 being in Dom0 with correct DT).
> > > > > 
> > > > > Something like:
> > > > > 
> > > > > @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
> > > > > virtio_device *dev)
> > > > >     {
> > > > >            domid_t backend_domid;
> > > > > 
> > > > > +       /* Xen grant DMA ops are not used when running as initial
> > > > > domain
> > > > > */
> > > > > +       if (xen_initial_domain())
> > > > > +               return false;
> > > > > +
> > > > >            if (!xen_grant_init_backend_domid(dev->dev.parent,
> > > > > &backend_domid)) {
> > > > >                    xen_grant_setup_dma_ops(dev->dev.parent,
> > > > > backend_domid);
> > > > >                    return true;
> > > > > (END)
> > > > > 
> > > > > 
> > > > > 
> > > > > If so, that commit subject would need to be updated accordingly.
> > > > > 
> > > > > Let's see what other reviewers will say.
> > > > 
> > > > Ok, makes sense.
> > > 
> > > I think this is okay for a fix of the current problem.
> > > 
> > > Passing through virtio devices to a PV domU is not covered by this fix,
> > > but
> > > this
> > > should be a rather rare configuration, which doesn't work today either. So
> > > the
> > > suggested patch would fix the current issue without introducing a
> > > regression.
> > > 
> > > Anything else can be done later.
> > 
> > Why do you say that passing through virtio devices to a PV domU doesn't
> > work today anyway? Also, as you know many people use Xen outside of
> > datacenter deployments (laptops, embedded etc.) where drivers domains
> > and device assignment are very common. You could assign a virtio network
> > card to a domU and use PV network to share the network with other
> > guests. Physical virtio devices, especially virtio-net devices, exist. I
> > could probably repro this problem today in a domU just installing
> > QubesOS inside QEMU. QubesOS uses network driver domains and if QEMU
> > provides a virtio-net network card, this would break even with this
> > patch.
> 
> I might be wrong, but I don't think all virtio frontends will work in that
> scenario. The main reason is the PFN/MFN difference: a frontend using guest
> consecutive memory for doing large I/Os will fail miserably. This was the
> main reason why I had to add the functionality of consecutive grants for
> large I/O buffers. The same goes for multi-page virtio ring pages.

I think for PV guests the virtio frontends would work but they might be
slow as they would have to bounce over swiotlb-xen every multi-page
buffer. But the virtio frontends should work OK for PVH and HVM guests.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
> On 21.06.23 16:12, Petr Pavlu wrote:
> 
> 
> Hello Petr
> 
> 
> > When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> > devices (all x86_64), dom0 tries to establish a grant for itself which
> > eventually results in a hang during the boot.
> > 
> > The backtrace looks as follows, the while loop in __send_control_msg()
> > makes no progress:
> > 
> >    #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
> >    #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
> >    #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
> >    #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
> >    #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
> >    #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
> >    #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
> >    #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
> >    #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
> >    #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
> >    #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
> >    #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
> >        fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
> >    #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
> >    #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
> >    #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
> >    #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
> >    #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
> >    #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
> >    #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
> >    #19 do_initcalls () at ../init/main.c:1335
> >    #20 do_basic_setup () at ../init/main.c:1354
> >    #21 kernel_init_freeable () at ../init/main.c:1571
> >    #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
> >    #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
> >    #24 0x0000000000000000 in ?? ()
> > 
> > Fix the problem by preventing xen_grant_init_backend_domid() from
> > setting dom0 as a backend when running in dom0.
> > 
> > Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
> 
> 
> I am not 100% sure whether the Fixes tag points to precise commit. If I 
> am not mistaken, the said commit just moves the code in the context 
> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was 
> introduced before.
> 
> 
> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > ---
> >   drivers/xen/grant-dma-ops.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> > index 76f6f26265a3..29ed27ac450e 100644
> > --- a/drivers/xen/grant-dma-ops.c
> > +++ b/drivers/xen/grant-dma-ops.c
> > @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
> >   	if (np) {
> >   		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
> >   		of_node_put(np);
> > -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
> > +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> > +		    xen_pv_domain()) &&
> > +		   !xen_initial_domain()) {
> 
> The commit lgtm, just one note:
> 
> 
> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
> as I assume the same issue could happen on Arm with DT (although there 
> we don't guess the backend's domid, we read it from DT and quite 
> unlikely we get Dom0 being in Dom0 with correct DT).
> 
> Something like:
> 
> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct 
> virtio_device *dev)
>   {
>          domid_t backend_domid;
> 
> +       /* Xen grant DMA ops are not used when running as initial domain */
> +       if (xen_initial_domain())
> +               return false;
> +
>          if (!xen_grant_init_backend_domid(dev->dev.parent, 
> &backend_domid)) {
>                  xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>                  return true;
> (END)
> 
> 
> 
> If so, that commit subject would need to be updated accordingly.
> 
> Let's see what other reviewers will say.

This doesn't work in all cases. Imagine using PCI Passthrough to assign
a "physical" virtio device to a domU. The domU will run into the same
error, right?

The problem is that we need a way for the virtio backend to advertise
its ability of handling grants. Right now we only have a way to do with
that with device tree on ARM. On x86, we only have
CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
"physical" virtio devices. Note that in this case we are fixing a
nested-virtualization bug, but there are actually physical
virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
break those too.

I think we need to add a second way? It could be anything that can help
us distinguish between a non-grants-capable virtio backend and a
grants-capable virtio backend, such as:
- a string on xenstore
- a xen param
- a special PCI configuration register value
- something in the ACPI tables
- the QEMU machine type

Or at least should we change CONFIG_XEN_VIRTIO_FORCE_GRANT into a
command line parameter so that it can be disabled in cases like this
one?

I realize that fixing this problem properly takes a lot longer than
adding a trivial if (dom0) return; check in the code. If you cannot find
a good way to solve the problem or you don't have time to do that now
and you need this bug fixed quickly, then I would be OK with the if
(dom0) return; check but please add a detailed TODO in-code comment to
explain that this is just a hack and we are still looking for a real
solution.

The check itself I prefer the original position because I want to retain
the ability of using virtio frontends with grant on ARM in Dom0 (DomD
case).

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 29.06.23 04:00, Stefano Stabellini wrote:

Hello Stefano

> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>> On 21.06.23 16:12, Petr Pavlu wrote:
>>
>>
>> Hello Petr
>>
>>
>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>> eventually results in a hang during the boot.
>>>
>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>> makes no progress:
>>>
>>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>     #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>     #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>     #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>     #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>>>     #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>     #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>     #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>>>     #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>>>     #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>>>     #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>>>     #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>>>     #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>>>     #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>>>     #19 do_initcalls () at ../init/main.c:1335
>>>     #20 do_basic_setup () at ../init/main.c:1354
>>>     #21 kernel_init_freeable () at ../init/main.c:1571
>>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>>>     #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>>>     #24 0x0000000000000000 in ?? ()
>>>
>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>> setting dom0 as a backend when running in dom0.
>>>
>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
>>
>>
>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>> am not mistaken, the said commit just moves the code in the context
>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>> introduced before.
>>
>>
>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>> ---
>>>    drivers/xen/grant-dma-ops.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>> index 76f6f26265a3..29ed27ac450e 100644
>>> --- a/drivers/xen/grant-dma-ops.c
>>> +++ b/drivers/xen/grant-dma-ops.c
>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>>>    	if (np) {
>>>    		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>>>    		of_node_put(np);
>>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
>>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>> +		    xen_pv_domain()) &&
>>> +		   !xen_initial_domain()) {
>>
>> The commit lgtm, just one note:
>>
>>
>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>> as I assume the same issue could happen on Arm with DT (although there
>> we don't guess the backend's domid, we read it from DT and quite
>> unlikely we get Dom0 being in Dom0 with correct DT).
>>
>> Something like:
>>
>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>> virtio_device *dev)
>>    {
>>           domid_t backend_domid;
>>
>> +       /* Xen grant DMA ops are not used when running as initial domain */
>> +       if (xen_initial_domain())
>> +               return false;
>> +
>>           if (!xen_grant_init_backend_domid(dev->dev.parent,
>> &backend_domid)) {
>>                   xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>>                   return true;
>> (END)
>>
>>
>>
>> If so, that commit subject would need to be updated accordingly.
>>
>> Let's see what other reviewers will say.
> 
> This doesn't work in all cases. Imagine using PCI Passthrough to assign
> a "physical" virtio device to a domU. The domU will run into the same
> error, right?
> 
> The problem is that we need a way for the virtio backend to advertise
> its ability of handling grants. Right now we only have a way to do with
> that with device tree on ARM. On x86, we only have
> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
> "physical" virtio devices. Note that in this case we are fixing a
> nested-virtualization bug, but there are actually physical
> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
> break those too.


If these "physical" virtio devices are also spawned by
drivers/virtio/virtio.c:virtio_dev_probe(), then yes, otherwise I don't 
see how this could even be possible, but I might miss something here.

xen_virtio_restricted_mem_acc() gets called indirectly from 
virtio_dev_probe()->virtio_features_ok()->
virtio_check_mem_acc_cb(). So the Xen grant DMA ops are only installed 
for those.

> 
> I think we need to add a second way? It could be anything that can help
> us distinguish between a non-grants-capable virtio backend and a
> grants-capable virtio backend, such as:
> - a string on xenstore
> - a xen param
> - a special PCI configuration register value
> - something in the ACPI tables
> - the QEMU machine type


Yes, I remember there was a discussion regarding that. The point is to 
choose a solution to be functional for both PV and HVM *and* to be able 
to support a hotplug. IIRC, the xenstore could be a possible candidate.


> 
> Or at least should we change CONFIG_XEN_VIRTIO_FORCE_GRANT into a
> command line parameter so that it can be disabled in cases like this
> one?

IIUC, this will help with HVM only.


> 
> I realize that fixing this problem properly takes a lot longer than
> adding a trivial if (dom0) return; check in the code. If you cannot find
> a good way to solve the problem or you don't have time to do that now
> and you need this bug fixed quickly, then I would be OK with the if
> (dom0) return; check but please add a detailed TODO in-code comment to
> explain that this is just a hack and we are still looking for a real
> solution.
> 
> The check itself I prefer the original position because I want to retain
> the ability of using virtio frontends with grant on ARM in Dom0 (DomD
> case).

Makes sense, agree.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> On 29.06.23 04:00, Stefano Stabellini wrote:
> 
> Hello Stefano
> 
> > On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
> >> On 21.06.23 16:12, Petr Pavlu wrote:
> >>
> >>
> >> Hello Petr
> >>
> >>
> >>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> >>> devices (all x86_64), dom0 tries to establish a grant for itself which
> >>> eventually results in a hang during the boot.
> >>>
> >>> The backtrace looks as follows, the while loop in __send_control_msg()
> >>> makes no progress:
> >>>
> >>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
> >>>     #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
> >>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
> >>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
> >>>     #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
> >>>     #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
> >>>     #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
> >>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
> >>>     #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
> >>>     #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
> >>>     #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
> >>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
> >>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
> >>>     #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
> >>>     #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
> >>>     #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
> >>>     #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
> >>>     #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
> >>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
> >>>     #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
> >>>     #19 do_initcalls () at ../init/main.c:1335
> >>>     #20 do_basic_setup () at ../init/main.c:1354
> >>>     #21 kernel_init_freeable () at ../init/main.c:1571
> >>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
> >>>     #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
> >>>     #24 0x0000000000000000 in ?? ()
> >>>
> >>> Fix the problem by preventing xen_grant_init_backend_domid() from
> >>> setting dom0 as a backend when running in dom0.
> >>>
> >>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
> >>
> >>
> >> I am not 100% sure whether the Fixes tag points to precise commit. If I
> >> am not mistaken, the said commit just moves the code in the context
> >> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
> >> introduced before.
> >>
> >>
> >>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> >>> ---
> >>>    drivers/xen/grant-dma-ops.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> >>> index 76f6f26265a3..29ed27ac450e 100644
> >>> --- a/drivers/xen/grant-dma-ops.c
> >>> +++ b/drivers/xen/grant-dma-ops.c
> >>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
> >>>    	if (np) {
> >>>    		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
> >>>    		of_node_put(np);
> >>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
> >>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> >>> +		    xen_pv_domain()) &&
> >>> +		   !xen_initial_domain()) {
> >>
> >> The commit lgtm, just one note:
> >>
> >>
> >> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
> >> as I assume the same issue could happen on Arm with DT (although there
> >> we don't guess the backend's domid, we read it from DT and quite
> >> unlikely we get Dom0 being in Dom0 with correct DT).
> >>
> >> Something like:
> >>
> >> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
> >> virtio_device *dev)
> >>    {
> >>           domid_t backend_domid;
> >>
> >> +       /* Xen grant DMA ops are not used when running as initial domain */
> >> +       if (xen_initial_domain())
> >> +               return false;
> >> +
> >>           if (!xen_grant_init_backend_domid(dev->dev.parent,
> >> &backend_domid)) {
> >>                   xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
> >>                   return true;
> >> (END)
> >>
> >>
> >>
> >> If so, that commit subject would need to be updated accordingly.
> >>
> >> Let's see what other reviewers will say.
> > 
> > This doesn't work in all cases. Imagine using PCI Passthrough to assign
> > a "physical" virtio device to a domU. The domU will run into the same
> > error, right?
> > 
> > The problem is that we need a way for the virtio backend to advertise
> > its ability of handling grants. Right now we only have a way to do with
> > that with device tree on ARM. On x86, we only have
> > CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
> > CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
> > "physical" virtio devices. Note that in this case we are fixing a
> > nested-virtualization bug, but there are actually physical
> > virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
> > break those too.
> 
> 
> If these "physical" virtio devices are also spawned by
> drivers/virtio/virtio.c:virtio_dev_probe(), then yes, otherwise I don't 
> see how this could even be possible, but I might miss something here.

Yes, I would imagine virtio_dev_probe() would be called for them too



> xen_virtio_restricted_mem_acc() gets called indirectly from 
> virtio_dev_probe()->virtio_features_ok()->
> virtio_check_mem_acc_cb(). So the Xen grant DMA ops are only installed 
> for those.
>
>
> > 
> > I think we need to add a second way? It could be anything that can help
> > us distinguish between a non-grants-capable virtio backend and a
> > grants-capable virtio backend, such as:
> > - a string on xenstore
> > - a xen param
> > - a special PCI configuration register value
> > - something in the ACPI tables
> > - the QEMU machine type
> 
> 
> Yes, I remember there was a discussion regarding that. The point is to 
> choose a solution to be functional for both PV and HVM *and* to be able 
> to support a hotplug. IIRC, the xenstore could be a possible candidate.

xenstore would be among the easiest to make work. The only downside is
the dependency on xenstore which otherwise virtio+grants doesn't have.

Vikram is working on virtio with grants support in QEMU as we speak.
Maybe we could find a way to add a flag in QEMU so that we can detect at
runtime if a given virtio device support grants or not.

 
> > Or at least should we change CONFIG_XEN_VIRTIO_FORCE_GRANT into a
> > command line parameter so that it can be disabled in cases like this
> > one?
> 
> IIUC, this will help with HVM only.

For sure this is the least attractive solution, only marginally better
than the fix proposed in this patch
 

> > I realize that fixing this problem properly takes a lot longer than
> > adding a trivial if (dom0) return; check in the code. If you cannot find
> > a good way to solve the problem or you don't have time to do that now
> > and you need this bug fixed quickly, then I would be OK with the if
> > (dom0) return; check but please add a detailed TODO in-code comment to
> > explain that this is just a hack and we are still looking for a real
> > solution.
> > 
> > The check itself I prefer the original position because I want to retain
> > the ability of using virtio frontends with grant on ARM in Dom0 (DomD
> > case).
> 
> Makes sense, agree.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 8622 bytes --]

On 30.06.23 00:44, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
>> On 29.06.23 04:00, Stefano Stabellini wrote:
>>
>> Hello Stefano
>>
>>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>>
>>>>
>>>> Hello Petr
>>>>
>>>>
>>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>>>> eventually results in a hang during the boot.
>>>>>
>>>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>>>> makes no progress:
>>>>>
>>>>>      #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>>>      #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>>>>>      #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>>>>>      #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>>>      #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>>>      #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>>>      #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>>      #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>>>>>      #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>>      #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>>      #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>>>>      #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>          fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>>>>>      #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>>>>>      #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>>>>>      #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>>>>>      #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>>>>>      #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>>>>>      #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>>>>>      #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>>>>>      #19 do_initcalls () at ../init/main.c:1335
>>>>>      #20 do_basic_setup () at ../init/main.c:1354
>>>>>      #21 kernel_init_freeable () at ../init/main.c:1571
>>>>>      #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>>>>>      #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>>>>>      #24 0x0000000000000000 in ?? ()
>>>>>
>>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>>> setting dom0 as a backend when running in dom0.
>>>>>
>>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
>>>>
>>>>
>>>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>>>> am not mistaken, the said commit just moves the code in the context
>>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>>> introduced before.
>>>>
>>>>
>>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>>>> ---
>>>>>     drivers/xen/grant-dma-ops.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>>> --- a/drivers/xen/grant-dma-ops.c
>>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>>>>>     	if (np) {
>>>>>     		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>>>>>     		of_node_put(np);
>>>>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
>>>>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>> +		    xen_pv_domain()) &&
>>>>> +		   !xen_initial_domain()) {
>>>>
>>>> The commit lgtm, just one note:
>>>>
>>>>
>>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>>>> as I assume the same issue could happen on Arm with DT (although there
>>>> we don't guess the backend's domid, we read it from DT and quite
>>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>>
>>>> Something like:
>>>>
>>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>>> virtio_device *dev)
>>>>     {
>>>>            domid_t backend_domid;
>>>>
>>>> +       /* Xen grant DMA ops are not used when running as initial domain */
>>>> +       if (xen_initial_domain())
>>>> +               return false;
>>>> +
>>>>            if (!xen_grant_init_backend_domid(dev->dev.parent,
>>>> &backend_domid)) {
>>>>                    xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>>>>                    return true;
>>>> (END)
>>>>
>>>>
>>>>
>>>> If so, that commit subject would need to be updated accordingly.
>>>>
>>>> Let's see what other reviewers will say.
>>>
>>> This doesn't work in all cases. Imagine using PCI Passthrough to assign
>>> a "physical" virtio device to a domU. The domU will run into the same
>>> error, right?
>>>
>>> The problem is that we need a way for the virtio backend to advertise
>>> its ability of handling grants. Right now we only have a way to do with
>>> that with device tree on ARM. On x86, we only have
>>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
>>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
>>> "physical" virtio devices. Note that in this case we are fixing a
>>> nested-virtualization bug, but there are actually physical
>>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
>>> break those too.
>>
>>
>> If these "physical" virtio devices are also spawned by
>> drivers/virtio/virtio.c:virtio_dev_probe(), then yes, otherwise I don't
>> see how this could even be possible, but I might miss something here.
> 
> Yes, I would imagine virtio_dev_probe() would be called for them too
> 
> 
> 
>> xen_virtio_restricted_mem_acc() gets called indirectly from
>> virtio_dev_probe()->virtio_features_ok()->
>> virtio_check_mem_acc_cb(). So the Xen grant DMA ops are only installed
>> for those.
>>
>>
>>>
>>> I think we need to add a second way? It could be anything that can help
>>> us distinguish between a non-grants-capable virtio backend and a
>>> grants-capable virtio backend, such as:
>>> - a string on xenstore
>>> - a xen param
>>> - a special PCI configuration register value
>>> - something in the ACPI tables
>>> - the QEMU machine type
>>
>>
>> Yes, I remember there was a discussion regarding that. The point is to
>> choose a solution to be functional for both PV and HVM *and* to be able
>> to support a hotplug. IIRC, the xenstore could be a possible candidate.
> 
> xenstore would be among the easiest to make work. The only downside is
> the dependency on xenstore which otherwise virtio+grants doesn't have.

I'm in favor of using xenstore. Especially the hotplug support would be
much easier using xenstore (the alternative would be ACPI, which I don't
think we want for PV guests or many Arm configurations).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > I think we need to add a second way? It could be anything that can help
> > > us distinguish between a non-grants-capable virtio backend and a
> > > grants-capable virtio backend, such as:
> > > - a string on xenstore
> > > - a xen param
> > > - a special PCI configuration register value
> > > - something in the ACPI tables
> > > - the QEMU machine type
> > 
> > 
> > Yes, I remember there was a discussion regarding that. The point is to 
> > choose a solution to be functional for both PV and HVM *and* to be able 
> > to support a hotplug. IIRC, the xenstore could be a possible candidate.
> 
> xenstore would be among the easiest to make work. The only downside is
> the dependency on xenstore which otherwise virtio+grants doesn't have.

I would avoid introducing a dependency on xenstore, if nothing else we
know it's a performance bottleneck.

We would also need to map the virtio device topology into xenstore, so
that we can pass different options for each device.

> Vikram is working on virtio with grants support in QEMU as we speak.
> Maybe we could find a way to add a flag in QEMU so that we can detect at
> runtime if a given virtio device support grants or not.

Isn't there a way for the device to expose capabilities already?  For
example how does a virtio-blk backend expose support for indirect
descriptors?

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2528 bytes --]

On 04.07.23 09:48, Roger Pau Monné wrote:
> On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
>> On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
>>> On 29.06.23 04:00, Stefano Stabellini wrote:
>>>> I think we need to add a second way? It could be anything that can help
>>>> us distinguish between a non-grants-capable virtio backend and a
>>>> grants-capable virtio backend, such as:
>>>> - a string on xenstore
>>>> - a xen param
>>>> - a special PCI configuration register value
>>>> - something in the ACPI tables
>>>> - the QEMU machine type
>>>
>>>
>>> Yes, I remember there was a discussion regarding that. The point is to
>>> choose a solution to be functional for both PV and HVM *and* to be able
>>> to support a hotplug. IIRC, the xenstore could be a possible candidate.
>>
>> xenstore would be among the easiest to make work. The only downside is
>> the dependency on xenstore which otherwise virtio+grants doesn't have.
> 
> I would avoid introducing a dependency on xenstore, if nothing else we
> know it's a performance bottleneck.
> 
> We would also need to map the virtio device topology into xenstore, so
> that we can pass different options for each device.

This aspect (different options) is important. How do you want to pass virtio
device configuration parameters from dom0 to the virtio backend domain? You
probably need something like Xenstore (a virtio based alternative like virtiofs
would work, too) for that purpose.

Mapping the topology should be rather easy via the PCI-Id, e.g.:

/local/domain/42/device/virtio/0000:00:1c.0/backend

> 
>> Vikram is working on virtio with grants support in QEMU as we speak.
>> Maybe we could find a way to add a flag in QEMU so that we can detect at
>> runtime if a given virtio device support grants or not.
> 
> Isn't there a way for the device to expose capabilities already?  For
> example how does a virtio-blk backend expose support for indirect
> descriptors?

Those capabilities are defined in the virtio spec [1]. Adding the backend
domid would be possible, but it probably wouldn't be that easy (requires
changing the virtio spec by either expanding an existing config area or by
adding a new one). I'm not sure handling in the specific frontends is
generic enough for being able to have a central place where the backend
domid could be retrieved, without requiring any change of the frontends.


Juergen

[1]: http://docs.oasis-open.org/virtio/virtio/v1.2/virtio-v1.2.html

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]

Hi,

FWIW, I have ran into this issue some time ago too. I run Xen on top of
KVM and then passthrough some of the virtio devices (network one
specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
domU one. As a temporary workaround I needed to disable
CONFIG_XEN_VIRTIO completely (just disabling
CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
With that context in place, the actual response below.

On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
> On 04.07.23 09:48, Roger Pau Monné wrote:
> > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > > > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > > > I think we need to add a second way? It could be anything that can help
> > > > > us distinguish between a non-grants-capable virtio backend and a
> > > > > grants-capable virtio backend, such as:
> > > > > - a string on xenstore
> > > > > - a xen param
> > > > > - a special PCI configuration register value
> > > > > - something in the ACPI tables
> > > > > - the QEMU machine type
> > > > 
> > > > 
> > > > Yes, I remember there was a discussion regarding that. The point is to
> > > > choose a solution to be functional for both PV and HVM *and* to be able
> > > > to support a hotplug. IIRC, the xenstore could be a possible candidate.
> > > 
> > > xenstore would be among the easiest to make work. The only downside is
> > > the dependency on xenstore which otherwise virtio+grants doesn't have.
> > 
> > I would avoid introducing a dependency on xenstore, if nothing else we
> > know it's a performance bottleneck.
> > 
> > We would also need to map the virtio device topology into xenstore, so
> > that we can pass different options for each device.
> 
> This aspect (different options) is important. How do you want to pass virtio
> device configuration parameters from dom0 to the virtio backend domain? You
> probably need something like Xenstore (a virtio based alternative like virtiofs
> would work, too) for that purpose.
> 
> Mapping the topology should be rather easy via the PCI-Id, e.g.:
> 
> /local/domain/42/device/virtio/0000:00:1c.0/backend

While I agree this would probably be the simplest to implement, I don't
like introducing xenstore dependency into virtio frontend either.
Toolstack -> backend communication is probably easier to solve, as it's
much more flexible (could use qemu cmdline, QMP, other similar
mechanisms for non-qemu backends etc).

> > > Vikram is working on virtio with grants support in QEMU as we speak.
> > > Maybe we could find a way to add a flag in QEMU so that we can detect at
> > > runtime if a given virtio device support grants or not.
> > 
> > Isn't there a way for the device to expose capabilities already?  For
> > example how does a virtio-blk backend expose support for indirect
> > descriptors?
> 
> Those capabilities are defined in the virtio spec [1]. Adding the backend
> domid would be possible, but it probably wouldn't be that easy (requires
> changing the virtio spec by either expanding an existing config area or by
> adding a new one). I'm not sure handling in the specific frontends is
> generic enough for being able to have a central place where the backend
> domid could be retrieved, without requiring any change of the frontends.

IMHO the proper solution is to extend the spec. I don't have much
experience with virtio code, but reading the spec it looks like new
config area will be better for compatibility/uniform handling in a
frontent-agnostic way. Since it will definitely take time, some
transitional solution (maybe even xenstore...) might be warranted.

> Juergen
> 
> [1]: http://docs.oasis-open.org/virtio/virtio/v1.2/virtio-v1.2.html






-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Tue, Jul 04, 2023 at 01:43:46PM +0200, Marek Marczykowski-Górecki wrote:
> Hi,
> 
> FWIW, I have ran into this issue some time ago too. I run Xen on top of
> KVM and then passthrough some of the virtio devices (network one
> specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
> domU one. As a temporary workaround I needed to disable
> CONFIG_XEN_VIRTIO completely (just disabling
> CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
> With that context in place, the actual response below.
> 
> On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
> > On 04.07.23 09:48, Roger Pau Monné wrote:
> > > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> > > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > > > > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > > > > I think we need to add a second way? It could be anything that can help
> > > > > > us distinguish between a non-grants-capable virtio backend and a
> > > > > > grants-capable virtio backend, such as:
> > > > > > - a string on xenstore
> > > > > > - a xen param
> > > > > > - a special PCI configuration register value
> > > > > > - something in the ACPI tables
> > > > > > - the QEMU machine type
> > > > > 
> > > > > 
> > > > > Yes, I remember there was a discussion regarding that. The point is to
> > > > > choose a solution to be functional for both PV and HVM *and* to be able
> > > > > to support a hotplug. IIRC, the xenstore could be a possible candidate.
> > > > 
> > > > xenstore would be among the easiest to make work. The only downside is
> > > > the dependency on xenstore which otherwise virtio+grants doesn't have.
> > > 
> > > I would avoid introducing a dependency on xenstore, if nothing else we
> > > know it's a performance bottleneck.
> > > 
> > > We would also need to map the virtio device topology into xenstore, so
> > > that we can pass different options for each device.
> > 
> > This aspect (different options) is important. How do you want to pass virtio
> > device configuration parameters from dom0 to the virtio backend domain? You
> > probably need something like Xenstore (a virtio based alternative like virtiofs
> > would work, too) for that purpose.
> > 
> > Mapping the topology should be rather easy via the PCI-Id, e.g.:
> > 
> > /local/domain/42/device/virtio/0000:00:1c.0/backend
> 
> While I agree this would probably be the simplest to implement, I don't
> like introducing xenstore dependency into virtio frontend either.
> Toolstack -> backend communication is probably easier to solve, as it's
> much more flexible (could use qemu cmdline, QMP, other similar
> mechanisms for non-qemu backends etc).

I also think features should be exposed uniformly for devices, it's at
least weird to have certain features exposed in the PCI config space
while other features exposed in xenstore.

For virtio-mmio this might get a bit confusing, are we going to add
xenstore entries based on the position of the device config mmio
region?

I think on Arm PCI enumeration is not (usually?) done by the firmware,
at which point the SBDF expected by the tools/backend might be
different than the value assigned by the guest OS.

I think there are two slightly different issues, one is how to pass
information to virtio backends, I think doing this initially based on
xenstore is not that bad, because it's an internal detail of the
backend implementation. However passing information to virtio
frontends using xenstore is IMO a bad idea, there's already a way to
negotiate features between virtio frontends and backends, and Xen
should just expand and use that.


> > > > Vikram is working on virtio with grants support in QEMU as we speak.
> > > > Maybe we could find a way to add a flag in QEMU so that we can detect at
> > > > runtime if a given virtio device support grants or not.
> > > 
> > > Isn't there a way for the device to expose capabilities already?  For
> > > example how does a virtio-blk backend expose support for indirect
> > > descriptors?
> > 
> > Those capabilities are defined in the virtio spec [1]. Adding the backend
> > domid would be possible, but it probably wouldn't be that easy (requires
> > changing the virtio spec by either expanding an existing config area or by
> > adding a new one). I'm not sure handling in the specific frontends is
> > generic enough for being able to have a central place where the backend
> > domid could be retrieved, without requiring any change of the frontends.
> 
> IMHO the proper solution is to extend the spec. I don't have much
> experience with virtio code, but reading the spec it looks like new
> config area will be better for compatibility/uniform handling in a
> frontent-agnostic way. Since it will definitely take time, some
> transitional solution (maybe even xenstore...) might be warranted.

My fear is that such 'transitional' solutions end up being definitive
ones, as people move to other stuff once things work.  Might be better
to implement a transitional approach based on a future virtio
specification change rather than something out of band based on a
completely different communication channel.

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

[-- Attachment #1: Type: text/plain, Size: 6302 bytes --]

On Tue, Jul 4, 2023 at 5:49 PM Roger Pau Monné <roger.pau@citrix.com> wrote:

Hello all.

[sorry for the possible format issues]


On Tue, Jul 04, 2023 at 01:43:46PM +0200, Marek Marczykowski-Górecki wrote:
> > Hi,
> >
> > FWIW, I have ran into this issue some time ago too. I run Xen on top of
> > KVM and then passthrough some of the virtio devices (network one
> > specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
> > domU one. As a temporary workaround I needed to disable
> > CONFIG_XEN_VIRTIO completely (just disabling
> > CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
> > With that context in place, the actual response below.
> >
> > On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
> > > On 04.07.23 09:48, Roger Pau Monné wrote:
> > > > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> > > > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > > > > > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > > > > > I think we need to add a second way? It could be anything that
> can help
> > > > > > > us distinguish between a non-grants-capable virtio backend and
> a
> > > > > > > grants-capable virtio backend, such as:
> > > > > > > - a string on xenstore
> > > > > > > - a xen param
> > > > > > > - a special PCI configuration register value
> > > > > > > - something in the ACPI tables
> > > > > > > - the QEMU machine type
> > > > > >
> > > > > >
> > > > > > Yes, I remember there was a discussion regarding that. The point
> is to
> > > > > > choose a solution to be functional for both PV and HVM *and* to
> be able
> > > > > > to support a hotplug. IIRC, the xenstore could be a possible
> candidate.
> > > > >
> > > > > xenstore would be among the easiest to make work. The only
> downside is
> > > > > the dependency on xenstore which otherwise virtio+grants doesn't
> have.
> > > >
> > > > I would avoid introducing a dependency on xenstore, if nothing else
> we
> > > > know it's a performance bottleneck.
> > > >
> > > > We would also need to map the virtio device topology into xenstore,
> so
> > > > that we can pass different options for each device.
> > >
> > > This aspect (different options) is important. How do you want to pass
> virtio
> > > device configuration parameters from dom0 to the virtio backend
> domain? You
> > > probably need something like Xenstore (a virtio based alternative like
> virtiofs
> > > would work, too) for that purpose.
> > >
> > > Mapping the topology should be rather easy via the PCI-Id, e.g.:
> > >
> > > /local/domain/42/device/virtio/0000:00:1c.0/backend
> >
> > While I agree this would probably be the simplest to implement, I don't
> > like introducing xenstore dependency into virtio frontend either.
> > Toolstack -> backend communication is probably easier to solve, as it's
> > much more flexible (could use qemu cmdline, QMP, other similar
> > mechanisms for non-qemu backends etc).
>
> I also think features should be exposed uniformly for devices, it's at
> least weird to have certain features exposed in the PCI config space
> while other features exposed in xenstore.
>
> For virtio-mmio this might get a bit confusing, are we going to add
> xenstore entries based on the position of the device config mmio
> region?
>
> I think on Arm PCI enumeration is not (usually?) done by the firmware,
> at which point the SBDF expected by the tools/backend might be
> different than the value assigned by the guest OS.
>
> I think there are two slightly different issues, one is how to pass
> information to virtio backends, I think doing this initially based on
> xenstore is not that bad, because it's an internal detail of the
> backend implementation. However passing information to virtio
> frontends using xenstore is IMO a bad idea, there's already a way to
> negotiate features between virtio frontends and backends, and Xen
> should just expand and use that.
>
>

On Arm with device-tree we have a special bindings which purpose is to
inform us whether we need to use grants for virtio and backend domid for a
particular device.Here on x86, we don't have a device tree, so cannot
(easily?) reuse this logic.

I have just recollected one idea suggested by Stefano some time ago [1].
The context of discussion was about what to do when device-tree and ACPI
cannot be reused (or something like that).The idea won't cover virtio-mmio,
but I have heard that virtio-mmio usage with x86 Xen is rather unusual case.

I will paste the text below for convenience.

**********

Part 1 (intro):

We could reuse a PCI config space register to expose the backend id.
However this solution requires a backend change (QEMU) to expose the
backend id via an emulated register for each emulated device.

To avoid having to introduce a special config space register in all
emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
could add a special PCI config space register at the emulated PCI Root
Complex level.

Basically the workflow would be as follow:

- Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
- Linux writes to special PCI config space register of the Xen PCI Root
  Complex the PCI device id (basically the BDF)
- The Xen PCI Root Complex emulated by Xen answers by writing back to
  the same location the backend id (domid of the backend)
- Linux reads back the same PCI config space register of the Xen PCI
  Root Complex and learn the relevant domid

Part 2 (clarification):

I think using a special config space register in the root complex would
not be terrible in terms of guest changes because it is easy to
introduce a new root complex driver in Linux and other OSes. The root
complex would still be ECAM compatible so the regular ECAM driver would
still work. A new driver would only be necessary if you want to be able
to access the special config space register.


**********
What do you think about it? Are there any pitfalls, etc? This also requires
system changes, but at least without virtio spec changes.

[1]
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2210061747590.3690179@ubuntu-linux-20-04-desktop/


-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #2: Type: text/html, Size: 8246 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 7059 bytes --]

On 04.07.23 19:14, Oleksandr Tyshchenko wrote:
> 
> 
> On Tue, Jul 4, 2023 at 5:49 PM Roger Pau Monné <roger.pau@citrix.com 
> <mailto:roger.pau@citrix.com>> wrote:
> 
> Hello all.
> 
> [sorry for the possible format issues]
> 
> 
>     On Tue, Jul 04, 2023 at 01:43:46PM +0200, Marek Marczykowski-Górecki wrote:
>      > Hi,
>      >
>      > FWIW, I have ran into this issue some time ago too. I run Xen on top of
>      > KVM and then passthrough some of the virtio devices (network one
>      > specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
>      > domU one. As a temporary workaround I needed to disable
>      > CONFIG_XEN_VIRTIO completely (just disabling
>      > CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
>      > With that context in place, the actual response below.
>      >
>      > On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
>      > > On 04.07.23 09:48, Roger Pau Monné wrote:
>      > > > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
>      > > > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
>      > > > > > On 29.06.23 04:00, Stefano Stabellini wrote:
>      > > > > > > I think we need to add a second way? It could be anything that
>     can help
>      > > > > > > us distinguish between a non-grants-capable virtio backend and a
>      > > > > > > grants-capable virtio backend, such as:
>      > > > > > > - a string on xenstore
>      > > > > > > - a xen param
>      > > > > > > - a special PCI configuration register value
>      > > > > > > - something in the ACPI tables
>      > > > > > > - the QEMU machine type
>      > > > > >
>      > > > > >
>      > > > > > Yes, I remember there was a discussion regarding that. The point
>     is to
>      > > > > > choose a solution to be functional for both PV and HVM *and* to
>     be able
>      > > > > > to support a hotplug. IIRC, the xenstore could be a possible
>     candidate.
>      > > > >
>      > > > > xenstore would be among the easiest to make work. The only downside is
>      > > > > the dependency on xenstore which otherwise virtio+grants doesn't have.
>      > > >
>      > > > I would avoid introducing a dependency on xenstore, if nothing else we
>      > > > know it's a performance bottleneck.
>      > > >
>      > > > We would also need to map the virtio device topology into xenstore, so
>      > > > that we can pass different options for each device.
>      > >
>      > > This aspect (different options) is important. How do you want to pass
>     virtio
>      > > device configuration parameters from dom0 to the virtio backend domain? You
>      > > probably need something like Xenstore (a virtio based alternative like
>     virtiofs
>      > > would work, too) for that purpose.
>      > >
>      > > Mapping the topology should be rather easy via the PCI-Id, e.g.:
>      > >
>      > > /local/domain/42/device/virtio/0000:00:1c.0/backend
>      >
>      > While I agree this would probably be the simplest to implement, I don't
>      > like introducing xenstore dependency into virtio frontend either.
>      > Toolstack -> backend communication is probably easier to solve, as it's
>      > much more flexible (could use qemu cmdline, QMP, other similar
>      > mechanisms for non-qemu backends etc).
> 
>     I also think features should be exposed uniformly for devices, it's at
>     least weird to have certain features exposed in the PCI config space
>     while other features exposed in xenstore.
> 
>     For virtio-mmio this might get a bit confusing, are we going to add
>     xenstore entries based on the position of the device config mmio
>     region?
> 
>     I think on Arm PCI enumeration is not (usually?) done by the firmware,
>     at which point the SBDF expected by the tools/backend might be
>     different than the value assigned by the guest OS.
> 
>     I think there are two slightly different issues, one is how to pass
>     information to virtio backends, I think doing this initially based on
>     xenstore is not that bad, because it's an internal detail of the
>     backend implementation. However passing information to virtio
>     frontends using xenstore is IMO a bad idea, there's already a way to
>     negotiate features between virtio frontends and backends, and Xen
>     should just expand and use that.
> 
> 
> 
> On Arm with device-tree we have a special bindings which purpose is to inform us 
> whether we need to use grants for virtio and backend domid for a particular 
> device.Here on x86, we don't have a device tree, so cannot (easily?) reuse this 
> logic.
> 
> I have just recollected one idea suggested by Stefano some time ago [1]. The 
> context of discussion was about what to do when device-tree and ACPI cannot be 
> reused (or something like that).The idea won't cover virtio-mmio, but I have 
> heard that virtio-mmio usage with x86 Xen is rather unusual case.
> 
> I will paste the text below for convenience.
> 
> **********
> 
> Part 1 (intro):
> 
> We could reuse a PCI config space register to expose the backend id.
> However this solution requires a backend change (QEMU) to expose the
> backend id via an emulated register for each emulated device.
> 
> To avoid having to introduce a special config space register in all
> emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
> could add a special PCI config space register at the emulated PCI Root
> Complex level.
> 
> Basically the workflow would be as follow:
> 
> - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
> - Linux writes to special PCI config space register of the Xen PCI Root
>    Complex the PCI device id (basically the BDF)
> - The Xen PCI Root Complex emulated by Xen answers by writing back to
>    the same location the backend id (domid of the backend)
> - Linux reads back the same PCI config space register of the Xen PCI
>    Root Complex and learn the relevant domid
> 
> Part 2 (clarification):
> 
> I think using a special config space register in the root complex would
> not be terrible in terms of guest changes because it is easy to
> introduce a new root complex driver in Linux and other OSes. The root
> complex would still be ECAM compatible so the regular ECAM driver would
> still work. A new driver would only be necessary if you want to be able
> to access the special config space register.
> 
> 
> **********
> What do you think about it? Are there any pitfalls, etc? This also requires 
> system changes, but at least without virtio spec changes.
> 
> [1] 
> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2210061747590.3690179@ubuntu-linux-20-04-desktop/ <https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2210061747590.3690179@ubuntu-linux-20-04-desktop/>

Sounds like a good idea. There would be one PCI root per backend domain needed,
but that should be possible.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> On Tue, Jul 4, 2023 at 5:49 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> Hello all.
> 
> [sorry for the possible format issues]
> 
> 
> On Tue, Jul 04, 2023 at 01:43:46PM +0200, Marek Marczykowski-Górecki wrote:
> > > Hi,
> > >
> > > FWIW, I have ran into this issue some time ago too. I run Xen on top of
> > > KVM and then passthrough some of the virtio devices (network one
> > > specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
> > > domU one. As a temporary workaround I needed to disable
> > > CONFIG_XEN_VIRTIO completely (just disabling
> > > CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
> > > With that context in place, the actual response below.
> > >
> > > On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
> > > > On 04.07.23 09:48, Roger Pau Monné wrote:
> > > > > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> > > > > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > > > > > > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > > > > > > I think we need to add a second way? It could be anything that
> > can help
> > > > > > > > us distinguish between a non-grants-capable virtio backend and
> > a
> > > > > > > > grants-capable virtio backend, such as:
> > > > > > > > - a string on xenstore
> > > > > > > > - a xen param
> > > > > > > > - a special PCI configuration register value
> > > > > > > > - something in the ACPI tables
> > > > > > > > - the QEMU machine type
> > > > > > >
> > > > > > >
> > > > > > > Yes, I remember there was a discussion regarding that. The point
> > is to
> > > > > > > choose a solution to be functional for both PV and HVM *and* to
> > be able
> > > > > > > to support a hotplug. IIRC, the xenstore could be a possible
> > candidate.
> > > > > >
> > > > > > xenstore would be among the easiest to make work. The only
> > downside is
> > > > > > the dependency on xenstore which otherwise virtio+grants doesn't
> > have.
> > > > >
> > > > > I would avoid introducing a dependency on xenstore, if nothing else
> > we
> > > > > know it's a performance bottleneck.
> > > > >
> > > > > We would also need to map the virtio device topology into xenstore,
> > so
> > > > > that we can pass different options for each device.
> > > >
> > > > This aspect (different options) is important. How do you want to pass
> > virtio
> > > > device configuration parameters from dom0 to the virtio backend
> > domain? You
> > > > probably need something like Xenstore (a virtio based alternative like
> > virtiofs
> > > > would work, too) for that purpose.
> > > >
> > > > Mapping the topology should be rather easy via the PCI-Id, e.g.:
> > > >
> > > > /local/domain/42/device/virtio/0000:00:1c.0/backend
> > >
> > > While I agree this would probably be the simplest to implement, I don't
> > > like introducing xenstore dependency into virtio frontend either.
> > > Toolstack -> backend communication is probably easier to solve, as it's
> > > much more flexible (could use qemu cmdline, QMP, other similar
> > > mechanisms for non-qemu backends etc).
> >
> > I also think features should be exposed uniformly for devices, it's at
> > least weird to have certain features exposed in the PCI config space
> > while other features exposed in xenstore.
> >
> > For virtio-mmio this might get a bit confusing, are we going to add
> > xenstore entries based on the position of the device config mmio
> > region?
> >
> > I think on Arm PCI enumeration is not (usually?) done by the firmware,
> > at which point the SBDF expected by the tools/backend might be
> > different than the value assigned by the guest OS.
> >
> > I think there are two slightly different issues, one is how to pass
> > information to virtio backends, I think doing this initially based on
> > xenstore is not that bad, because it's an internal detail of the
> > backend implementation. However passing information to virtio
> > frontends using xenstore is IMO a bad idea, there's already a way to
> > negotiate features between virtio frontends and backends, and Xen
> > should just expand and use that.
> >
> >
> 
> On Arm with device-tree we have a special bindings which purpose is to
> inform us whether we need to use grants for virtio and backend domid for a
> particular device.Here on x86, we don't have a device tree, so cannot
> (easily?) reuse this logic.
> 
> I have just recollected one idea suggested by Stefano some time ago [1].
> The context of discussion was about what to do when device-tree and ACPI
> cannot be reused (or something like that).The idea won't cover virtio-mmio,
> but I have heard that virtio-mmio usage with x86 Xen is rather unusual case.
> 
> I will paste the text below for convenience.
> 
> **********
> 
> Part 1 (intro):
> 
> We could reuse a PCI config space register to expose the backend id.
> However this solution requires a backend change (QEMU) to expose the
> backend id via an emulated register for each emulated device.
> 
> To avoid having to introduce a special config space register in all
> emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
> could add a special PCI config space register at the emulated PCI Root
> Complex level.
> 
> Basically the workflow would be as follow:
> 
> - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
> - Linux writes to special PCI config space register of the Xen PCI Root
>   Complex the PCI device id (basically the BDF)
> - The Xen PCI Root Complex emulated by Xen answers by writing back to
>   the same location the backend id (domid of the backend)
> - Linux reads back the same PCI config space register of the Xen PCI
>   Root Complex and learn the relevant domid

IMO this seems awfully complex.  I'm not familiar with the VirtIO
spec, but I see there's a Vendor data capability, could we possibly
expose Xen-specific information on that capability?

> Part 2 (clarification):
> 
> I think using a special config space register in the root complex would
> not be terrible in terms of guest changes because it is easy to
> introduce a new root complex driver in Linux and other OSes. The root
> complex would still be ECAM compatible so the regular ECAM driver would
> still work. A new driver would only be necessary if you want to be able
> to access the special config space register.

I'm slightly worry of this approach, we end up modifying a root
complex emulation in order to avoid modifying a PCI device emulation
on QEMU, not sure that's a good trade off.

Note also that different architectures will likely have different root
complex, and so you might need to modify several of them, plus then
arrange the PCI layout correctly in order to have the proper hierarchy
so that devices belonging to different driver domains are assigned to
different bridges.

> 
> 
> **********
> What do you think about it? Are there any pitfalls, etc? This also requires
> system changes, but at least without virtio spec changes.

Why are we so reluctant to add spec changes?  I understand this might
take time an effort, but it's the only way IMO to build a sustainable
VirtIO Xen implementation.  Did we already attempt to negotiate with
Oasis Xen related spec changes and those where refused?

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8894 bytes --]

On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > On Tue, Jul 4, 2023 at 5:49 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > 
> > Hello all.
> > 
> > [sorry for the possible format issues]
> > 
> > 
> > On Tue, Jul 04, 2023 at 01:43:46PM +0200, Marek Marczykowski-Górecki wrote:
> > > > Hi,
> > > >
> > > > FWIW, I have ran into this issue some time ago too. I run Xen on top of
> > > > KVM and then passthrough some of the virtio devices (network one
> > > > specifically) into a (PV) guest. So, I hit both cases, the dom0 one and
> > > > domU one. As a temporary workaround I needed to disable
> > > > CONFIG_XEN_VIRTIO completely (just disabling
> > > > CONFIG_XEN_VIRTIO_FORCE_GRANT was not enough to fix it).
> > > > With that context in place, the actual response below.
> > > >
> > > > On Tue, Jul 04, 2023 at 12:39:40PM +0200, Juergen Gross wrote:
> > > > > On 04.07.23 09:48, Roger Pau Monné wrote:
> > > > > > On Thu, Jun 29, 2023 at 03:44:04PM -0700, Stefano Stabellini wrote:
> > > > > > > On Thu, 29 Jun 2023, Oleksandr Tyshchenko wrote:
> > > > > > > > On 29.06.23 04:00, Stefano Stabellini wrote:
> > > > > > > > > I think we need to add a second way? It could be anything that
> > > can help
> > > > > > > > > us distinguish between a non-grants-capable virtio backend and
> > > a
> > > > > > > > > grants-capable virtio backend, such as:
> > > > > > > > > - a string on xenstore
> > > > > > > > > - a xen param
> > > > > > > > > - a special PCI configuration register value
> > > > > > > > > - something in the ACPI tables
> > > > > > > > > - the QEMU machine type
> > > > > > > >
> > > > > > > >
> > > > > > > > Yes, I remember there was a discussion regarding that. The point
> > > is to
> > > > > > > > choose a solution to be functional for both PV and HVM *and* to
> > > be able
> > > > > > > > to support a hotplug. IIRC, the xenstore could be a possible
> > > candidate.
> > > > > > >
> > > > > > > xenstore would be among the easiest to make work. The only
> > > downside is
> > > > > > > the dependency on xenstore which otherwise virtio+grants doesn't
> > > have.
> > > > > >
> > > > > > I would avoid introducing a dependency on xenstore, if nothing else
> > > we
> > > > > > know it's a performance bottleneck.
> > > > > >
> > > > > > We would also need to map the virtio device topology into xenstore,
> > > so
> > > > > > that we can pass different options for each device.
> > > > >
> > > > > This aspect (different options) is important. How do you want to pass
> > > virtio
> > > > > device configuration parameters from dom0 to the virtio backend
> > > domain? You
> > > > > probably need something like Xenstore (a virtio based alternative like
> > > virtiofs
> > > > > would work, too) for that purpose.
> > > > >
> > > > > Mapping the topology should be rather easy via the PCI-Id, e.g.:
> > > > >
> > > > > /local/domain/42/device/virtio/0000:00:1c.0/backend
> > > >
> > > > While I agree this would probably be the simplest to implement, I don't
> > > > like introducing xenstore dependency into virtio frontend either.
> > > > Toolstack -> backend communication is probably easier to solve, as it's
> > > > much more flexible (could use qemu cmdline, QMP, other similar
> > > > mechanisms for non-qemu backends etc).
> > >
> > > I also think features should be exposed uniformly for devices, it's at
> > > least weird to have certain features exposed in the PCI config space
> > > while other features exposed in xenstore.
> > >
> > > For virtio-mmio this might get a bit confusing, are we going to add
> > > xenstore entries based on the position of the device config mmio
> > > region?
> > >
> > > I think on Arm PCI enumeration is not (usually?) done by the firmware,
> > > at which point the SBDF expected by the tools/backend might be
> > > different than the value assigned by the guest OS.
> > >
> > > I think there are two slightly different issues, one is how to pass
> > > information to virtio backends, I think doing this initially based on
> > > xenstore is not that bad, because it's an internal detail of the
> > > backend implementation. However passing information to virtio
> > > frontends using xenstore is IMO a bad idea, there's already a way to
> > > negotiate features between virtio frontends and backends, and Xen
> > > should just expand and use that.
> > >
> > >
> > 
> > On Arm with device-tree we have a special bindings which purpose is to
> > inform us whether we need to use grants for virtio and backend domid for a
> > particular device.Here on x86, we don't have a device tree, so cannot
> > (easily?) reuse this logic.
> > 
> > I have just recollected one idea suggested by Stefano some time ago [1].
> > The context of discussion was about what to do when device-tree and ACPI
> > cannot be reused (or something like that).The idea won't cover virtio-mmio,
> > but I have heard that virtio-mmio usage with x86 Xen is rather unusual case.
> > 
> > I will paste the text below for convenience.

Thanks Oleksandr! I had forgotten about this, but in retrospect it was a
good suggestion :-D


> > **********
> > 
> > Part 1 (intro):
> > 
> > We could reuse a PCI config space register to expose the backend id.
> > However this solution requires a backend change (QEMU) to expose the
> > backend id via an emulated register for each emulated device.
> > 
> > To avoid having to introduce a special config space register in all
> > emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
> > could add a special PCI config space register at the emulated PCI Root
> > Complex level.
> > 
> > Basically the workflow would be as follow:
> > 
> > - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
> > - Linux writes to special PCI config space register of the Xen PCI Root
> >   Complex the PCI device id (basically the BDF)
> > - The Xen PCI Root Complex emulated by Xen answers by writing back to
> >   the same location the backend id (domid of the backend)
> > - Linux reads back the same PCI config space register of the Xen PCI
> >   Root Complex and learn the relevant domid
> 
> IMO this seems awfully complex.  I'm not familiar with the VirtIO
> spec, but I see there's a Vendor data capability, could we possibly
> expose Xen-specific information on that capability?

That is also a possibility too. Also we could use a PCI conf register
which is known to be unused in the Virtio spec to expose the grant
capability and backend domid.


> > Part 2 (clarification):
> > 
> > I think using a special config space register in the root complex would
> > not be terrible in terms of guest changes because it is easy to
> > introduce a new root complex driver in Linux and other OSes. The root
> > complex would still be ECAM compatible so the regular ECAM driver would
> > still work. A new driver would only be necessary if you want to be able
> > to access the special config space register.
> 
> I'm slightly worry of this approach, we end up modifying a root
> complex emulation in order to avoid modifying a PCI device emulation
> on QEMU, not sure that's a good trade off.
> 
> Note also that different architectures will likely have different root
> complex, and so you might need to modify several of them, plus then
> arrange the PCI layout correctly in order to have the proper hierarchy
> so that devices belonging to different driver domains are assigned to
> different bridges.

I do think that adding something to the PCI conf register somewhere is
the best option because it is not dependent on ACPI and it is not
dependent on xenstore both of which are very undesirable.

I am not sure where specifically is the best place. These are 3 ideas
we came up with:
1. PCI root complex
2. a register on the device itself
3. a new capability of the device
4. add one extra dummy PCI device for the sole purpose of exposing the
   grants capability


Looking at the spec, there is a way to add a vendor-specific capability
(cap_vndr = 0x9). Could we use that? It doesn't look like it is used
today, Linux doesn't parse it.


> > 
> > 
> > **********
> > What do you think about it? Are there any pitfalls, etc? This also requires
> > system changes, but at least without virtio spec changes.
> 
> Why are we so reluctant to add spec changes?  I understand this might
> take time an effort, but it's the only way IMO to build a sustainable
> VirtIO Xen implementation.  Did we already attempt to negotiate with
> Oasis Xen related spec changes and those where refused?

That's because spec changes can be very slow. This is a bug that we need
a relatively quick solution for and waiting 12-24 months for a spec
update is not realistic.

I think a spec change would be best as a long term solution. We also
need a short term solution. The short term solution doesn't have to be
ideal but it has to work now.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > **********
> > > 
> > > Part 1 (intro):
> > > 
> > > We could reuse a PCI config space register to expose the backend id.
> > > However this solution requires a backend change (QEMU) to expose the
> > > backend id via an emulated register for each emulated device.
> > > 
> > > To avoid having to introduce a special config space register in all
> > > emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
> > > could add a special PCI config space register at the emulated PCI Root
> > > Complex level.
> > > 
> > > Basically the workflow would be as follow:
> > > 
> > > - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
> > > - Linux writes to special PCI config space register of the Xen PCI Root
> > >   Complex the PCI device id (basically the BDF)
> > > - The Xen PCI Root Complex emulated by Xen answers by writing back to
> > >   the same location the backend id (domid of the backend)
> > > - Linux reads back the same PCI config space register of the Xen PCI
> > >   Root Complex and learn the relevant domid
> > 
> > IMO this seems awfully complex.  I'm not familiar with the VirtIO
> > spec, but I see there's a Vendor data capability, could we possibly
> > expose Xen-specific information on that capability?
> 
> That is also a possibility too. Also we could use a PCI conf register
> which is known to be unused in the Virtio spec to expose the grant
> capability and backend domid.

Capabilities don't have a fixed config space register, they are a
linked list, and so capabilities end up at different positions
depending on the specific device layout.  The only fixed part is the
range from [0, 0x3F), and that's fully defined in the specification.

Trying to define a fixed address for Xen use after the 3f boundary
seems like a bad idea, as it's going to be hard to make sure that such
address is not used on all possible devices.  IMO the only way is to
place such information in a capability, whether that's an existing
capability or a new one I don't really know.

> 
> > > Part 2 (clarification):
> > > 
> > > I think using a special config space register in the root complex would
> > > not be terrible in terms of guest changes because it is easy to
> > > introduce a new root complex driver in Linux and other OSes. The root
> > > complex would still be ECAM compatible so the regular ECAM driver would
> > > still work. A new driver would only be necessary if you want to be able
> > > to access the special config space register.
> > 
> > I'm slightly worry of this approach, we end up modifying a root
> > complex emulation in order to avoid modifying a PCI device emulation
> > on QEMU, not sure that's a good trade off.
> > 
> > Note also that different architectures will likely have different root
> > complex, and so you might need to modify several of them, plus then
> > arrange the PCI layout correctly in order to have the proper hierarchy
> > so that devices belonging to different driver domains are assigned to
> > different bridges.
> 
> I do think that adding something to the PCI conf register somewhere is
> the best option because it is not dependent on ACPI and it is not
> dependent on xenstore both of which are very undesirable.
> 
> I am not sure where specifically is the best place. These are 3 ideas
> we came up with:
> 1. PCI root complex
> 2. a register on the device itself
> 3. a new capability of the device
> 4. add one extra dummy PCI device for the sole purpose of exposing the
>    grants capability
> 
> 
> Looking at the spec, there is a way to add a vendor-specific capability
> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> today, Linux doesn't parse it.

I did wonder the same from a quick look at the spec.  There's however
a text in the specification that says:

"The driver SHOULD NOT use the Vendor data capability except for
debugging and reporting purposes."

So we would at least need to change that because the capability would
then be used by other purposes different than debugging and reporting.

Seems like a minor adjustment, so might we worth asking upstream about
their opinion, and to get a conversation started.

> 
> > > 
> > > 
> > > **********
> > > What do you think about it? Are there any pitfalls, etc? This also requires
> > > system changes, but at least without virtio spec changes.
> > 
> > Why are we so reluctant to add spec changes?  I understand this might
> > take time an effort, but it's the only way IMO to build a sustainable
> > VirtIO Xen implementation.  Did we already attempt to negotiate with
> > Oasis Xen related spec changes and those where refused?
> 
> That's because spec changes can be very slow. This is a bug that we need
> a relatively quick solution for and waiting 12-24 months for a spec
> update is not realistic.
> 
> I think a spec change would be best as a long term solution. We also
> need a short term solution. The short term solution doesn't have to be
> ideal but it has to work now.

My fear with such approach is that once a bodge is in place people
move on to other stuff and this never gets properly fixed.

I know this might not be a well received opinion, but it would be
better if such bodge is kept in each interested party patchqueue for
the time being, until a proper solution is implemented.  That way
there's an interest from parties into properly fixing it upstream.

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6095 bytes --]

On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > > **********
> > > > 
> > > > Part 1 (intro):
> > > > 
> > > > We could reuse a PCI config space register to expose the backend id.
> > > > However this solution requires a backend change (QEMU) to expose the
> > > > backend id via an emulated register for each emulated device.
> > > > 
> > > > To avoid having to introduce a special config space register in all
> > > > emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
> > > > could add a special PCI config space register at the emulated PCI Root
> > > > Complex level.
> > > > 
> > > > Basically the workflow would be as follow:
> > > > 
> > > > - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
> > > > - Linux writes to special PCI config space register of the Xen PCI Root
> > > >   Complex the PCI device id (basically the BDF)
> > > > - The Xen PCI Root Complex emulated by Xen answers by writing back to
> > > >   the same location the backend id (domid of the backend)
> > > > - Linux reads back the same PCI config space register of the Xen PCI
> > > >   Root Complex and learn the relevant domid
> > > 
> > > IMO this seems awfully complex.  I'm not familiar with the VirtIO
> > > spec, but I see there's a Vendor data capability, could we possibly
> > > expose Xen-specific information on that capability?
> > 
> > That is also a possibility too. Also we could use a PCI conf register
> > which is known to be unused in the Virtio spec to expose the grant
> > capability and backend domid.
> 
> Capabilities don't have a fixed config space register, they are a
> linked list, and so capabilities end up at different positions
> depending on the specific device layout.  The only fixed part is the
> range from [0, 0x3F), and that's fully defined in the specification.
> 
> Trying to define a fixed address for Xen use after the 3f boundary
> seems like a bad idea, as it's going to be hard to make sure that such
> address is not used on all possible devices.  IMO the only way is to
> place such information in a capability, whether that's an existing
> capability or a new one I don't really know.

That seems like a good idea
 
 
> > > > Part 2 (clarification):
> > > > 
> > > > I think using a special config space register in the root complex would
> > > > not be terrible in terms of guest changes because it is easy to
> > > > introduce a new root complex driver in Linux and other OSes. The root
> > > > complex would still be ECAM compatible so the regular ECAM driver would
> > > > still work. A new driver would only be necessary if you want to be able
> > > > to access the special config space register.
> > > 
> > > I'm slightly worry of this approach, we end up modifying a root
> > > complex emulation in order to avoid modifying a PCI device emulation
> > > on QEMU, not sure that's a good trade off.
> > > 
> > > Note also that different architectures will likely have different root
> > > complex, and so you might need to modify several of them, plus then
> > > arrange the PCI layout correctly in order to have the proper hierarchy
> > > so that devices belonging to different driver domains are assigned to
> > > different bridges.
> > 
> > I do think that adding something to the PCI conf register somewhere is
> > the best option because it is not dependent on ACPI and it is not
> > dependent on xenstore both of which are very undesirable.
> > 
> > I am not sure where specifically is the best place. These are 3 ideas
> > we came up with:
> > 1. PCI root complex
> > 2. a register on the device itself
> > 3. a new capability of the device
> > 4. add one extra dummy PCI device for the sole purpose of exposing the
> >    grants capability
> > 
> > 
> > Looking at the spec, there is a way to add a vendor-specific capability
> > (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> > today, Linux doesn't parse it.
> 
> I did wonder the same from a quick look at the spec.  There's however
> a text in the specification that says:
> 
> "The driver SHOULD NOT use the Vendor data capability except for
> debugging and reporting purposes."
> 
> So we would at least need to change that because the capability would
> then be used by other purposes different than debugging and reporting.
> 
> Seems like a minor adjustment, so might we worth asking upstream about
> their opinion, and to get a conversation started.

Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
we are doing, right?

 
> > > > 
> > > > 
> > > > **********
> > > > What do you think about it? Are there any pitfalls, etc? This also requires
> > > > system changes, but at least without virtio spec changes.
> > > 
> > > Why are we so reluctant to add spec changes?  I understand this might
> > > take time an effort, but it's the only way IMO to build a sustainable
> > > VirtIO Xen implementation.  Did we already attempt to negotiate with
> > > Oasis Xen related spec changes and those where refused?
> > 
> > That's because spec changes can be very slow. This is a bug that we need
> > a relatively quick solution for and waiting 12-24 months for a spec
> > update is not realistic.
> > 
> > I think a spec change would be best as a long term solution. We also
> > need a short term solution. The short term solution doesn't have to be
> > ideal but it has to work now.
> 
> My fear with such approach is that once a bodge is in place people
> move on to other stuff and this never gets properly fixed.
> 
> I know this might not be a well received opinion, but it would be
> better if such bodge is kept in each interested party patchqueue for
> the time being, until a proper solution is implemented.  That way
> there's an interest from parties into properly fixing it upstream.

Unfortunately we are in the situation where we have an outstanding
upstream bug, so we have to take action one way or the other.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 6938 bytes --]

On 06.07.23 23:49, Stefano Stabellini wrote:
> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>> **********
>>>>>
>>>>> Part 1 (intro):
>>>>>
>>>>> We could reuse a PCI config space register to expose the backend id.
>>>>> However this solution requires a backend change (QEMU) to expose the
>>>>> backend id via an emulated register for each emulated device.
>>>>>
>>>>> To avoid having to introduce a special config space register in all
>>>>> emulated PCI devices (virtio-net, virtio-block, etc) I wonder if we
>>>>> could add a special PCI config space register at the emulated PCI Root
>>>>> Complex level.
>>>>>
>>>>> Basically the workflow would be as follow:
>>>>>
>>>>> - Linux recognizes the PCI Root Complex as a Xen PCI Root Complex
>>>>> - Linux writes to special PCI config space register of the Xen PCI Root
>>>>>    Complex the PCI device id (basically the BDF)
>>>>> - The Xen PCI Root Complex emulated by Xen answers by writing back to
>>>>>    the same location the backend id (domid of the backend)
>>>>> - Linux reads back the same PCI config space register of the Xen PCI
>>>>>    Root Complex and learn the relevant domid
>>>>
>>>> IMO this seems awfully complex.  I'm not familiar with the VirtIO
>>>> spec, but I see there's a Vendor data capability, could we possibly
>>>> expose Xen-specific information on that capability?
>>>
>>> That is also a possibility too. Also we could use a PCI conf register
>>> which is known to be unused in the Virtio spec to expose the grant
>>> capability and backend domid.
>>
>> Capabilities don't have a fixed config space register, they are a
>> linked list, and so capabilities end up at different positions
>> depending on the specific device layout.  The only fixed part is the
>> range from [0, 0x3F), and that's fully defined in the specification.
>>
>> Trying to define a fixed address for Xen use after the 3f boundary
>> seems like a bad idea, as it's going to be hard to make sure that such
>> address is not used on all possible devices.  IMO the only way is to
>> place such information in a capability, whether that's an existing
>> capability or a new one I don't really know.
> 
> That seems like a good idea
>   
>   
>>>>> Part 2 (clarification):
>>>>>
>>>>> I think using a special config space register in the root complex would
>>>>> not be terrible in terms of guest changes because it is easy to
>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>> still work. A new driver would only be necessary if you want to be able
>>>>> to access the special config space register.
>>>>
>>>> I'm slightly worry of this approach, we end up modifying a root
>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>> on QEMU, not sure that's a good trade off.
>>>>
>>>> Note also that different architectures will likely have different root
>>>> complex, and so you might need to modify several of them, plus then
>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>> so that devices belonging to different driver domains are assigned to
>>>> different bridges.
>>>
>>> I do think that adding something to the PCI conf register somewhere is
>>> the best option because it is not dependent on ACPI and it is not
>>> dependent on xenstore both of which are very undesirable.
>>>
>>> I am not sure where specifically is the best place. These are 3 ideas
>>> we came up with:
>>> 1. PCI root complex
>>> 2. a register on the device itself
>>> 3. a new capability of the device
>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>     grants capability
>>>
>>>
>>> Looking at the spec, there is a way to add a vendor-specific capability
>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>> today, Linux doesn't parse it.
>>
>> I did wonder the same from a quick look at the spec.  There's however
>> a text in the specification that says:
>>
>> "The driver SHOULD NOT use the Vendor data capability except for
>> debugging and reporting purposes."
>>
>> So we would at least need to change that because the capability would
>> then be used by other purposes different than debugging and reporting.
>>
>> Seems like a minor adjustment, so might we worth asking upstream about
>> their opinion, and to get a conversation started.
> 
> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
> we are doing, right?

I'd understand "reporting" as e.g. logging, transferring statistics, ...

We'd like to use it for configuration purposes.

Another idea would be to enhance the virtio IOMMU device to suit our needs:
we could add the domid as another virtio IOMMU device capability and (for now)
use bypass mode for all "productive" devices.

Later we could even add grant-V3 support to Xen and to the virtio IOMMU device
(see my last year Xen Summit design session). This could be usable for
disaggregated KVM setups, too, so I believe there is a chance to get this
accepted.

>>>>> **********
>>>>> What do you think about it? Are there any pitfalls, etc? This also requires
>>>>> system changes, but at least without virtio spec changes.
>>>>
>>>> Why are we so reluctant to add spec changes?  I understand this might
>>>> take time an effort, but it's the only way IMO to build a sustainable
>>>> VirtIO Xen implementation.  Did we already attempt to negotiate with
>>>> Oasis Xen related spec changes and those where refused?
>>>
>>> That's because spec changes can be very slow. This is a bug that we need
>>> a relatively quick solution for and waiting 12-24 months for a spec
>>> update is not realistic.
>>>
>>> I think a spec change would be best as a long term solution. We also
>>> need a short term solution. The short term solution doesn't have to be
>>> ideal but it has to work now.
>>
>> My fear with such approach is that once a bodge is in place people
>> move on to other stuff and this never gets properly fixed.
>>
>> I know this might not be a well received opinion, but it would be
>> better if such bodge is kept in each interested party patchqueue for
>> the time being, until a proper solution is implemented.  That way
>> there's an interest from parties into properly fixing it upstream.
> 
> Unfortunately we are in the situation where we have an outstanding
> upstream bug, so we have to take action one way or the other.

The required virtio IOMMU device modification would be rather small, so
adding it maybe under a CONFIG option defaulting to off might be
acceptable.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
> On 06.07.23 23:49, Stefano Stabellini wrote:
> > On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> > > On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> > > > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > > > > Part 2 (clarification):
> > > > > > 
> > > > > > I think using a special config space register in the root complex would
> > > > > > not be terrible in terms of guest changes because it is easy to
> > > > > > introduce a new root complex driver in Linux and other OSes. The root
> > > > > > complex would still be ECAM compatible so the regular ECAM driver would
> > > > > > still work. A new driver would only be necessary if you want to be able
> > > > > > to access the special config space register.
> > > > > 
> > > > > I'm slightly worry of this approach, we end up modifying a root
> > > > > complex emulation in order to avoid modifying a PCI device emulation
> > > > > on QEMU, not sure that's a good trade off.
> > > > > 
> > > > > Note also that different architectures will likely have different root
> > > > > complex, and so you might need to modify several of them, plus then
> > > > > arrange the PCI layout correctly in order to have the proper hierarchy
> > > > > so that devices belonging to different driver domains are assigned to
> > > > > different bridges.
> > > > 
> > > > I do think that adding something to the PCI conf register somewhere is
> > > > the best option because it is not dependent on ACPI and it is not
> > > > dependent on xenstore both of which are very undesirable.
> > > > 
> > > > I am not sure where specifically is the best place. These are 3 ideas
> > > > we came up with:
> > > > 1. PCI root complex
> > > > 2. a register on the device itself
> > > > 3. a new capability of the device
> > > > 4. add one extra dummy PCI device for the sole purpose of exposing the
> > > >     grants capability
> > > > 
> > > > 
> > > > Looking at the spec, there is a way to add a vendor-specific capability
> > > > (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> > > > today, Linux doesn't parse it.
> > > 
> > > I did wonder the same from a quick look at the spec.  There's however
> > > a text in the specification that says:
> > > 
> > > "The driver SHOULD NOT use the Vendor data capability except for
> > > debugging and reporting purposes."
> > > 
> > > So we would at least need to change that because the capability would
> > > then be used by other purposes different than debugging and reporting.
> > > 
> > > Seems like a minor adjustment, so might we worth asking upstream about
> > > their opinion, and to get a conversation started.
> > 
> > Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
> > we are doing, right?
> 
> I'd understand "reporting" as e.g. logging, transferring statistics, ...
> 
> We'd like to use it for configuration purposes.

I've also read it that way.

> Another idea would be to enhance the virtio IOMMU device to suit our needs:
> we could add the domid as another virtio IOMMU device capability and (for now)
> use bypass mode for all "productive" devices.

If we have to start adding capabilties, won't it be easier to just add
it to the each device instead of adding it to virtio IOMMU.  Or is the
parsing of capabilities device specific, and hence we would have to
implement such parsing for each device?  I would expect some
capabilities are shared between all devices, and a Xen capability could
be one of those.

> Later we could even add grant-V3 support to Xen and to the virtio IOMMU device
> (see my last year Xen Summit design session). This could be usable for
> disaggregated KVM setups, too, so I believe there is a chance to get this
> accepted.
> 
> > > > > > **********
> > > > > > What do you think about it? Are there any pitfalls, etc? This also requires
> > > > > > system changes, but at least without virtio spec changes.
> > > > > 
> > > > > Why are we so reluctant to add spec changes?  I understand this might
> > > > > take time an effort, but it's the only way IMO to build a sustainable
> > > > > VirtIO Xen implementation.  Did we already attempt to negotiate with
> > > > > Oasis Xen related spec changes and those where refused?
> > > > 
> > > > That's because spec changes can be very slow. This is a bug that we need
> > > > a relatively quick solution for and waiting 12-24 months for a spec
> > > > update is not realistic.
> > > > 
> > > > I think a spec change would be best as a long term solution. We also
> > > > need a short term solution. The short term solution doesn't have to be
> > > > ideal but it has to work now.
> > > 
> > > My fear with such approach is that once a bodge is in place people
> > > move on to other stuff and this never gets properly fixed.
> > > 
> > > I know this might not be a well received opinion, but it would be
> > > better if such bodge is kept in each interested party patchqueue for
> > > the time being, until a proper solution is implemented.  That way
> > > there's an interest from parties into properly fixing it upstream.
> > 
> > Unfortunately we are in the situation where we have an outstanding
> > upstream bug, so we have to take action one way or the other.
> 
> The required virtio IOMMU device modification would be rather small, so
> adding it maybe under a CONFIG option defaulting to off might be
> acceptable.

Would you then do the grant allocation as part of virtio IOMMU?

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 6215 bytes --]

On 07.07.23 11:50, Roger Pau Monné wrote:
> On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
>> On 06.07.23 23:49, Stefano Stabellini wrote:
>>> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>>>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>>>> Part 2 (clarification):
>>>>>>>
>>>>>>> I think using a special config space register in the root complex would
>>>>>>> not be terrible in terms of guest changes because it is easy to
>>>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>>>> still work. A new driver would only be necessary if you want to be able
>>>>>>> to access the special config space register.
>>>>>>
>>>>>> I'm slightly worry of this approach, we end up modifying a root
>>>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>>>> on QEMU, not sure that's a good trade off.
>>>>>>
>>>>>> Note also that different architectures will likely have different root
>>>>>> complex, and so you might need to modify several of them, plus then
>>>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>>>> so that devices belonging to different driver domains are assigned to
>>>>>> different bridges.
>>>>>
>>>>> I do think that adding something to the PCI conf register somewhere is
>>>>> the best option because it is not dependent on ACPI and it is not
>>>>> dependent on xenstore both of which are very undesirable.
>>>>>
>>>>> I am not sure where specifically is the best place. These are 3 ideas
>>>>> we came up with:
>>>>> 1. PCI root complex
>>>>> 2. a register on the device itself
>>>>> 3. a new capability of the device
>>>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>>>      grants capability
>>>>>
>>>>>
>>>>> Looking at the spec, there is a way to add a vendor-specific capability
>>>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>>>> today, Linux doesn't parse it.
>>>>
>>>> I did wonder the same from a quick look at the spec.  There's however
>>>> a text in the specification that says:
>>>>
>>>> "The driver SHOULD NOT use the Vendor data capability except for
>>>> debugging and reporting purposes."
>>>>
>>>> So we would at least need to change that because the capability would
>>>> then be used by other purposes different than debugging and reporting.
>>>>
>>>> Seems like a minor adjustment, so might we worth asking upstream about
>>>> their opinion, and to get a conversation started.
>>>
>>> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
>>> we are doing, right?
>>
>> I'd understand "reporting" as e.g. logging, transferring statistics, ...
>>
>> We'd like to use it for configuration purposes.
> 
> I've also read it that way.
> 
>> Another idea would be to enhance the virtio IOMMU device to suit our needs:
>> we could add the domid as another virtio IOMMU device capability and (for now)
>> use bypass mode for all "productive" devices.
> 
> If we have to start adding capabilties, won't it be easier to just add
> it to the each device instead of adding it to virtio IOMMU.  Or is the
> parsing of capabilities device specific, and hence we would have to
> implement such parsing for each device?  I would expect some
> capabilities are shared between all devices, and a Xen capability could
> be one of those.

Have a look at [1], which is describing the common device config layout.
The problem here is that we'd need to add the domid after the queue specific
data, resulting in a mess if further queue fields would be added later.

We could try that, of course.

> 
>> Later we could even add grant-V3 support to Xen and to the virtio IOMMU device
>> (see my last year Xen Summit design session). This could be usable for
>> disaggregated KVM setups, too, so I believe there is a chance to get this
>> accepted.
>>
>>>>>>> **********
>>>>>>> What do you think about it? Are there any pitfalls, etc? This also requires
>>>>>>> system changes, but at least without virtio spec changes.
>>>>>>
>>>>>> Why are we so reluctant to add spec changes?  I understand this might
>>>>>> take time an effort, but it's the only way IMO to build a sustainable
>>>>>> VirtIO Xen implementation.  Did we already attempt to negotiate with
>>>>>> Oasis Xen related spec changes and those where refused?
>>>>>
>>>>> That's because spec changes can be very slow. This is a bug that we need
>>>>> a relatively quick solution for and waiting 12-24 months for a spec
>>>>> update is not realistic.
>>>>>
>>>>> I think a spec change would be best as a long term solution. We also
>>>>> need a short term solution. The short term solution doesn't have to be
>>>>> ideal but it has to work now.
>>>>
>>>> My fear with such approach is that once a bodge is in place people
>>>> move on to other stuff and this never gets properly fixed.
>>>>
>>>> I know this might not be a well received opinion, but it would be
>>>> better if such bodge is kept in each interested party patchqueue for
>>>> the time being, until a proper solution is implemented.  That way
>>>> there's an interest from parties into properly fixing it upstream.
>>>
>>> Unfortunately we are in the situation where we have an outstanding
>>> upstream bug, so we have to take action one way or the other.
>>
>> The required virtio IOMMU device modification would be rather small, so
>> adding it maybe under a CONFIG option defaulting to off might be
>> acceptable.
> 
> Would you then do the grant allocation as part of virtio IOMMU?

Long term, maybe. Do you remember my Grant-V3 design session last year? Being
able to reuse the same layout for virtio IOMMU was one of the basic ideas for
that layout (this would need some heavy work on the virtio IOMMU frontend and
backend, of course).


Juergen

[1]: 
http://docs.oasis-open.org/virtio/virtio/v1.2/virtio-v1.2.pdf#subsubsection.4.1.4.3

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4405 bytes --]

On 07.07.23 16:10, Juergen Gross wrote:
> On 07.07.23 11:50, Roger Pau Monné wrote:
>> On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
>>> On 06.07.23 23:49, Stefano Stabellini wrote:
>>>> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>>>>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>>>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>>>>> Part 2 (clarification):
>>>>>>>>
>>>>>>>> I think using a special config space register in the root complex would
>>>>>>>> not be terrible in terms of guest changes because it is easy to
>>>>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>>>>> still work. A new driver would only be necessary if you want to be able
>>>>>>>> to access the special config space register.
>>>>>>>
>>>>>>> I'm slightly worry of this approach, we end up modifying a root
>>>>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>>>>> on QEMU, not sure that's a good trade off.
>>>>>>>
>>>>>>> Note also that different architectures will likely have different root
>>>>>>> complex, and so you might need to modify several of them, plus then
>>>>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>>>>> so that devices belonging to different driver domains are assigned to
>>>>>>> different bridges.
>>>>>>
>>>>>> I do think that adding something to the PCI conf register somewhere is
>>>>>> the best option because it is not dependent on ACPI and it is not
>>>>>> dependent on xenstore both of which are very undesirable.
>>>>>>
>>>>>> I am not sure where specifically is the best place. These are 3 ideas
>>>>>> we came up with:
>>>>>> 1. PCI root complex
>>>>>> 2. a register on the device itself
>>>>>> 3. a new capability of the device
>>>>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>>>>      grants capability
>>>>>>
>>>>>>
>>>>>> Looking at the spec, there is a way to add a vendor-specific capability
>>>>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>>>>> today, Linux doesn't parse it.
>>>>>
>>>>> I did wonder the same from a quick look at the spec.  There's however
>>>>> a text in the specification that says:
>>>>>
>>>>> "The driver SHOULD NOT use the Vendor data capability except for
>>>>> debugging and reporting purposes."
>>>>>
>>>>> So we would at least need to change that because the capability would
>>>>> then be used by other purposes different than debugging and reporting.
>>>>>
>>>>> Seems like a minor adjustment, so might we worth asking upstream about
>>>>> their opinion, and to get a conversation started.
>>>>
>>>> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
>>>> we are doing, right?
>>>
>>> I'd understand "reporting" as e.g. logging, transferring statistics, ...
>>>
>>> We'd like to use it for configuration purposes.
>>
>> I've also read it that way.
>>
>>> Another idea would be to enhance the virtio IOMMU device to suit our needs:
>>> we could add the domid as another virtio IOMMU device capability and (for now)
>>> use bypass mode for all "productive" devices.
>>
>> If we have to start adding capabilties, won't it be easier to just add
>> it to the each device instead of adding it to virtio IOMMU.  Or is the
>> parsing of capabilities device specific, and hence we would have to
>> implement such parsing for each device?  I would expect some
>> capabilities are shared between all devices, and a Xen capability could
>> be one of those.
> 
> Have a look at [1], which is describing the common device config layout.
> The problem here is that we'd need to add the domid after the queue specific
> data, resulting in a mess if further queue fields would be added later.
> 
> We could try that, of course.

Thinking more about it, the virtio IOMMU device seems to be a better fit:

In case we'd add the domid to the device's PCI config space, the value would
be controlled by the backend domain. IMO the domid passed to the frontend
should be controlled by a trusted entity (dom0 or the hypervisor), which
would be the natural backend of the virtio IOMMU device.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Fri, Jul 07, 2023 at 04:27:59PM +0200, Juergen Gross wrote:
> On 07.07.23 16:10, Juergen Gross wrote:
> > On 07.07.23 11:50, Roger Pau Monné wrote:
> > > On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
> > > > On 06.07.23 23:49, Stefano Stabellini wrote:
> > > > > On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> > > > > > On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> > > > > > > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > > > > > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > > > > > > > Part 2 (clarification):
> > > > > > > > > 
> > > > > > > > > I think using a special config space register in the root complex would
> > > > > > > > > not be terrible in terms of guest changes because it is easy to
> > > > > > > > > introduce a new root complex driver in Linux and other OSes. The root
> > > > > > > > > complex would still be ECAM compatible so the regular ECAM driver would
> > > > > > > > > still work. A new driver would only be necessary if you want to be able
> > > > > > > > > to access the special config space register.
> > > > > > > > 
> > > > > > > > I'm slightly worry of this approach, we end up modifying a root
> > > > > > > > complex emulation in order to avoid modifying a PCI device emulation
> > > > > > > > on QEMU, not sure that's a good trade off.
> > > > > > > > 
> > > > > > > > Note also that different architectures will likely have different root
> > > > > > > > complex, and so you might need to modify several of them, plus then
> > > > > > > > arrange the PCI layout correctly in order to have the proper hierarchy
> > > > > > > > so that devices belonging to different driver domains are assigned to
> > > > > > > > different bridges.
> > > > > > > 
> > > > > > > I do think that adding something to the PCI conf register somewhere is
> > > > > > > the best option because it is not dependent on ACPI and it is not
> > > > > > > dependent on xenstore both of which are very undesirable.
> > > > > > > 
> > > > > > > I am not sure where specifically is the best place. These are 3 ideas
> > > > > > > we came up with:
> > > > > > > 1. PCI root complex
> > > > > > > 2. a register on the device itself
> > > > > > > 3. a new capability of the device
> > > > > > > 4. add one extra dummy PCI device for the sole purpose of exposing the
> > > > > > >      grants capability
> > > > > > > 
> > > > > > > 
> > > > > > > Looking at the spec, there is a way to add a vendor-specific capability
> > > > > > > (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> > > > > > > today, Linux doesn't parse it.
> > > > > > 
> > > > > > I did wonder the same from a quick look at the spec.  There's however
> > > > > > a text in the specification that says:
> > > > > > 
> > > > > > "The driver SHOULD NOT use the Vendor data capability except for
> > > > > > debugging and reporting purposes."
> > > > > > 
> > > > > > So we would at least need to change that because the capability would
> > > > > > then be used by other purposes different than debugging and reporting.
> > > > > > 
> > > > > > Seems like a minor adjustment, so might we worth asking upstream about
> > > > > > their opinion, and to get a conversation started.
> > > > > 
> > > > > Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
> > > > > we are doing, right?
> > > > 
> > > > I'd understand "reporting" as e.g. logging, transferring statistics, ...
> > > > 
> > > > We'd like to use it for configuration purposes.
> > > 
> > > I've also read it that way.
> > > 
> > > > Another idea would be to enhance the virtio IOMMU device to suit our needs:
> > > > we could add the domid as another virtio IOMMU device capability and (for now)
> > > > use bypass mode for all "productive" devices.
> > > 
> > > If we have to start adding capabilties, won't it be easier to just add
> > > it to the each device instead of adding it to virtio IOMMU.  Or is the
> > > parsing of capabilities device specific, and hence we would have to
> > > implement such parsing for each device?  I would expect some
> > > capabilities are shared between all devices, and a Xen capability could
> > > be one of those.
> > 
> > Have a look at [1], which is describing the common device config layout.
> > The problem here is that we'd need to add the domid after the queue specific
> > data, resulting in a mess if further queue fields would be added later.
> > 
> > We could try that, of course.
> 
> Thinking more about it, the virtio IOMMU device seems to be a better fit:
> 
> In case we'd add the domid to the device's PCI config space, the value would
> be controlled by the backend domain. IMO the domid passed to the frontend
> should be controlled by a trusted entity (dom0 or the hypervisor), which
> would be the natural backend of the virtio IOMMU device.

Hm, yes.  I'm however failing to see how a backed could exploit that.

The guest would be granting memory to a different domain than the one
running the backend, but otherwise that memory would be granted to the
backend domain, which could then also make it available to other
domains (without having to play with the reported backend domid).

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 5314 bytes --]

On 07.07.23 16:48, Roger Pau Monné wrote:
> On Fri, Jul 07, 2023 at 04:27:59PM +0200, Juergen Gross wrote:
>> On 07.07.23 16:10, Juergen Gross wrote:
>>> On 07.07.23 11:50, Roger Pau Monné wrote:
>>>> On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
>>>>> On 06.07.23 23:49, Stefano Stabellini wrote:
>>>>>> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>>>>>>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>>>>>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>>>>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>>>>>>> Part 2 (clarification):
>>>>>>>>>>
>>>>>>>>>> I think using a special config space register in the root complex would
>>>>>>>>>> not be terrible in terms of guest changes because it is easy to
>>>>>>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>>>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>>>>>>> still work. A new driver would only be necessary if you want to be able
>>>>>>>>>> to access the special config space register.
>>>>>>>>>
>>>>>>>>> I'm slightly worry of this approach, we end up modifying a root
>>>>>>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>>>>>>> on QEMU, not sure that's a good trade off.
>>>>>>>>>
>>>>>>>>> Note also that different architectures will likely have different root
>>>>>>>>> complex, and so you might need to modify several of them, plus then
>>>>>>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>>>>>>> so that devices belonging to different driver domains are assigned to
>>>>>>>>> different bridges.
>>>>>>>>
>>>>>>>> I do think that adding something to the PCI conf register somewhere is
>>>>>>>> the best option because it is not dependent on ACPI and it is not
>>>>>>>> dependent on xenstore both of which are very undesirable.
>>>>>>>>
>>>>>>>> I am not sure where specifically is the best place. These are 3 ideas
>>>>>>>> we came up with:
>>>>>>>> 1. PCI root complex
>>>>>>>> 2. a register on the device itself
>>>>>>>> 3. a new capability of the device
>>>>>>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>>>>>>       grants capability
>>>>>>>>
>>>>>>>>
>>>>>>>> Looking at the spec, there is a way to add a vendor-specific capability
>>>>>>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>>>>>>> today, Linux doesn't parse it.
>>>>>>>
>>>>>>> I did wonder the same from a quick look at the spec.  There's however
>>>>>>> a text in the specification that says:
>>>>>>>
>>>>>>> "The driver SHOULD NOT use the Vendor data capability except for
>>>>>>> debugging and reporting purposes."
>>>>>>>
>>>>>>> So we would at least need to change that because the capability would
>>>>>>> then be used by other purposes different than debugging and reporting.
>>>>>>>
>>>>>>> Seems like a minor adjustment, so might we worth asking upstream about
>>>>>>> their opinion, and to get a conversation started.
>>>>>>
>>>>>> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
>>>>>> we are doing, right?
>>>>>
>>>>> I'd understand "reporting" as e.g. logging, transferring statistics, ...
>>>>>
>>>>> We'd like to use it for configuration purposes.
>>>>
>>>> I've also read it that way.
>>>>
>>>>> Another idea would be to enhance the virtio IOMMU device to suit our needs:
>>>>> we could add the domid as another virtio IOMMU device capability and (for now)
>>>>> use bypass mode for all "productive" devices.
>>>>
>>>> If we have to start adding capabilties, won't it be easier to just add
>>>> it to the each device instead of adding it to virtio IOMMU.  Or is the
>>>> parsing of capabilities device specific, and hence we would have to
>>>> implement such parsing for each device?  I would expect some
>>>> capabilities are shared between all devices, and a Xen capability could
>>>> be one of those.
>>>
>>> Have a look at [1], which is describing the common device config layout.
>>> The problem here is that we'd need to add the domid after the queue specific
>>> data, resulting in a mess if further queue fields would be added later.
>>>
>>> We could try that, of course.
>>
>> Thinking more about it, the virtio IOMMU device seems to be a better fit:
>>
>> In case we'd add the domid to the device's PCI config space, the value would
>> be controlled by the backend domain. IMO the domid passed to the frontend
>> should be controlled by a trusted entity (dom0 or the hypervisor), which
>> would be the natural backend of the virtio IOMMU device.
> 
> Hm, yes.  I'm however failing to see how a backed could exploit that.
> 
> The guest would be granting memory to a different domain than the one
> running the backend, but otherwise that memory would be granted to the
> backend domain, which could then also make it available to other
> domains (without having to play with the reported backend domid).

I agree that an exploit is at least not obvious.

It is still not a clean solution, though.

Giving the wrong domain direct access to some of the guest's memory is worse
than the ability to pass the contents indirectly to the wrong domain IMHO.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Fri, Jul 07, 2023 at 04:10:14PM +0200, Juergen Gross wrote:
> On 07.07.23 11:50, Roger Pau Monné wrote:
> > On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
> > > On 06.07.23 23:49, Stefano Stabellini wrote:
> > > > On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> > > > > On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> > > > > > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > > > > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > > > > > > Part 2 (clarification):
> > > > > > > > 
> > > > > > > > I think using a special config space register in the root complex would
> > > > > > > > not be terrible in terms of guest changes because it is easy to
> > > > > > > > introduce a new root complex driver in Linux and other OSes. The root
> > > > > > > > complex would still be ECAM compatible so the regular ECAM driver would
> > > > > > > > still work. A new driver would only be necessary if you want to be able
> > > > > > > > to access the special config space register.
> > > > > > > 
> > > > > > > I'm slightly worry of this approach, we end up modifying a root
> > > > > > > complex emulation in order to avoid modifying a PCI device emulation
> > > > > > > on QEMU, not sure that's a good trade off.
> > > > > > > 
> > > > > > > Note also that different architectures will likely have different root
> > > > > > > complex, and so you might need to modify several of them, plus then
> > > > > > > arrange the PCI layout correctly in order to have the proper hierarchy
> > > > > > > so that devices belonging to different driver domains are assigned to
> > > > > > > different bridges.
> > > > > > 
> > > > > > I do think that adding something to the PCI conf register somewhere is
> > > > > > the best option because it is not dependent on ACPI and it is not
> > > > > > dependent on xenstore both of which are very undesirable.
> > > > > > 
> > > > > > I am not sure where specifically is the best place. These are 3 ideas
> > > > > > we came up with:
> > > > > > 1. PCI root complex
> > > > > > 2. a register on the device itself
> > > > > > 3. a new capability of the device
> > > > > > 4. add one extra dummy PCI device for the sole purpose of exposing the
> > > > > >      grants capability
> > > > > > 
> > > > > > 
> > > > > > Looking at the spec, there is a way to add a vendor-specific capability
> > > > > > (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> > > > > > today, Linux doesn't parse it.
> > > > > 
> > > > > I did wonder the same from a quick look at the spec.  There's however
> > > > > a text in the specification that says:
> > > > > 
> > > > > "The driver SHOULD NOT use the Vendor data capability except for
> > > > > debugging and reporting purposes."
> > > > > 
> > > > > So we would at least need to change that because the capability would
> > > > > then be used by other purposes different than debugging and reporting.
> > > > > 
> > > > > Seems like a minor adjustment, so might we worth asking upstream about
> > > > > their opinion, and to get a conversation started.
> > > > 
> > > > Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
> > > > we are doing, right?
> > > 
> > > I'd understand "reporting" as e.g. logging, transferring statistics, ...
> > > 
> > > We'd like to use it for configuration purposes.
> > 
> > I've also read it that way.
> > 
> > > Another idea would be to enhance the virtio IOMMU device to suit our needs:
> > > we could add the domid as another virtio IOMMU device capability and (for now)
> > > use bypass mode for all "productive" devices.
> > 
> > If we have to start adding capabilties, won't it be easier to just add
> > it to the each device instead of adding it to virtio IOMMU.  Or is the
> > parsing of capabilities device specific, and hence we would have to
> > implement such parsing for each device?  I would expect some
> > capabilities are shared between all devices, and a Xen capability could
> > be one of those.
> 
> Have a look at [1], which is describing the common device config layout.
> The problem here is that we'd need to add the domid after the queue specific
> data, resulting in a mess if further queue fields would be added later.
> 
> We could try that, of course.

Right, we must make it part of the standard if we modify
virtio_pci_common_cfg, or else newly added fields would overlap the
Xen specific one.

Would it be possible to signal Xen-grants support in the
`device_feature` field, and then expose it from a vendor capability?
IOW, would it be possible to add a Xen-specific hook in the parsing of
virtio_pci_common_cfg that would then fetch additional data from a
capability?

That would likely be less intrusive than adding a new Xen-specific
field to virtio_pci_common_cfg while still allowing us to do Xen
specific configuration for all VirtIO devices.

> > 
> > > Later we could even add grant-V3 support to Xen and to the virtio IOMMU device
> > > (see my last year Xen Summit design session). This could be usable for
> > > disaggregated KVM setups, too, so I believe there is a chance to get this
> > > accepted.
> > > 
> > > > > > > > **********
> > > > > > > > What do you think about it? Are there any pitfalls, etc? This also requires
> > > > > > > > system changes, but at least without virtio spec changes.
> > > > > > > 
> > > > > > > Why are we so reluctant to add spec changes?  I understand this might
> > > > > > > take time an effort, but it's the only way IMO to build a sustainable
> > > > > > > VirtIO Xen implementation.  Did we already attempt to negotiate with
> > > > > > > Oasis Xen related spec changes and those where refused?
> > > > > > 
> > > > > > That's because spec changes can be very slow. This is a bug that we need
> > > > > > a relatively quick solution for and waiting 12-24 months for a spec
> > > > > > update is not realistic.
> > > > > > 
> > > > > > I think a spec change would be best as a long term solution. We also
> > > > > > need a short term solution. The short term solution doesn't have to be
> > > > > > ideal but it has to work now.
> > > > > 
> > > > > My fear with such approach is that once a bodge is in place people
> > > > > move on to other stuff and this never gets properly fixed.
> > > > > 
> > > > > I know this might not be a well received opinion, but it would be
> > > > > better if such bodge is kept in each interested party patchqueue for
> > > > > the time being, until a proper solution is implemented.  That way
> > > > > there's an interest from parties into properly fixing it upstream.
> > > > 
> > > > Unfortunately we are in the situation where we have an outstanding
> > > > upstream bug, so we have to take action one way or the other.
> > > 
> > > The required virtio IOMMU device modification would be rather small, so
> > > adding it maybe under a CONFIG option defaulting to off might be
> > > acceptable.
> > 
> > Would you then do the grant allocation as part of virtio IOMMU?
> 
> Long term, maybe. Do you remember my Grant-V3 design session last year? Being
> able to reuse the same layout for virtio IOMMU was one of the basic ideas for
> that layout (this would need some heavy work on the virtio IOMMU frontend and
> backend, of course).

While this might well be the best option, do we have anyone with the
time and expertise to work on this?  I might be wrong, but it seems
like a huge task.

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 7671 bytes --]

On 07.07.23 16:42, Roger Pau Monné wrote:
> On Fri, Jul 07, 2023 at 04:10:14PM +0200, Juergen Gross wrote:
>> On 07.07.23 11:50, Roger Pau Monné wrote:
>>> On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
>>>> On 06.07.23 23:49, Stefano Stabellini wrote:
>>>>> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>>>>>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>>>>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>>>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>>>>>> Part 2 (clarification):
>>>>>>>>>
>>>>>>>>> I think using a special config space register in the root complex would
>>>>>>>>> not be terrible in terms of guest changes because it is easy to
>>>>>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>>>>>> still work. A new driver would only be necessary if you want to be able
>>>>>>>>> to access the special config space register.
>>>>>>>>
>>>>>>>> I'm slightly worry of this approach, we end up modifying a root
>>>>>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>>>>>> on QEMU, not sure that's a good trade off.
>>>>>>>>
>>>>>>>> Note also that different architectures will likely have different root
>>>>>>>> complex, and so you might need to modify several of them, plus then
>>>>>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>>>>>> so that devices belonging to different driver domains are assigned to
>>>>>>>> different bridges.
>>>>>>>
>>>>>>> I do think that adding something to the PCI conf register somewhere is
>>>>>>> the best option because it is not dependent on ACPI and it is not
>>>>>>> dependent on xenstore both of which are very undesirable.
>>>>>>>
>>>>>>> I am not sure where specifically is the best place. These are 3 ideas
>>>>>>> we came up with:
>>>>>>> 1. PCI root complex
>>>>>>> 2. a register on the device itself
>>>>>>> 3. a new capability of the device
>>>>>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>>>>>       grants capability
>>>>>>>
>>>>>>>
>>>>>>> Looking at the spec, there is a way to add a vendor-specific capability
>>>>>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>>>>>> today, Linux doesn't parse it.
>>>>>>
>>>>>> I did wonder the same from a quick look at the spec.  There's however
>>>>>> a text in the specification that says:
>>>>>>
>>>>>> "The driver SHOULD NOT use the Vendor data capability except for
>>>>>> debugging and reporting purposes."
>>>>>>
>>>>>> So we would at least need to change that because the capability would
>>>>>> then be used by other purposes different than debugging and reporting.
>>>>>>
>>>>>> Seems like a minor adjustment, so might we worth asking upstream about
>>>>>> their opinion, and to get a conversation started.
>>>>>
>>>>> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
>>>>> we are doing, right?
>>>>
>>>> I'd understand "reporting" as e.g. logging, transferring statistics, ...
>>>>
>>>> We'd like to use it for configuration purposes.
>>>
>>> I've also read it that way.
>>>
>>>> Another idea would be to enhance the virtio IOMMU device to suit our needs:
>>>> we could add the domid as another virtio IOMMU device capability and (for now)
>>>> use bypass mode for all "productive" devices.
>>>
>>> If we have to start adding capabilties, won't it be easier to just add
>>> it to the each device instead of adding it to virtio IOMMU.  Or is the
>>> parsing of capabilities device specific, and hence we would have to
>>> implement such parsing for each device?  I would expect some
>>> capabilities are shared between all devices, and a Xen capability could
>>> be one of those.
>>
>> Have a look at [1], which is describing the common device config layout.
>> The problem here is that we'd need to add the domid after the queue specific
>> data, resulting in a mess if further queue fields would be added later.
>>
>> We could try that, of course.
> 
> Right, we must make it part of the standard if we modify
> virtio_pci_common_cfg, or else newly added fields would overlap the
> Xen specific one.
> 
> Would it be possible to signal Xen-grants support in the
> `device_feature` field, and then expose it from a vendor capability?
> IOW, would it be possible to add a Xen-specific hook in the parsing of
> virtio_pci_common_cfg that would then fetch additional data from a
> capability?

TBH, I don't know. It might require some changes in the central parsing
logic, but this shouldn't be too hard to do.

> That would likely be less intrusive than adding a new Xen-specific
> field to virtio_pci_common_cfg while still allowing us to do Xen
> specific configuration for all VirtIO devices.

In case we want to go that route, this should be in a new "platform config"
capability, which might be just another form of a vendor capability.

> 
>>>
>>>> Later we could even add grant-V3 support to Xen and to the virtio IOMMU device
>>>> (see my last year Xen Summit design session). This could be usable for
>>>> disaggregated KVM setups, too, so I believe there is a chance to get this
>>>> accepted.
>>>>
>>>>>>>>> **********
>>>>>>>>> What do you think about it? Are there any pitfalls, etc? This also requires
>>>>>>>>> system changes, but at least without virtio spec changes.
>>>>>>>>
>>>>>>>> Why are we so reluctant to add spec changes?  I understand this might
>>>>>>>> take time an effort, but it's the only way IMO to build a sustainable
>>>>>>>> VirtIO Xen implementation.  Did we already attempt to negotiate with
>>>>>>>> Oasis Xen related spec changes and those where refused?
>>>>>>>
>>>>>>> That's because spec changes can be very slow. This is a bug that we need
>>>>>>> a relatively quick solution for and waiting 12-24 months for a spec
>>>>>>> update is not realistic.
>>>>>>>
>>>>>>> I think a spec change would be best as a long term solution. We also
>>>>>>> need a short term solution. The short term solution doesn't have to be
>>>>>>> ideal but it has to work now.
>>>>>>
>>>>>> My fear with such approach is that once a bodge is in place people
>>>>>> move on to other stuff and this never gets properly fixed.
>>>>>>
>>>>>> I know this might not be a well received opinion, but it would be
>>>>>> better if such bodge is kept in each interested party patchqueue for
>>>>>> the time being, until a proper solution is implemented.  That way
>>>>>> there's an interest from parties into properly fixing it upstream.
>>>>>
>>>>> Unfortunately we are in the situation where we have an outstanding
>>>>> upstream bug, so we have to take action one way or the other.
>>>>
>>>> The required virtio IOMMU device modification would be rather small, so
>>>> adding it maybe under a CONFIG option defaulting to off might be
>>>> acceptable.
>>>
>>> Would you then do the grant allocation as part of virtio IOMMU?
>>
>> Long term, maybe. Do you remember my Grant-V3 design session last year? Being
>> able to reuse the same layout for virtio IOMMU was one of the basic ideas for
>> that layout (this would need some heavy work on the virtio IOMMU frontend and
>> backend, of course).
> 
> While this might well be the best option, do we have anyone with the
> time and expertise to work on this?  I might be wrong, but it seems
> like a huge task.

As a background project I'd like to pursue it. OTOH I'm not sure how much time
I could spend on it.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Roger Pau Monné]

On Fri, Jul 07, 2023 at 05:01:38PM +0200, Juergen Gross wrote:
> On 07.07.23 16:42, Roger Pau Monné wrote:
> > On Fri, Jul 07, 2023 at 04:10:14PM +0200, Juergen Gross wrote:
> > > On 07.07.23 11:50, Roger Pau Monné wrote:
> > > > On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
> > > > > On 06.07.23 23:49, Stefano Stabellini wrote:
> > > > > > On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> > > > > > > On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
> > > > > > > > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > > > > > > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
> > > > > > > > > > Part 2 (clarification):
> > > > > > > > > > 
> > > > > > > > > > I think using a special config space register in the root complex would
> > > > > > > > > > not be terrible in terms of guest changes because it is easy to
> > > > > > > > > > introduce a new root complex driver in Linux and other OSes. The root
> > > > > > > > > > complex would still be ECAM compatible so the regular ECAM driver would
> > > > > > > > > > still work. A new driver would only be necessary if you want to be able
> > > > > > > > > > to access the special config space register.
> > > > > > > > > 
> > > > > > > > > I'm slightly worry of this approach, we end up modifying a root
> > > > > > > > > complex emulation in order to avoid modifying a PCI device emulation
> > > > > > > > > on QEMU, not sure that's a good trade off.
> > > > > > > > > 
> > > > > > > > > Note also that different architectures will likely have different root
> > > > > > > > > complex, and so you might need to modify several of them, plus then
> > > > > > > > > arrange the PCI layout correctly in order to have the proper hierarchy
> > > > > > > > > so that devices belonging to different driver domains are assigned to
> > > > > > > > > different bridges.
> > > > > > > > 
> > > > > > > > I do think that adding something to the PCI conf register somewhere is
> > > > > > > > the best option because it is not dependent on ACPI and it is not
> > > > > > > > dependent on xenstore both of which are very undesirable.
> > > > > > > > 
> > > > > > > > I am not sure where specifically is the best place. These are 3 ideas
> > > > > > > > we came up with:
> > > > > > > > 1. PCI root complex
> > > > > > > > 2. a register on the device itself
> > > > > > > > 3. a new capability of the device
> > > > > > > > 4. add one extra dummy PCI device for the sole purpose of exposing the
> > > > > > > >       grants capability
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Looking at the spec, there is a way to add a vendor-specific capability
> > > > > > > > (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
> > > > > > > > today, Linux doesn't parse it.
> > > > > > > 
> > > > > > > I did wonder the same from a quick look at the spec.  There's however
> > > > > > > a text in the specification that says:
> > > > > > > 
> > > > > > > "The driver SHOULD NOT use the Vendor data capability except for
> > > > > > > debugging and reporting purposes."
> > > > > > > 
> > > > > > > So we would at least need to change that because the capability would
> > > > > > > then be used by other purposes different than debugging and reporting.
> > > > > > > 
> > > > > > > Seems like a minor adjustment, so might we worth asking upstream about
> > > > > > > their opinion, and to get a conversation started.
> > > > > > 
> > > > > > Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
> > > > > > we are doing, right?
> > > > > 
> > > > > I'd understand "reporting" as e.g. logging, transferring statistics, ...
> > > > > 
> > > > > We'd like to use it for configuration purposes.
> > > > 
> > > > I've also read it that way.
> > > > 
> > > > > Another idea would be to enhance the virtio IOMMU device to suit our needs:
> > > > > we could add the domid as another virtio IOMMU device capability and (for now)
> > > > > use bypass mode for all "productive" devices.
> > > > 
> > > > If we have to start adding capabilties, won't it be easier to just add
> > > > it to the each device instead of adding it to virtio IOMMU.  Or is the
> > > > parsing of capabilities device specific, and hence we would have to
> > > > implement such parsing for each device?  I would expect some
> > > > capabilities are shared between all devices, and a Xen capability could
> > > > be one of those.
> > > 
> > > Have a look at [1], which is describing the common device config layout.
> > > The problem here is that we'd need to add the domid after the queue specific
> > > data, resulting in a mess if further queue fields would be added later.
> > > 
> > > We could try that, of course.
> > 
> > Right, we must make it part of the standard if we modify
> > virtio_pci_common_cfg, or else newly added fields would overlap the
> > Xen specific one.
> > 
> > Would it be possible to signal Xen-grants support in the
> > `device_feature` field, and then expose it from a vendor capability?
> > IOW, would it be possible to add a Xen-specific hook in the parsing of
> > virtio_pci_common_cfg that would then fetch additional data from a
> > capability?
> 
> TBH, I don't know. It might require some changes in the central parsing
> logic, but this shouldn't be too hard to do.
> 
> > That would likely be less intrusive than adding a new Xen-specific
> > field to virtio_pci_common_cfg while still allowing us to do Xen
> > specific configuration for all VirtIO devices.
> 
> In case we want to go that route, this should be in a new "platform config"
> capability, which might be just another form of a vendor capability.

I think telling people that they will need to implement grants-v3 in
order to solve this might be too much.  I would rather prefer a more
concrete solution that doesn't have so many loose ends.

Anyway, it's up to the person doing the job, but starting with "you
will have to implement grants-v3" is quite likely to deter anyone from
attempting to solve this I'm afraid.

Thanks, Roger.

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 5829 bytes --]

On 07.07.23 17:14, Roger Pau Monné wrote:
> On Fri, Jul 07, 2023 at 05:01:38PM +0200, Juergen Gross wrote:
>> On 07.07.23 16:42, Roger Pau Monné wrote:
>>> On Fri, Jul 07, 2023 at 04:10:14PM +0200, Juergen Gross wrote:
>>>> On 07.07.23 11:50, Roger Pau Monné wrote:
>>>>> On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
>>>>>> On 06.07.23 23:49, Stefano Stabellini wrote:
>>>>>>> On Thu, 6 Jul 2023, Roger Pau Monné wrote:
>>>>>>>> On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini wrote:
>>>>>>>>> On Wed, 5 Jul 2023, Roger Pau Monné wrote:
>>>>>>>>>> On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko wrote:
>>>>>>>>>>> Part 2 (clarification):
>>>>>>>>>>>
>>>>>>>>>>> I think using a special config space register in the root complex would
>>>>>>>>>>> not be terrible in terms of guest changes because it is easy to
>>>>>>>>>>> introduce a new root complex driver in Linux and other OSes. The root
>>>>>>>>>>> complex would still be ECAM compatible so the regular ECAM driver would
>>>>>>>>>>> still work. A new driver would only be necessary if you want to be able
>>>>>>>>>>> to access the special config space register.
>>>>>>>>>>
>>>>>>>>>> I'm slightly worry of this approach, we end up modifying a root
>>>>>>>>>> complex emulation in order to avoid modifying a PCI device emulation
>>>>>>>>>> on QEMU, not sure that's a good trade off.
>>>>>>>>>>
>>>>>>>>>> Note also that different architectures will likely have different root
>>>>>>>>>> complex, and so you might need to modify several of them, plus then
>>>>>>>>>> arrange the PCI layout correctly in order to have the proper hierarchy
>>>>>>>>>> so that devices belonging to different driver domains are assigned to
>>>>>>>>>> different bridges.
>>>>>>>>>
>>>>>>>>> I do think that adding something to the PCI conf register somewhere is
>>>>>>>>> the best option because it is not dependent on ACPI and it is not
>>>>>>>>> dependent on xenstore both of which are very undesirable.
>>>>>>>>>
>>>>>>>>> I am not sure where specifically is the best place. These are 3 ideas
>>>>>>>>> we came up with:
>>>>>>>>> 1. PCI root complex
>>>>>>>>> 2. a register on the device itself
>>>>>>>>> 3. a new capability of the device
>>>>>>>>> 4. add one extra dummy PCI device for the sole purpose of exposing the
>>>>>>>>>        grants capability
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Looking at the spec, there is a way to add a vendor-specific capability
>>>>>>>>> (cap_vndr = 0x9). Could we use that? It doesn't look like it is used
>>>>>>>>> today, Linux doesn't parse it.
>>>>>>>>
>>>>>>>> I did wonder the same from a quick look at the spec.  There's however
>>>>>>>> a text in the specification that says:
>>>>>>>>
>>>>>>>> "The driver SHOULD NOT use the Vendor data capability except for
>>>>>>>> debugging and reporting purposes."
>>>>>>>>
>>>>>>>> So we would at least need to change that because the capability would
>>>>>>>> then be used by other purposes different than debugging and reporting.
>>>>>>>>
>>>>>>>> Seems like a minor adjustment, so might we worth asking upstream about
>>>>>>>> their opinion, and to get a conversation started.
>>>>>>>
>>>>>>> Wait, wouldn't this use-case fall under "reporting" ? It is exactly what
>>>>>>> we are doing, right?
>>>>>>
>>>>>> I'd understand "reporting" as e.g. logging, transferring statistics, ...
>>>>>>
>>>>>> We'd like to use it for configuration purposes.
>>>>>
>>>>> I've also read it that way.
>>>>>
>>>>>> Another idea would be to enhance the virtio IOMMU device to suit our needs:
>>>>>> we could add the domid as another virtio IOMMU device capability and (for now)
>>>>>> use bypass mode for all "productive" devices.
>>>>>
>>>>> If we have to start adding capabilties, won't it be easier to just add
>>>>> it to the each device instead of adding it to virtio IOMMU.  Or is the
>>>>> parsing of capabilities device specific, and hence we would have to
>>>>> implement such parsing for each device?  I would expect some
>>>>> capabilities are shared between all devices, and a Xen capability could
>>>>> be one of those.
>>>>
>>>> Have a look at [1], which is describing the common device config layout.
>>>> The problem here is that we'd need to add the domid after the queue specific
>>>> data, resulting in a mess if further queue fields would be added later.
>>>>
>>>> We could try that, of course.
>>>
>>> Right, we must make it part of the standard if we modify
>>> virtio_pci_common_cfg, or else newly added fields would overlap the
>>> Xen specific one.
>>>
>>> Would it be possible to signal Xen-grants support in the
>>> `device_feature` field, and then expose it from a vendor capability?
>>> IOW, would it be possible to add a Xen-specific hook in the parsing of
>>> virtio_pci_common_cfg that would then fetch additional data from a
>>> capability?
>>
>> TBH, I don't know. It might require some changes in the central parsing
>> logic, but this shouldn't be too hard to do.
>>
>>> That would likely be less intrusive than adding a new Xen-specific
>>> field to virtio_pci_common_cfg while still allowing us to do Xen
>>> specific configuration for all VirtIO devices.
>>
>> In case we want to go that route, this should be in a new "platform config"
>> capability, which might be just another form of a vendor capability.
> 
> I think telling people that they will need to implement grants-v3 in
> order to solve this might be too much.  I would rather prefer a more
> concrete solution that doesn't have so many loose ends.
> 
> Anyway, it's up to the person doing the job, but starting with "you
> will have to implement grants-v3" is quite likely to deter anyone from
> attempting to solve this I'm afraid.

Fair enough. :-)


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 7371 bytes --]

Re-reading the whole thread again ...

On 29.06.23 03:00, Stefano Stabellini wrote:
> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>> On 21.06.23 16:12, Petr Pavlu wrote:
>>
>>
>> Hello Petr
>>
>>
>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>> eventually results in a hang during the boot.
>>>
>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>> makes no progress:
>>>
>>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>     #1  0xffffffff817086b7 in virtqueue_get_buf (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) at ../drivers/virtio/virtio_ring.c:2333
>>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized out>, port_id=0xffffffff, event=0x0, value=0x1) at ../drivers/char/virtio_console.c:562
>>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized out>, port_id=<optimized out>, event=<optimized out>, value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>     #4  0xffffffff817618b1 in virtcons_probe (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>     #5  0xffffffff81707117 in virtio_dev_probe (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>     #6  0xffffffff8198e348 in call_driver_probe (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:658
>>>     #8  0xffffffff8198e58f in __driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>     #9  0xffffffff8198e65a in driver_probe_device (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>     #10 0xffffffff8198e832 in __driver_attach (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1216
>>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, start=start@entry=0x0 <fixed_percpu_data>, data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at ../drivers/base/bus.c:368
>>>     #12 0xffffffff8198db65 in driver_attach (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/dd.c:1233
>>>     #13 0xffffffff8198d207 in bus_add_driver (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/bus.c:673
>>>     #14 0xffffffff8198f550 in driver_register (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/base/driver.c:246
>>>     #15 0xffffffff81706b47 in register_virtio_driver (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at ../drivers/virtio/virtio.c:357
>>>     #16 0xffffffff832cd34b in virtio_console_init () at ../drivers/char/virtio_console.c:2258
>>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 <virtio_console_init>) at ../init/main.c:1246
>>>     #18 0xffffffff83277293 in do_initcall_level (command_line=0xffff888003e2f900 "root", level=0x6) at ../init/main.c:1319
>>>     #19 do_initcalls () at ../init/main.c:1335
>>>     #20 do_basic_setup () at ../init/main.c:1354
>>>     #21 kernel_init_freeable () at ../init/main.c:1571
>>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) at ../init/main.c:1462
>>>     #23 0xffffffff81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
>>>     #24 0x0000000000000000 in ?? ()
>>>
>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>> setting dom0 as a backend when running in dom0.
>>>
>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" devices")
>>
>>
>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>> am not mistaken, the said commit just moves the code in the context
>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>> introduced before.
>>
>>
>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>> ---
>>>    drivers/xen/grant-dma-ops.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>> index 76f6f26265a3..29ed27ac450e 100644
>>> --- a/drivers/xen/grant-dma-ops.c
>>> +++ b/drivers/xen/grant-dma-ops.c
>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
>>>    	if (np) {
>>>    		ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>>>    		of_node_put(np);
>>> -	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) {
>>> +	} else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>> +		    xen_pv_domain()) &&
>>> +		   !xen_initial_domain()) {
>>
>> The commit lgtm, just one note:
>>
>>
>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>> as I assume the same issue could happen on Arm with DT (although there
>> we don't guess the backend's domid, we read it from DT and quite
>> unlikely we get Dom0 being in Dom0 with correct DT).
>>
>> Something like:
>>
>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>> virtio_device *dev)
>>    {
>>           domid_t backend_domid;
>>
>> +       /* Xen grant DMA ops are not used when running as initial domain */
>> +       if (xen_initial_domain())
>> +               return false;
>> +
>>           if (!xen_grant_init_backend_domid(dev->dev.parent,
>> &backend_domid)) {
>>                   xen_grant_setup_dma_ops(dev->dev.parent, backend_domid);
>>                   return true;
>> (END)
>>
>>
>>
>> If so, that commit subject would need to be updated accordingly.
>>
>> Let's see what other reviewers will say.
> 
> This doesn't work in all cases. Imagine using PCI Passthrough to assign
> a "physical" virtio device to a domU. The domU will run into the same
> error, right?
> 
> The problem is that we need a way for the virtio backend to advertise
> its ability of handling grants. Right now we only have a way to do with
> that with device tree on ARM. On x86, we only have
> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
> "physical" virtio devices. Note that in this case we are fixing a
> nested-virtualization bug, but there are actually physical
> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
> break those too.

In case you want virtio device passthrough, you shouldn't use a kernel
built with CONFIG_XEN_VIRTIO_FORCE_GRANT.

And supporting passing through virtio devices of the host to pv-domUs is
a security risk anyway.

We _could_ drop the requirement of the backend needing to set
VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
handling for all guests. For this to work xen_virtio_restricted_mem_acc()
would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
Maybe we'd want to enable that possibility via a boot parameter?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 07.07.23 10:04, Juergen Gross wrote:

Hello Juergen


> Re-reading the whole thread again ...
> 
> On 29.06.23 03:00, Stefano Stabellini wrote:
>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>
>>>
>>> Hello Petr
>>>
>>>
>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>>> eventually results in a hang during the boot.
>>>>
>>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>>> makes no progress:
>>>>
>>>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, 
>>>> len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 
>>>> <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>>     #1  0xffffffff817086b7 in virtqueue_get_buf 
>>>> (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) 
>>>> at ../drivers/virtio/virtio_ring.c:2333
>>>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized 
>>>> out>, port_id=0xffffffff, event=0x0, value=0x1) at 
>>>> ../drivers/char/virtio_console.c:562
>>>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized 
>>>> out>, port_id=<optimized out>, event=<optimized out>, 
>>>> value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>>     #4  0xffffffff817618b1 in virtcons_probe 
>>>> (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>>     #5  0xffffffff81707117 in virtio_dev_probe 
>>>> (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>>     #6  0xffffffff8198e348 in call_driver_probe 
>>>> (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 
>>>> <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, 
>>>> drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/dd.c:658
>>>>     #8  0xffffffff8198e58f in __driver_probe_device 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, 
>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>     #9  0xffffffff8198e65a in driver_probe_device 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, 
>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>     #10 0xffffffff8198e832 in __driver_attach 
>>>> (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) 
>>>> at ../drivers/base/dd.c:1216
>>>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, 
>>>> start=start@entry=0x0 <fixed_percpu_data>, 
>>>> data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at 
>>>> ../drivers/base/bus.c:368
>>>>     #12 0xffffffff8198db65 in driver_attach 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/dd.c:1233
>>>>     #13 0xffffffff8198d207 in bus_add_driver 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/bus.c:673
>>>>     #14 0xffffffff8198f550 in driver_register 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/driver.c:246
>>>>     #15 0xffffffff81706b47 in register_virtio_driver 
>>>> (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/virtio/virtio.c:357
>>>>     #16 0xffffffff832cd34b in virtio_console_init () at 
>>>> ../drivers/char/virtio_console.c:2258
>>>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 
>>>> <virtio_console_init>) at ../init/main.c:1246
>>>>     #18 0xffffffff83277293 in do_initcall_level 
>>>> (command_line=0xffff888003e2f900 "root", level=0x6) at 
>>>> ../init/main.c:1319
>>>>     #19 do_initcalls () at ../init/main.c:1335
>>>>     #20 do_basic_setup () at ../init/main.c:1354
>>>>     #21 kernel_init_freeable () at ../init/main.c:1571
>>>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) 
>>>> at ../init/main.c:1462
>>>>     #23 0xffffffff81001f49 in ret_from_fork () at 
>>>> ../arch/x86/entry/entry_64.S:308
>>>>     #24 0x0000000000000000 in ?? ()
>>>>
>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>> setting dom0 as a backend when running in dom0.
>>>>
>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of 
>>>> "xen-grant-dma" devices")
>>>
>>>
>>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>>> am not mistaken, the said commit just moves the code in the context
>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>> introduced before.
>>>
>>>
>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>>> ---
>>>>    drivers/xen/grant-dma-ops.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>> --- a/drivers/xen/grant-dma-ops.c
>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct 
>>>> device *dev,
>>>>        if (np) {
>>>>            ret = xen_dt_grant_init_backend_domid(dev, np, 
>>>> backend_domid);
>>>>            of_node_put(np);
>>>> -    } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || 
>>>> xen_pv_domain()) {
>>>> +    } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>> +            xen_pv_domain()) &&
>>>> +           !xen_initial_domain()) {
>>>
>>> The commit lgtm, just one note:
>>>
>>>
>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>>> as I assume the same issue could happen on Arm with DT (although there
>>> we don't guess the backend's domid, we read it from DT and quite
>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>
>>> Something like:
>>>
>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>> virtio_device *dev)
>>>    {
>>>           domid_t backend_domid;
>>>
>>> +       /* Xen grant DMA ops are not used when running as initial 
>>> domain */
>>> +       if (xen_initial_domain())
>>> +               return false;
>>> +
>>>           if (!xen_grant_init_backend_domid(dev->dev.parent,
>>> &backend_domid)) {
>>>                   xen_grant_setup_dma_ops(dev->dev.parent, 
>>> backend_domid);
>>>                   return true;
>>> (END)
>>>
>>>
>>>
>>> If so, that commit subject would need to be updated accordingly.
>>>
>>> Let's see what other reviewers will say.
>>
>> This doesn't work in all cases. Imagine using PCI Passthrough to assign
>> a "physical" virtio device to a domU. The domU will run into the same
>> error, right?
>>
>> The problem is that we need a way for the virtio backend to advertise
>> its ability of handling grants. Right now we only have a way to do with
>> that with device tree on ARM. On x86, we only have
>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
>> "physical" virtio devices. Note that in this case we are fixing a
>> nested-virtualization bug, but there are actually physical
>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
>> break those too.
> 
> In case you want virtio device passthrough, you shouldn't use a kernel
> built with CONFIG_XEN_VIRTIO_FORCE_GRANT.
> 
> And supporting passing through virtio devices of the host to pv-domUs is
> a security risk anyway.
> 
> We _could_ drop the requirement of the backend needing to set
> VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
> handling for all guests. For this to work xen_virtio_restricted_mem_acc()
> would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
> Maybe we'd want to enable that possibility via a boot parameter?


Maybe, yes. I don't see at the moment why this won't work.

At the same time I wonder, could we just modify xen_pv_init_platform() 
to call virtio_no_restricted_mem_acc() if forcibly disabled by boot 
parameter irrespective of VIRTIO_F_ACCESS_PLATFORM presence?


> 
> 
> Juergen

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 8860 bytes --]

On 07.07.23 10:00, Oleksandr Tyshchenko wrote:
> 
> 
> On 07.07.23 10:04, Juergen Gross wrote:
> 
> Hello Juergen
> 
> 
>> Re-reading the whole thread again ...
>>
>> On 29.06.23 03:00, Stefano Stabellini wrote:
>>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>>
>>>>
>>>> Hello Petr
>>>>
>>>>
>>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>>>> eventually results in a hang during the boot.
>>>>>
>>>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>>>> makes no progress:
>>>>>
>>>>>      #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400,
>>>>> len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0
>>>>> <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>>>      #1  0xffffffff817086b7 in virtqueue_get_buf
>>>>> (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94)
>>>>> at ../drivers/virtio/virtio_ring.c:2333
>>>>>      #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized
>>>>> out>, port_id=0xffffffff, event=0x0, value=0x1) at
>>>>> ../drivers/char/virtio_console.c:562
>>>>>      #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized
>>>>> out>, port_id=<optimized out>, event=<optimized out>,
>>>>> value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>>>      #4  0xffffffff817618b1 in virtcons_probe
>>>>> (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>>>      #5  0xffffffff81707117 in virtio_dev_probe
>>>>> (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>>>      #6  0xffffffff8198e348 in call_driver_probe
>>>>> (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0
>>>>> <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>>      #7  really_probe (dev=dev@entry=0xffff88800585e810,
>>>>> drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:658
>>>>>      #8  0xffffffff8198e58f in __driver_probe_device
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>>      #9  0xffffffff8198e65a in driver_probe_device
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>>      #10 0xffffffff8198e832 in __driver_attach
>>>>> (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>)
>>>>> at ../drivers/base/dd.c:1216
>>>>>      #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>,
>>>>> start=start@entry=0x0 <fixed_percpu_data>,
>>>>> data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>          fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at
>>>>> ../drivers/base/bus.c:368
>>>>>      #12 0xffffffff8198db65 in driver_attach
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:1233
>>>>>      #13 0xffffffff8198d207 in bus_add_driver
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/bus.c:673
>>>>>      #14 0xffffffff8198f550 in driver_register
>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/driver.c:246
>>>>>      #15 0xffffffff81706b47 in register_virtio_driver
>>>>> (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/virtio/virtio.c:357
>>>>>      #16 0xffffffff832cd34b in virtio_console_init () at
>>>>> ../drivers/char/virtio_console.c:2258
>>>>>      #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0
>>>>> <virtio_console_init>) at ../init/main.c:1246
>>>>>      #18 0xffffffff83277293 in do_initcall_level
>>>>> (command_line=0xffff888003e2f900 "root", level=0x6) at
>>>>> ../init/main.c:1319
>>>>>      #19 do_initcalls () at ../init/main.c:1335
>>>>>      #20 do_basic_setup () at ../init/main.c:1354
>>>>>      #21 kernel_init_freeable () at ../init/main.c:1571
>>>>>      #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>)
>>>>> at ../init/main.c:1462
>>>>>      #23 0xffffffff81001f49 in ret_from_fork () at
>>>>> ../arch/x86/entry/entry_64.S:308
>>>>>      #24 0x0000000000000000 in ?? ()
>>>>>
>>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>>> setting dom0 as a backend when running in dom0.
>>>>>
>>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of
>>>>> "xen-grant-dma" devices")
>>>>
>>>>
>>>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>>>> am not mistaken, the said commit just moves the code in the context
>>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>>> introduced before.
>>>>
>>>>
>>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>>>> ---
>>>>>     drivers/xen/grant-dma-ops.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>>> --- a/drivers/xen/grant-dma-ops.c
>>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct
>>>>> device *dev,
>>>>>         if (np) {
>>>>>             ret = xen_dt_grant_init_backend_domid(dev, np,
>>>>> backend_domid);
>>>>>             of_node_put(np);
>>>>> -    } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>> xen_pv_domain()) {
>>>>> +    } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>> +            xen_pv_domain()) &&
>>>>> +           !xen_initial_domain()) {
>>>>
>>>> The commit lgtm, just one note:
>>>>
>>>>
>>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>>>> as I assume the same issue could happen on Arm with DT (although there
>>>> we don't guess the backend's domid, we read it from DT and quite
>>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>>
>>>> Something like:
>>>>
>>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>>> virtio_device *dev)
>>>>     {
>>>>            domid_t backend_domid;
>>>>
>>>> +       /* Xen grant DMA ops are not used when running as initial
>>>> domain */
>>>> +       if (xen_initial_domain())
>>>> +               return false;
>>>> +
>>>>            if (!xen_grant_init_backend_domid(dev->dev.parent,
>>>> &backend_domid)) {
>>>>                    xen_grant_setup_dma_ops(dev->dev.parent,
>>>> backend_domid);
>>>>                    return true;
>>>> (END)
>>>>
>>>>
>>>>
>>>> If so, that commit subject would need to be updated accordingly.
>>>>
>>>> Let's see what other reviewers will say.
>>>
>>> This doesn't work in all cases. Imagine using PCI Passthrough to assign
>>> a "physical" virtio device to a domU. The domU will run into the same
>>> error, right?
>>>
>>> The problem is that we need a way for the virtio backend to advertise
>>> its ability of handling grants. Right now we only have a way to do with
>>> that with device tree on ARM. On x86, we only have
>>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
>>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
>>> "physical" virtio devices. Note that in this case we are fixing a
>>> nested-virtualization bug, but there are actually physical
>>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
>>> break those too.
>>
>> In case you want virtio device passthrough, you shouldn't use a kernel
>> built with CONFIG_XEN_VIRTIO_FORCE_GRANT.
>>
>> And supporting passing through virtio devices of the host to pv-domUs is
>> a security risk anyway.
>>
>> We _could_ drop the requirement of the backend needing to set
>> VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
>> handling for all guests. For this to work xen_virtio_restricted_mem_acc()
>> would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
>> Maybe we'd want to enable that possibility via a boot parameter?
> 
> 
> Maybe, yes. I don't see at the moment why this won't work.
> 
> At the same time I wonder, could we just modify xen_pv_init_platform()
> to call virtio_no_restricted_mem_acc() if forcibly disabled by boot
> parameter irrespective of VIRTIO_F_ACCESS_PLATFORM presence?

This wouldn't work for the case where a host virtio device is passed through
to the pv domU and at the same time another virtio device is using dom0 as a
backend. I think we should use grants if possible.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 07.07.23 11:11, Juergen Gross wrote:

Hello Juergen


> On 07.07.23 10:00, Oleksandr Tyshchenko wrote:
>>
>>
>> On 07.07.23 10:04, Juergen Gross wrote:
>>
>> Hello Juergen
>>
>>
>>> Re-reading the whole thread again ...
>>>
>>> On 29.06.23 03:00, Stefano Stabellini wrote:
>>>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>>>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>>>
>>>>>
>>>>> Hello Petr
>>>>>
>>>>>
>>>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>>>> devices (all x86_64), dom0 tries to establish a grant for itself 
>>>>>> which
>>>>>> eventually results in a hang during the boot.
>>>>>>
>>>>>> The backtrace looks as follows, the while loop in 
>>>>>> __send_control_msg()
>>>>>> makes no progress:
>>>>>>
>>>>>>      #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400,
>>>>>> len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0
>>>>>> <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>>>>      #1  0xffffffff817086b7 in virtqueue_get_buf
>>>>>> (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94)
>>>>>> at ../drivers/virtio/virtio_ring.c:2333
>>>>>>      #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized
>>>>>> out>, port_id=0xffffffff, event=0x0, value=0x1) at
>>>>>> ../drivers/char/virtio_console.c:562
>>>>>>      #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized
>>>>>> out>, port_id=<optimized out>, event=<optimized out>,
>>>>>> value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>>>>      #4  0xffffffff817618b1 in virtcons_probe
>>>>>> (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>>>>      #5  0xffffffff81707117 in virtio_dev_probe
>>>>>> (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>>>>      #6  0xffffffff8198e348 in call_driver_probe
>>>>>> (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0
>>>>>> <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>>>      #7  really_probe (dev=dev@entry=0xffff88800585e810,
>>>>>> drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>>> ../drivers/base/dd.c:658
>>>>>>      #8  0xffffffff8198e58f in __driver_probe_device
>>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>>>      #9  0xffffffff8198e65a in driver_probe_device
>>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>>>      #10 0xffffffff8198e832 in __driver_attach
>>>>>> (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>)
>>>>>> at ../drivers/base/dd.c:1216
>>>>>>      #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>,
>>>>>> start=start@entry=0x0 <fixed_percpu_data>,
>>>>>> data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>>>          fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at
>>>>>> ../drivers/base/bus.c:368
>>>>>>      #12 0xffffffff8198db65 in driver_attach
>>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>>> ../drivers/base/dd.c:1233
>>>>>>      #13 0xffffffff8198d207 in bus_add_driver
>>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>>> ../drivers/base/bus.c:673
>>>>>>      #14 0xffffffff8198f550 in driver_register
>>>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>>> ../drivers/base/driver.c:246
>>>>>>      #15 0xffffffff81706b47 in register_virtio_driver
>>>>>> (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at
>>>>>> ../drivers/virtio/virtio.c:357
>>>>>>      #16 0xffffffff832cd34b in virtio_console_init () at
>>>>>> ../drivers/char/virtio_console.c:2258
>>>>>>      #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0
>>>>>> <virtio_console_init>) at ../init/main.c:1246
>>>>>>      #18 0xffffffff83277293 in do_initcall_level
>>>>>> (command_line=0xffff888003e2f900 "root", level=0x6) at
>>>>>> ../init/main.c:1319
>>>>>>      #19 do_initcalls () at ../init/main.c:1335
>>>>>>      #20 do_basic_setup () at ../init/main.c:1354
>>>>>>      #21 kernel_init_freeable () at ../init/main.c:1571
>>>>>>      #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>)
>>>>>> at ../init/main.c:1462
>>>>>>      #23 0xffffffff81001f49 in ret_from_fork () at
>>>>>> ../arch/x86/entry/entry_64.S:308
>>>>>>      #24 0x0000000000000000 in ?? ()
>>>>>>
>>>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>>>> setting dom0 as a backend when running in dom0.
>>>>>>
>>>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of
>>>>>> "xen-grant-dma" devices")
>>>>>
>>>>>
>>>>> I am not 100% sure whether the Fixes tag points to precise commit. 
>>>>> If I
>>>>> am not mistaken, the said commit just moves the code in the context
>>>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>>>> introduced before.
>>>>>
>>>>>
>>>>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>>>>> ---
>>>>>>     drivers/xen/grant-dma-ops.c | 4 +++-
>>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/xen/grant-dma-ops.c 
>>>>>> b/drivers/xen/grant-dma-ops.c
>>>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>>>> --- a/drivers/xen/grant-dma-ops.c
>>>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct
>>>>>> device *dev,
>>>>>>         if (np) {
>>>>>>             ret = xen_dt_grant_init_backend_domid(dev, np,
>>>>>> backend_domid);
>>>>>>             of_node_put(np);
>>>>>> -    } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>>> xen_pv_domain()) {
>>>>>> +    } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>>>> +            xen_pv_domain()) &&
>>>>>> +           !xen_initial_domain()) {
>>>>>
>>>>> The commit lgtm, just one note:
>>>>>
>>>>>
>>>>> I would even bail out early in xen_virtio_restricted_mem_acc() 
>>>>> instead,
>>>>> as I assume the same issue could happen on Arm with DT (although there
>>>>> we don't guess the backend's domid, we read it from DT and quite
>>>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>>>
>>>>> Something like:
>>>>>
>>>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>>>> virtio_device *dev)
>>>>>     {
>>>>>            domid_t backend_domid;
>>>>>
>>>>> +       /* Xen grant DMA ops are not used when running as initial
>>>>> domain */
>>>>> +       if (xen_initial_domain())
>>>>> +               return false;
>>>>> +
>>>>>            if (!xen_grant_init_backend_domid(dev->dev.parent,
>>>>> &backend_domid)) {
>>>>>                    xen_grant_setup_dma_ops(dev->dev.parent,
>>>>> backend_domid);
>>>>>                    return true;
>>>>> (END)
>>>>>
>>>>>
>>>>>
>>>>> If so, that commit subject would need to be updated accordingly.
>>>>>
>>>>> Let's see what other reviewers will say.
>>>>
>>>> This doesn't work in all cases. Imagine using PCI Passthrough to assign
>>>> a "physical" virtio device to a domU. The domU will run into the same
>>>> error, right?
>>>>
>>>> The problem is that we need a way for the virtio backend to advertise
>>>> its ability of handling grants. Right now we only have a way to do with
>>>> that with device tree on ARM. On x86, we only have
>>>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
>>>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
>>>> "physical" virtio devices. Note that in this case we are fixing a
>>>> nested-virtualization bug, but there are actually physical
>>>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
>>>> break those too.
>>>
>>> In case you want virtio device passthrough, you shouldn't use a kernel
>>> built with CONFIG_XEN_VIRTIO_FORCE_GRANT.
>>>
>>> And supporting passing through virtio devices of the host to pv-domUs is
>>> a security risk anyway.
>>>
>>> We _could_ drop the requirement of the backend needing to set
>>> VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
>>> handling for all guests. For this to work 
>>> xen_virtio_restricted_mem_acc()
>>> would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
>>> Maybe we'd want to enable that possibility via a boot parameter?
>>
>>
>> Maybe, yes. I don't see at the moment why this won't work.
>>
>> At the same time I wonder, could we just modify xen_pv_init_platform()
>> to call virtio_no_restricted_mem_acc() if forcibly disabled by boot
>> parameter irrespective of VIRTIO_F_ACCESS_PLATFORM presence?
> 
> This wouldn't work for the case where a host virtio device is passed 
> through
> to the pv domU and at the same time another virtio device is using dom0 
> as a
> backend. I think we should use grants if possible.

Indeed, I missed that possible scenario. I agree with the explanations, 
thanks.


> 
> 
> Juergen
> 

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 9664 bytes --]

On Fri, 7 Jul 2023:
> On 07.07.23 16:42, Roger Pau Monné wrote:
> > On Fri, Jul 07, 2023 at 04:10:14PM +0200, Juergen Gross wrote:
> > > On 07.07.23 11:50, Roger Pau Monné wrote:
> > > > On Fri, Jul 07, 2023 at 06:38:48AM +0200, Juergen Gross wrote:
> > > > > On 06.07.23 23:49, Stefano Stabellini wrote:
> > > > > > On Thu, 6 Jul 2023, Roger Pau Monné wrote:
> > > > > > > On Wed, Jul 05, 2023 at 03:41:10PM -0700, Stefano Stabellini
> > > > > > > wrote:
> > > > > > > > On Wed, 5 Jul 2023, Roger Pau Monné wrote:
> > > > > > > > > On Tue, Jul 04, 2023 at 08:14:59PM +0300, Oleksandr Tyshchenko
> > > > > > > > > wrote:
> > > > > > > > > > Part 2 (clarification):
> > > > > > > > > > 
> > > > > > > > > > I think using a special config space register in the root
> > > > > > > > > > complex would
> > > > > > > > > > not be terrible in terms of guest changes because it is easy
> > > > > > > > > > to
> > > > > > > > > > introduce a new root complex driver in Linux and other OSes.
> > > > > > > > > > The root
> > > > > > > > > > complex would still be ECAM compatible so the regular ECAM
> > > > > > > > > > driver would
> > > > > > > > > > still work. A new driver would only be necessary if you want
> > > > > > > > > > to be able
> > > > > > > > > > to access the special config space register.
> > > > > > > > > 
> > > > > > > > > I'm slightly worry of this approach, we end up modifying a
> > > > > > > > > root
> > > > > > > > > complex emulation in order to avoid modifying a PCI device
> > > > > > > > > emulation
> > > > > > > > > on QEMU, not sure that's a good trade off.
> > > > > > > > > 
> > > > > > > > > Note also that different architectures will likely have
> > > > > > > > > different root
> > > > > > > > > complex, and so you might need to modify several of them, plus
> > > > > > > > > then
> > > > > > > > > arrange the PCI layout correctly in order to have the proper
> > > > > > > > > hierarchy
> > > > > > > > > so that devices belonging to different driver domains are
> > > > > > > > > assigned to
> > > > > > > > > different bridges.
> > > > > > > > 
> > > > > > > > I do think that adding something to the PCI conf register
> > > > > > > > somewhere is
> > > > > > > > the best option because it is not dependent on ACPI and it is
> > > > > > > > not
> > > > > > > > dependent on xenstore both of which are very undesirable.
> > > > > > > > 
> > > > > > > > I am not sure where specifically is the best place. These are 3
> > > > > > > > ideas
> > > > > > > > we came up with:
> > > > > > > > 1. PCI root complex
> > > > > > > > 2. a register on the device itself
> > > > > > > > 3. a new capability of the device
> > > > > > > > 4. add one extra dummy PCI device for the sole purpose of
> > > > > > > > exposing the
> > > > > > > >       grants capability
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Looking at the spec, there is a way to add a vendor-specific
> > > > > > > > capability
> > > > > > > > (cap_vndr = 0x9). Could we use that? It doesn't look like it is
> > > > > > > > used
> > > > > > > > today, Linux doesn't parse it.
> > > > > > > 
> > > > > > > I did wonder the same from a quick look at the spec.  There's
> > > > > > > however
> > > > > > > a text in the specification that says:
> > > > > > > 
> > > > > > > "The driver SHOULD NOT use the Vendor data capability except for
> > > > > > > debugging and reporting purposes."
> > > > > > > 
> > > > > > > So we would at least need to change that because the capability
> > > > > > > would
> > > > > > > then be used by other purposes different than debugging and
> > > > > > > reporting.
> > > > > > > 
> > > > > > > Seems like a minor adjustment, so might we worth asking upstream
> > > > > > > about
> > > > > > > their opinion, and to get a conversation started.
> > > > > > 
> > > > > > Wait, wouldn't this use-case fall under "reporting" ? It is exactly
> > > > > > what
> > > > > > we are doing, right?
> > > > > 
> > > > > I'd understand "reporting" as e.g. logging, transferring statistics,
> > > > > ...
> > > > > 
> > > > > We'd like to use it for configuration purposes.
> > > > 
> > > > I've also read it that way.
> > > > 
> > > > > Another idea would be to enhance the virtio IOMMU device to suit our
> > > > > needs:
> > > > > we could add the domid as another virtio IOMMU device capability and
> > > > > (for now)
> > > > > use bypass mode for all "productive" devices.
> > > > 
> > > > If we have to start adding capabilties, won't it be easier to just add
> > > > it to the each device instead of adding it to virtio IOMMU.  Or is the
> > > > parsing of capabilities device specific, and hence we would have to
> > > > implement such parsing for each device?  I would expect some
> > > > capabilities are shared between all devices, and a Xen capability could
> > > > be one of those.
> > > 
> > > Have a look at [1], which is describing the common device config layout.
> > > The problem here is that we'd need to add the domid after the queue
> > > specific
> > > data, resulting in a mess if further queue fields would be added later.
> > > 
> > > We could try that, of course.
> > 
> > Right, we must make it part of the standard if we modify
> > virtio_pci_common_cfg, or else newly added fields would overlap the
> > Xen specific one.
> > 
> > Would it be possible to signal Xen-grants support in the
> > `device_feature` field, and then expose it from a vendor capability?
> > IOW, would it be possible to add a Xen-specific hook in the parsing of
> > virtio_pci_common_cfg that would then fetch additional data from a
> > capability?
> 
> TBH, I don't know. It might require some changes in the central parsing
> logic, but this shouldn't be too hard to do.
> 
> > That would likely be less intrusive than adding a new Xen-specific
> > field to virtio_pci_common_cfg while still allowing us to do Xen
> > specific configuration for all VirtIO devices.
> 
> In case we want to go that route, this should be in a new "platform config"
> capability, which might be just another form of a vendor capability.
 
I think this is the best idea. We should look into this.


> > > > > Later we could even add grant-V3 support to Xen and to the virtio
> > > > > IOMMU device
> > > > > (see my last year Xen Summit design session). This could be usable for
> > > > > disaggregated KVM setups, too, so I believe there is a chance to get
> > > > > this
> > > > > accepted.
> > > > > 
> > > > > > > > > > **********
> > > > > > > > > > What do you think about it? Are there any pitfalls, etc?
> > > > > > > > > > This also requires
> > > > > > > > > > system changes, but at least without virtio spec changes.
> > > > > > > > > 
> > > > > > > > > Why are we so reluctant to add spec changes?  I understand
> > > > > > > > > this might
> > > > > > > > > take time an effort, but it's the only way IMO to build a
> > > > > > > > > sustainable
> > > > > > > > > VirtIO Xen implementation.  Did we already attempt to
> > > > > > > > > negotiate with
> > > > > > > > > Oasis Xen related spec changes and those where refused?
> > > > > > > > 
> > > > > > > > That's because spec changes can be very slow. This is a bug that
> > > > > > > > we need
> > > > > > > > a relatively quick solution for and waiting 12-24 months for a
> > > > > > > > spec
> > > > > > > > update is not realistic.
> > > > > > > > 
> > > > > > > > I think a spec change would be best as a long term solution. We
> > > > > > > > also
> > > > > > > > need a short term solution. The short term solution doesn't have
> > > > > > > > to be
> > > > > > > > ideal but it has to work now.
> > > > > > > 
> > > > > > > My fear with such approach is that once a bodge is in place people
> > > > > > > move on to other stuff and this never gets properly fixed.
> > > > > > > 
> > > > > > > I know this might not be a well received opinion, but it would be
> > > > > > > better if such bodge is kept in each interested party patchqueue
> > > > > > > for
> > > > > > > the time being, until a proper solution is implemented.  That way
> > > > > > > there's an interest from parties into properly fixing it upstream.
> > > > > > 
> > > > > > Unfortunately we are in the situation where we have an outstanding
> > > > > > upstream bug, so we have to take action one way or the other.
> > > > > 
> > > > > The required virtio IOMMU device modification would be rather small,
> > > > > so
> > > > > adding it maybe under a CONFIG option defaulting to off might be
> > > > > acceptable.
> > > > 
> > > > Would you then do the grant allocation as part of virtio IOMMU?
> > > 
> > > Long term, maybe. Do you remember my Grant-V3 design session last year?
> > > Being
> > > able to reuse the same layout for virtio IOMMU was one of the basic ideas
> > > for
> > > that layout (this would need some heavy work on the virtio IOMMU frontend
> > > and
> > > backend, of course).
> > 
> > While this might well be the best option, do we have anyone with the
> > time and expertise to work on this?  I might be wrong, but it seems
> > like a huge task.
> 
> As a background project I'd like to pursue it. OTOH I'm not sure how much time
> I could spend on it.

Not only it is complex but also has severe implications in terms of
security, safety, and needs to interact with potential virtual IOMMUs in
the guest (virtual IOMMUs to expose another IOMMU stage of translation
in the guest.)

This is definitely not simple.

At that point I would feel more confident in a solution that uses ACPI
tables to add the necessary information the same way we use Device Tree
to do it on ARM. Keep in mind that if an existing ACPI table doesn't
have the fields that we need, we can introduce a new ACPI table.