[Qemu-devel] [PATCH 0/6] Xen stubdom support

[Qemu-devel] [PATCH 0/6] Xen stubdom support

[Jason Andryuk]

Xen supports running QEMU in a dedicated service vm - a stub domain or
stubdom.  QEMU is then isolated outside of the privileged Domain-0.

When running in a stubdom, there are a few changes needed for QEMU.  On
older Xen versions, the default ioreq server needs to have the stubdom's
domid specified.  The stubdom doesn't run PV backends, so that
initialization code can be skipped.  Stubdom's don't support MSI-X, so
that PCI capability must be hidden from passed through devices.

Stubdom mode is enabled by the new -xen-stubdom flag.

Jason Andryuk (5):
  xen: Introduce -xen-stubdom option
  xen: Move xenstore initialization to common location
  xen: Skip backend initialization for stubdom
  xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen
  xen-pt: Hide MSI-X from xen stubdoms

Simon Gaiser (1):
  xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

 hw/i386/xen/xen-hvm.c       | 22 ++++++++++++++++------
 hw/xen/xen-legacy-backend.c |  8 --------
 hw/xen/xen_pt.c             | 10 +++++++---
 hw/xen/xen_pt_config_init.c |  3 +++
 include/hw/xen/xen.h        |  6 ++++++
 include/hw/xen/xen_common.h |  5 +++++
 qemu-options.hx             |  7 +++++++
 vl.c                        |  8 ++++++++
 8 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.20.1

[PATCH 0/6] Xen stubdom support

[Jason Andryuk]

Xen supports running QEMU in a dedicated service vm - a stub domain or
stubdom.  QEMU is then isolated outside of the privileged Domain-0.

When running in a stubdom, there are a few changes needed for QEMU.  On
older Xen versions, the default ioreq server needs to have the stubdom's
domid specified.  The stubdom doesn't run PV backends, so that
initialization code can be skipped.  Stubdom's don't support MSI-X, so
that PCI capability must be hidden from passed through devices.

Stubdom mode is enabled by the new -xen-stubdom flag.

Jason Andryuk (5):
  xen: Introduce -xen-stubdom option
  xen: Move xenstore initialization to common location
  xen: Skip backend initialization for stubdom
  xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen
  xen-pt: Hide MSI-X from xen stubdoms

Simon Gaiser (1):
  xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

 hw/i386/xen/xen-hvm.c       | 22 ++++++++++++++++------
 hw/xen/xen-legacy-backend.c |  8 --------
 hw/xen/xen_pt.c             | 10 +++++++---
 hw/xen/xen_pt_config_init.c |  3 +++
 include/hw/xen/xen.h        |  6 ++++++
 include/hw/xen/xen_common.h |  5 +++++
 qemu-options.hx             |  7 +++++++
 vl.c                        |  8 ++++++++
 8 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
There are a few differences when running in a stubdom compared to dom0.
Add the -xen-stubdom option to select this mode at runtime.  The default
is off.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 include/hw/xen/xen.h | 6 ++++++
 qemu-options.hx      | 7 +++++++
 vl.c                 | 8 ++++++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index ba039c146d..fed3611623 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -21,6 +21,7 @@ enum xen_mode {
 extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 extern bool xen_domid_restrict;
+extern bool xen_stubdom;
 
 extern bool xen_allowed;
 
@@ -29,6 +30,11 @@ static inline bool xen_enabled(void)
     return xen_allowed;
 }
 
+static inline bool xen_stubdom_enabled(void)
+{
+    return xen_stubdom;
+}
+
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
diff --git a/qemu-options.hx b/qemu-options.hx
index 1cf9aac1fe..ba56c3dd9a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3386,6 +3386,10 @@ DEF("xen-domid-restrict", 0, QEMU_OPTION_xen_domid_restrict,
     "                        to specified domain id. (Does not affect\n"
     "                        xenpv machine type).\n",
     QEMU_ARCH_ALL)
+DEF("xen-stubdom", 0, QEMU_OPTION_xen_stubdom,
+    "-xen-stubdom    specify QEMU is running in a stubdom, so certain\n"
+    "                behavior changes. (Does not affect xenpv machine type).\n",
+    QEMU_ARCH_ALL)
 STEXI
 @item -xen-domid @var{id}
 @findex -xen-domid
@@ -3396,6 +3400,9 @@ Attach to existing xen domain.
 libxl will use this when starting QEMU (XEN only).
 @findex -xen-domid-restrict
 Restrict set of available xen operations to specified domain id (XEN only).
+@findex -xen-stubdom
+@item -xen-stubdom
+Run qemu in stubdom-mode (XEN only).
 ETEXI
 
 DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \
diff --git a/vl.c b/vl.c
index 4a350de5cd..0d04319d9b 100644
--- a/vl.c
+++ b/vl.c
@@ -206,6 +206,7 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 bool xen_domid_restrict;
+bool xen_stubdom;
 
 static int has_defaults = 1;
 static int default_serial = 1;
@@ -3796,6 +3797,13 @@ int main(int argc, char **argv, char **envp)
                 }
                 xen_domid_restrict = true;
                 break;
+            case QEMU_OPTION_xen_stubdom:
+                if (!(xen_available())) {
+                    error_report("Option not supported for this target");
+                    exit(1);
+                }
+                xen_stubdom = true;
+                break;
             case QEMU_OPTION_trace:
                 g_free(trace_file);
                 trace_file = trace_opt_parse(optarg);
-- 
2.20.1

[PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
There are a few differences when running in a stubdom compared to dom0.
Add the -xen-stubdom option to select this mode at runtime.  The default
is off.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 include/hw/xen/xen.h | 6 ++++++
 qemu-options.hx      | 7 +++++++
 vl.c                 | 8 ++++++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index ba039c146d..fed3611623 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -21,6 +21,7 @@ enum xen_mode {
 extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 extern bool xen_domid_restrict;
+extern bool xen_stubdom;
 
 extern bool xen_allowed;
 
@@ -29,6 +30,11 @@ static inline bool xen_enabled(void)
     return xen_allowed;
 }
 
+static inline bool xen_stubdom_enabled(void)
+{
+    return xen_stubdom;
+}
+
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 void xen_piix3_set_irq(void *opaque, int irq_num, int level);
 void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
diff --git a/qemu-options.hx b/qemu-options.hx
index 1cf9aac1fe..ba56c3dd9a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3386,6 +3386,10 @@ DEF("xen-domid-restrict", 0, QEMU_OPTION_xen_domid_restrict,
     "                        to specified domain id. (Does not affect\n"
     "                        xenpv machine type).\n",
     QEMU_ARCH_ALL)
+DEF("xen-stubdom", 0, QEMU_OPTION_xen_stubdom,
+    "-xen-stubdom    specify QEMU is running in a stubdom, so certain\n"
+    "                behavior changes. (Does not affect xenpv machine type).\n",
+    QEMU_ARCH_ALL)
 STEXI
 @item -xen-domid @var{id}
 @findex -xen-domid
@@ -3396,6 +3400,9 @@ Attach to existing xen domain.
 libxl will use this when starting QEMU (XEN only).
 @findex -xen-domid-restrict
 Restrict set of available xen operations to specified domain id (XEN only).
+@findex -xen-stubdom
+@item -xen-stubdom
+Run qemu in stubdom-mode (XEN only).
 ETEXI
 
 DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \
diff --git a/vl.c b/vl.c
index 4a350de5cd..0d04319d9b 100644
--- a/vl.c
+++ b/vl.c
@@ -206,6 +206,7 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 bool xen_domid_restrict;
+bool xen_stubdom;
 
 static int has_defaults = 1;
 static int default_serial = 1;
@@ -3796,6 +3797,13 @@ int main(int argc, char **argv, char **envp)
                 }
                 xen_domid_restrict = true;
                 break;
+            case QEMU_OPTION_xen_stubdom:
+                if (!(xen_available())) {
+                    error_report("Option not supported for this target");
+                    exit(1);
+                }
+                xen_stubdom = true;
+                break;
             case QEMU_OPTION_trace:
                 g_free(trace_file);
                 trace_file = trace_opt_parse(optarg);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

[Jason Andryuk]

For the xen stubdom case, we'll want xenstore initialized, but we'll
want to skip the rest of xen_be_init.  Move the initialization to
xen_hvm_init so we can conditionalize calling xen_be_init.

xs_domain_open() is deprecated for xs_open(0), so make the replacement
as well.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/i386/xen/xen-hvm.c       | 8 ++++++++
 hw/xen/xen-legacy-backend.c | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 2939122e7c..c20c4b27f6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     xen_bus_init();
 
+    xenstore = xs_open(0);
+    if (!xenstore) {
+        error_report("Can't connect to xenstored");
+        goto err;
+    }
+
+    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
+
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {
         error_report("xen backend core setup failed");
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 36fd1e9b09..bdf2fa917f 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -683,14 +683,6 @@ int xen_be_init(void)
 {
     xengnttab_handle *gnttabdev;
 
-    xenstore = xs_daemon_open();
-    if (!xenstore) {
-        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
-        return -1;
-    }
-
-    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
-
     if (xen_xc == NULL || xen_fmem == NULL) {
         /* Check if xen_init() have been called */
         goto err;
-- 
2.20.1

[PATCH 2/6] xen: Move xenstore initialization to common location

[Jason Andryuk]

For the xen stubdom case, we'll want xenstore initialized, but we'll
want to skip the rest of xen_be_init.  Move the initialization to
xen_hvm_init so we can conditionalize calling xen_be_init.

xs_domain_open() is deprecated for xs_open(0), so make the replacement
as well.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/i386/xen/xen-hvm.c       | 8 ++++++++
 hw/xen/xen-legacy-backend.c | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 2939122e7c..c20c4b27f6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     xen_bus_init();
 
+    xenstore = xs_open(0);
+    if (!xenstore) {
+        error_report("Can't connect to xenstored");
+        goto err;
+    }
+
+    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
+
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {
         error_report("xen backend core setup failed");
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 36fd1e9b09..bdf2fa917f 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -683,14 +683,6 @@ int xen_be_init(void)
 {
     xengnttab_handle *gnttabdev;
 
-    xenstore = xs_daemon_open();
-    if (!xenstore) {
-        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
-        return -1;
-    }
-
-    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
-
     if (xen_xc == NULL || xen_fmem == NULL) {
         /* Check if xen_init() have been called */
         goto err;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 3/6] xen: Skip backend initialization for stubdom

[Jason Andryuk]

When QEMU is running in a stubdom, it does not provide any
Paravirtualized backends.  Those still run in dom0 or another driver
domain.  Therefore we skip backend initialization (xen_bus_init and
xen_be_init) for the stubdom case.

Original patch by Anthony PERARD <anthony.perard@citrix.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/i386/xen/xen-hvm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index c20c4b27f6..4b62f070cb 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1485,8 +1485,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
     QLIST_INIT(&state->dev_list);
     device_listener_register(&state->device_listener);
 
-    xen_bus_init();
-
     xenstore = xs_open(0);
     if (!xenstore) {
         error_report("Can't connect to xenstored");
@@ -1495,12 +1493,16 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
 
-    /* Initialize backend core & drivers */
-    if (xen_be_init() != 0) {
-        error_report("xen backend core setup failed");
-        goto err;
+    if (!xen_stubdom_enabled()) {
+        xen_bus_init();
+
+        /* Initialize backend core & drivers */
+        if (xen_be_init() != 0) {
+            error_report("xen backend core setup failed");
+            goto err;
+        }
+        xen_be_register_common();
     }
-    xen_be_register_common();
 
     QLIST_INIT(&xen_physmap);
     xen_read_physmap(state);
-- 
2.20.1

[PATCH 3/6] xen: Skip backend initialization for stubdom

[Jason Andryuk]

When QEMU is running in a stubdom, it does not provide any
Paravirtualized backends.  Those still run in dom0 or another driver
domain.  Therefore we skip backend initialization (xen_bus_init and
xen_be_init) for the stubdom case.

Original patch by Anthony PERARD <anthony.perard@citrix.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/i386/xen/xen-hvm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index c20c4b27f6..4b62f070cb 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1485,8 +1485,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
     QLIST_INIT(&state->dev_list);
     device_listener_register(&state->device_listener);
 
-    xen_bus_init();
-
     xenstore = xs_open(0);
     if (!xenstore) {
         error_report("Can't connect to xenstored");
@@ -1495,12 +1493,16 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
 
-    /* Initialize backend core & drivers */
-    if (xen_be_init() != 0) {
-        error_report("xen backend core setup failed");
-        goto err;
+    if (!xen_stubdom_enabled()) {
+        xen_bus_init();
+
+        /* Initialize backend core & drivers */
+        if (xen_be_init() != 0) {
+            error_report("xen backend core setup failed");
+            goto err;
+        }
+        xen_be_register_common();
     }
-    xen_be_register_common();
 
     QLIST_INIT(&xen_physmap);
     xen_read_physmap(state);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 4/6] xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen

[Jason Andryuk]

When running in a stubdom, we have to inform the hypervisor that the
stubdom and not dom0 is handling the device model.  Explicitly created
ioreq servers are fine, but a call to HVM_PARAM_DM_DOMAIN is needed for
the default ioreq server.

Xen 4.12 removes the default ioreq server.  With that, Xen started
returning an error when setting HVM_PARAM_DM_DOMAIN.  Put the
HVM_PARAM_DM_DOMAIN call in the version compatibility header.  When we
fallback to the default ioreq server, issue the call and don't bother to
check the return value.

Original patch by Anthony PERARD <anthony.perard@citrix.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 include/hw/xen/xen_common.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 9a8155e172..f59f841a43 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -616,6 +616,11 @@ static inline void xen_create_ioreq_server(domid_t dom,
 
     *ioservid = 0;
     use_default_ioreq_server = true;
+
+    if (xen_stubdom_enabled()) {
+        xc_hvm_param_set(xen_xc, xen_domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
+    }
+
     trace_xen_default_ioreq_server();
 }
 
-- 
2.20.1

[PATCH 4/6] xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen

[Jason Andryuk]

When running in a stubdom, we have to inform the hypervisor that the
stubdom and not dom0 is handling the device model.  Explicitly created
ioreq servers are fine, but a call to HVM_PARAM_DM_DOMAIN is needed for
the default ioreq server.

Xen 4.12 removes the default ioreq server.  With that, Xen started
returning an error when setting HVM_PARAM_DM_DOMAIN.  Put the
HVM_PARAM_DM_DOMAIN call in the version compatibility header.  When we
fallback to the default ioreq server, issue the call and don't bother to
check the return value.

Original patch by Anthony PERARD <anthony.perard@citrix.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 include/hw/xen/xen_common.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 9a8155e172..f59f841a43 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -616,6 +616,11 @@ static inline void xen_create_ioreq_server(domid_t dom,
 
     *ioservid = 0;
     use_default_ioreq_server = true;
+
+    if (xen_stubdom_enabled()) {
+        xc_hvm_param_set(xen_xc, xen_domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
+    }
+
     trace_xen_default_ioreq_server();
 }
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.

A compile-time patch was originally written by James McKenzie
<james.mckenzie@bromium.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/xen/xen_pt_config_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 31ec5add1d..b827a493ea 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -54,6 +54,9 @@ static int xen_pt_hide_dev_cap(const XenHostPCIDevice *d, uint8_t grp_id)
             return 1;
         }
         break;
+    case PCI_CAP_ID_MSIX:
+        /* stubdoms don't support MSI-X so skip it. */
+        return xen_stubdom_enabled();
     }
     return 0;
 }
-- 
2.20.1

[PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.

A compile-time patch was originally written by James McKenzie
<james.mckenzie@bromium.com>

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/xen/xen_pt_config_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 31ec5add1d..b827a493ea 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -54,6 +54,9 @@ static int xen_pt_hide_dev_cap(const XenHostPCIDevice *d, uint8_t grp_id)
             return 1;
         }
         break;
+    case PCI_CAP_ID_MSIX:
+        /* stubdoms don't support MSI-X so skip it. */
+        return xen_stubdom_enabled();
     }
     return 0;
 }
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Qemu-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

From: Simon Gaiser <simon@invisiblethingslab.com>

If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
an address which is not page aligned. This breaks the memory mapping via
xc_domain_memory_mapping since this function is page based and the
"offset" is therefore lost.

Without this patch you will see error like this in the stubdom log:

  [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0000000000000004

QubesOS/qubes-issues#2849

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/xen/xen_pt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 5539d56c3a..7f680442ee 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
     /* Register PIO/MMIO BARs */
     for (i = 0; i < PCI_ROM_SLOT; i++) {
         XenHostPCIIORegion *r = &d->io_regions[i];
+        pcibus_t r_size = r->size;
         uint8_t type;
 
-        if (r->base_addr == 0 || r->size == 0) {
+        if (r->base_addr == 0 || r_size == 0) {
             continue;
         }
 
@@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
                 type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
             }
             *cmd |= PCI_COMMAND_MEMORY;
+
+            /* Round up to a full page for the hypercall. */
+            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
         }
 
         memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
-                              "xen-pci-pt-bar", r->size);
+                              "xen-pci-pt-bar", r_size);
         pci_register_bar(&s->dev, i, type, &s->bar[i]);
 
         XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
                    " base_addr=0x%08"PRIx64" type: %#x)\n",
-                   i, r->size, r->base_addr, type);
+                   i, r_size, r->base_addr, type);
     }
 
     /* Register expansion ROM address */
-- 
2.20.1

[PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

From: Simon Gaiser <simon@invisiblethingslab.com>

If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
an address which is not page aligned. This breaks the memory mapping via
xc_domain_memory_mapping since this function is page based and the
"offset" is therefore lost.

Without this patch you will see error like this in the stubdom log:

  [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0000000000000004

QubesOS/qubes-issues#2849

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/xen/xen_pt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 5539d56c3a..7f680442ee 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
     /* Register PIO/MMIO BARs */
     for (i = 0; i < PCI_ROM_SLOT; i++) {
         XenHostPCIIORegion *r = &d->io_regions[i];
+        pcibus_t r_size = r->size;
         uint8_t type;
 
-        if (r->base_addr == 0 || r->size == 0) {
+        if (r->base_addr == 0 || r_size == 0) {
             continue;
         }
 
@@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
                 type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
             }
             *cmd |= PCI_COMMAND_MEMORY;
+
+            /* Round up to a full page for the hypercall. */
+            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
         }
 
         memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
-                              "xen-pci-pt-bar", r->size);
+                              "xen-pci-pt-bar", r_size);
         pci_register_bar(&s->dev, i, type, &s->bar[i]);
 
         XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
                    " base_addr=0x%08"PRIx64" type: %#x)\n",
-                   i, r->size, r->base_addr, type);
+                   i, r_size, r->base_addr, type);
     }
 
     /* Register expansion ROM address */
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 1/6] xen: Introduce -xen-stubdom option

[Paolo Bonzini]

On 11/03/19 19:02, Jason Andryuk wrote:
> With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
> There are a few differences when running in a stubdom compared to dom0.
> Add the -xen-stubdom option to select this mode at runtime.  The default
> is off.

This should be "-accel xen,stubdom=on".  You should find examples for
tcg that explain how to add a suboption to -accel.

Paolo

> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  include/hw/xen/xen.h | 6 ++++++
>  qemu-options.hx      | 7 +++++++
>  vl.c                 | 8 ++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index ba039c146d..fed3611623 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -21,6 +21,7 @@ enum xen_mode {
>  extern uint32_t xen_domid;
>  extern enum xen_mode xen_mode;
>  extern bool xen_domid_restrict;
> +extern bool xen_stubdom;
>  
>  extern bool xen_allowed;
>  
> @@ -29,6 +30,11 @@ static inline bool xen_enabled(void)
>      return xen_allowed;
>  }
>  
> +static inline bool xen_stubdom_enabled(void)
> +{
> +    return xen_stubdom;
> +}
> +
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 1cf9aac1fe..ba56c3dd9a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3386,6 +3386,10 @@ DEF("xen-domid-restrict", 0, QEMU_OPTION_xen_domid_restrict,
>      "                        to specified domain id. (Does not affect\n"
>      "                        xenpv machine type).\n",
>      QEMU_ARCH_ALL)
> +DEF("xen-stubdom", 0, QEMU_OPTION_xen_stubdom,
> +    "-xen-stubdom    specify QEMU is running in a stubdom, so certain\n"
> +    "                behavior changes. (Does not affect xenpv machine type).\n",
> +    QEMU_ARCH_ALL)
>  STEXI
>  @item -xen-domid @var{id}
>  @findex -xen-domid
> @@ -3396,6 +3400,9 @@ Attach to existing xen domain.
>  libxl will use this when starting QEMU (XEN only).
>  @findex -xen-domid-restrict
>  Restrict set of available xen operations to specified domain id (XEN only).
> +@findex -xen-stubdom
> +@item -xen-stubdom
> +Run qemu in stubdom-mode (XEN only).
>  ETEXI
>  
>  DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \
> diff --git a/vl.c b/vl.c
> index 4a350de5cd..0d04319d9b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -206,6 +206,7 @@ bool xen_allowed;
>  uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
>  bool xen_domid_restrict;
> +bool xen_stubdom;
>  
>  static int has_defaults = 1;
>  static int default_serial = 1;
> @@ -3796,6 +3797,13 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  xen_domid_restrict = true;
>                  break;
> +            case QEMU_OPTION_xen_stubdom:
> +                if (!(xen_available())) {
> +                    error_report("Option not supported for this target");
> +                    exit(1);
> +                }
> +                xen_stubdom = true;
> +                break;
>              case QEMU_OPTION_trace:
>                  g_free(trace_file);
>                  trace_file = trace_opt_parse(optarg);
> 

Re: [PATCH 1/6] xen: Introduce -xen-stubdom option

[Paolo Bonzini]

On 11/03/19 19:02, Jason Andryuk wrote:
> With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
> There are a few differences when running in a stubdom compared to dom0.
> Add the -xen-stubdom option to select this mode at runtime.  The default
> is off.

This should be "-accel xen,stubdom=on".  You should find examples for
tcg that explain how to add a suboption to -accel.

Paolo

> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  include/hw/xen/xen.h | 6 ++++++
>  qemu-options.hx      | 7 +++++++
>  vl.c                 | 8 ++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index ba039c146d..fed3611623 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -21,6 +21,7 @@ enum xen_mode {
>  extern uint32_t xen_domid;
>  extern enum xen_mode xen_mode;
>  extern bool xen_domid_restrict;
> +extern bool xen_stubdom;
>  
>  extern bool xen_allowed;
>  
> @@ -29,6 +30,11 @@ static inline bool xen_enabled(void)
>      return xen_allowed;
>  }
>  
> +static inline bool xen_stubdom_enabled(void)
> +{
> +    return xen_stubdom;
> +}
> +
>  int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 1cf9aac1fe..ba56c3dd9a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3386,6 +3386,10 @@ DEF("xen-domid-restrict", 0, QEMU_OPTION_xen_domid_restrict,
>      "                        to specified domain id. (Does not affect\n"
>      "                        xenpv machine type).\n",
>      QEMU_ARCH_ALL)
> +DEF("xen-stubdom", 0, QEMU_OPTION_xen_stubdom,
> +    "-xen-stubdom    specify QEMU is running in a stubdom, so certain\n"
> +    "                behavior changes. (Does not affect xenpv machine type).\n",
> +    QEMU_ARCH_ALL)
>  STEXI
>  @item -xen-domid @var{id}
>  @findex -xen-domid
> @@ -3396,6 +3400,9 @@ Attach to existing xen domain.
>  libxl will use this when starting QEMU (XEN only).
>  @findex -xen-domid-restrict
>  Restrict set of available xen operations to specified domain id (XEN only).
> +@findex -xen-stubdom
> +@item -xen-stubdom
> +Run qemu in stubdom-mode (XEN only).
>  ETEXI
>  
>  DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \
> diff --git a/vl.c b/vl.c
> index 4a350de5cd..0d04319d9b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -206,6 +206,7 @@ bool xen_allowed;
>  uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
>  bool xen_domid_restrict;
> +bool xen_stubdom;
>  
>  static int has_defaults = 1;
>  static int default_serial = 1;
> @@ -3796,6 +3797,13 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  xen_domid_restrict = true;
>                  break;
> +            case QEMU_OPTION_xen_stubdom:
> +                if (!(xen_available())) {
> +                    error_report("Option not supported for this target");
> +                    exit(1);
> +                }
> +                xen_stubdom = true;
> +                break;
>              case QEMU_OPTION_trace:
>                  g_free(trace_file);
>                  trace_file = trace_opt_parse(optarg);
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

On Mon, Mar 11, 2019 at 2:06 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/03/19 19:02, Jason Andryuk wrote:
> > With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
> > There are a few differences when running in a stubdom compared to dom0.
> > Add the -xen-stubdom option to select this mode at runtime.  The default
> > is off.
>
> This should be "-accel xen,stubdom=on".  You should find examples for
> tcg that explain how to add a suboption to -accel.

Thanks, Paolo.  I'll re-work the option as you suggest.

Regards,
Jason

Re: [PATCH 1/6] xen: Introduce -xen-stubdom option

[Jason Andryuk]

On Mon, Mar 11, 2019 at 2:06 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/03/19 19:02, Jason Andryuk wrote:
> > With Xen, QEMU can run isolated in a dedicated service VM - a stubdom.
> > There are a few differences when running in a stubdom compared to dom0.
> > Add the -xen-stubdom option to select this mode at runtime.  The default
> > is off.
>
> This should be "-accel xen,stubdom=on".  You should find examples for
> tcg that explain how to add a suboption to -accel.

Thanks, Paolo.  I'll re-work the option as you suggest.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Roger Pau Monné]

On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.

I'm afraid this requires some more context. What's the actual issue
that prevents MSI-X from working?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]

On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> 
> I'm afraid this requires some more context. What's the actual issue
> that prevents MSI-X from working?

At least missing "Fix PCI passthrough for HVM with stubdomain" series,
but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
config space).
Some of it can be worked around by enabling permissive mode. Jason, did
you had a chance to test it with any MSI-X device?
I'm not aware of anything thing particular that breaks MSI-X but not
MSI. Besides much less devices lying around to test MSI-X...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> >
> > I'm afraid this requires some more context. What's the actual issue
> > that prevents MSI-X from working?
>
> At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> config space).
> Some of it can be worked around by enabling permissive mode. Jason, did
> you had a chance to test it with any MSI-X device?
> I'm not aware of anything thing particular that breaks MSI-X but not
> MSI. Besides much less devices lying around to test MSI-X...

OpenXT and Qubes have used a compile time patch that disabled MSI-X
for a long time.  The OpenXT patch description doesn't help:
"""
Currently we do not support MSI-X setup for PCI devices passed through.

Although the specification mentions that PCI-e devices might implement only
MSI-X there is not a lot of those and mostly none that we have encountered yet.
Considering that, we force devices to use MSI by hiding the MSI-X capability.
"""

To be honest, I didn't question the reasoning and just made the
compile-time disabling into a runtime disabling.

I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
was an error related to setting up MSI-X when I failed to pass the
"-xen-stubdom" flag.  I can pull that log when I get back to the
machine.  With this patch, MSI-X was hidden in the guest, but dom0
showed MSI-X present but unused.

Marek, is "Use xc_physdev_msi_set_enable for enabling MSI..." the QEMU
patch you are refer to?  Do you think permissive mode would allow
MSI-X to work without that patch?  I could test that out.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Roger Pau Monné]

On Tue, Mar 12, 2019 at 09:58:56AM -0400, Jason Andryuk wrote:
> On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> > >
> > > I'm afraid this requires some more context. What's the actual issue
> > > that prevents MSI-X from working?
> >
> > At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> > but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> > config space).
> > Some of it can be worked around by enabling permissive mode. Jason, did
> > you had a chance to test it with any MSI-X device?
> > I'm not aware of anything thing particular that breaks MSI-X but not
> > MSI. Besides much less devices lying around to test MSI-X...
> 
> OpenXT and Qubes have used a compile time patch that disabled MSI-X
> for a long time.  The OpenXT patch description doesn't help:
> """
> Currently we do not support MSI-X setup for PCI devices passed through.
> 
> Although the specification mentions that PCI-e devices might implement only
> MSI-X there is not a lot of those and mostly none that we have encountered yet.
> Considering that, we force devices to use MSI by hiding the MSI-X capability.
> """
> 
> To be honest, I didn't question the reasoning and just made the
> compile-time disabling into a runtime disabling.
> 
> I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
> was an error related to setting up MSI-X when I failed to pass the
> "-xen-stubdom" flag.  I can pull that log when I get back to the
> machine.  With this patch, MSI-X was hidden in the guest, but dom0
> showed MSI-X present but unused.

Given that Marek is working on a series to make both MSI and MSI-X
work for pci-passthrough and stubdomains I'm not sure how did you even
manage to get plain MSI working. Is there something I'm missing?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2516 bytes --]

On Tue, Mar 12, 2019 at 09:58:56AM -0400, Jason Andryuk wrote:
> On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> > >
> > > I'm afraid this requires some more context. What's the actual issue
> > > that prevents MSI-X from working?
> >
> > At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> > but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> > config space).
> > Some of it can be worked around by enabling permissive mode. Jason, did
> > you had a chance to test it with any MSI-X device?
> > I'm not aware of anything thing particular that breaks MSI-X but not
> > MSI. Besides much less devices lying around to test MSI-X...
> 
> OpenXT and Qubes have used a compile time patch that disabled MSI-X
> for a long time.  The OpenXT patch description doesn't help:
> """
> Currently we do not support MSI-X setup for PCI devices passed through.
> 
> Although the specification mentions that PCI-e devices might implement only
> MSI-X there is not a lot of those and mostly none that we have encountered yet.
> Considering that, we force devices to use MSI by hiding the MSI-X capability.
> """
> 
> To be honest, I didn't question the reasoning and just made the
> compile-time disabling into a runtime disabling.
> 
> I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
> was an error related to setting up MSI-X when I failed to pass the
> "-xen-stubdom" flag.  I can pull that log when I get back to the
> machine.  With this patch, MSI-X was hidden in the guest, but dom0
> showed MSI-X present but unused.
> 
> Marek, is "Use xc_physdev_msi_set_enable for enabling MSI..." the QEMU
> patch you are refer to?  Do you think permissive mode would allow
> MSI-X to work without that patch?  I could test that out.

Yes, this one. Permissive mode should work around it.
There is also another patch about IRQ permission, but I believe you
already have it in OpenXT.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 10:13 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Tue, Mar 12, 2019 at 09:58:56AM -0400, Jason Andryuk wrote:
> > On Tue, Mar 12, 2019 at 8:38 AM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > >
> > > On Tue, Mar 12, 2019 at 01:04:19PM +0100, Roger Pau Monné wrote:
> > > > On Mon, Mar 11, 2019 at 02:02:15PM -0400, Jason Andryuk wrote:
> > > > > MSI-X is not supported in Xen stubdoms, so it must be disabled.  Use the
> > > > > existing xen_pt_hide_dev_cap to hide when running under -xen-stubdom.
> > > >
> > > > I'm afraid this requires some more context. What's the actual issue
> > > > that prevents MSI-X from working?
> > >
> > > At least missing "Fix PCI passthrough for HVM with stubdomain" series,
> > > but that's mostly on Xen side (+ one change how QEMU enable MSI-X in
> > > config space).
> > > Some of it can be worked around by enabling permissive mode. Jason, did
> > > you had a chance to test it with any MSI-X device?
> > > I'm not aware of anything thing particular that breaks MSI-X but not
> > > MSI. Besides much less devices lying around to test MSI-X...
> >
> > OpenXT and Qubes have used a compile time patch that disabled MSI-X
> > for a long time.  The OpenXT patch description doesn't help:
> > """
> > Currently we do not support MSI-X setup for PCI devices passed through.
> >
> > Although the specification mentions that PCI-e devices might implement only
> > MSI-X there is not a lot of those and mostly none that we have encountered yet.
> > Considering that, we force devices to use MSI by hiding the MSI-X capability.
> > """
> >
> > To be honest, I didn't question the reasoning and just made the
> > compile-time disabling into a runtime disabling.
> >
> > I tested with a NEC uPD720200 XHCI controller supporting MSI-X.  There
> > was an error related to setting up MSI-X when I failed to pass the
> > "-xen-stubdom" flag.  I can pull that log when I get back to the
> > machine.  With this patch, MSI-X was hidden in the guest, but dom0
> > showed MSI-X present but unused.
>
> Given that Marek is working on a series to make both MSI and MSI-X
> work for pci-passthrough and stubdomains I'm not sure how did you even
> manage to get plain MSI working. Is there something I'm missing?

Marek's series adds a hypercall to enable MSI/MSI-X since they are not
allowed by default in PCI passthrough.  PCI passthrough also has a
permissive mode to allow access to a device's entire PCI configuration
space including enabling MSI.

Qubes 4.0 has an out-of-tree patch that whitelists enabling MSI in
non-permissive mode - I've tested these patches on Qubes 4.0 with the
MSI-X XHCI device where MSI is enabled but not MSI-X.  Other NICs with
only MSI also work.

OpenXT linux stubdoms use permissive mode PCI passthrough, so the
stubdom can program the PCI config space to enable MSI.  I've tested
the patches there, but none of my OpenXT test systems have MSI-X.  MSI
devices work properly.

Marek also tested a "vanilla" linux stubdom version in his osstest
suite, but that doesn't test passthrough.

If Marek's pending patch series or "permissive" mode will enable
MSI/MSI-X, then maybe this patch should just be dropped in favor of
those options.  I'll test to verify whether MSI-X works with
permissive mode.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms

[Jason Andryuk]

On Tue, Mar 12, 2019 at 11:15 AM Jason Andryuk <jandryuk@gmail.com> wrote:
<snip>
> I'll test to verify whether MSI-X works with
> permissive mode.

Dropping this patch and enabling permissive mode allowed MSI-X to work.

{"execute": "device_add", "arguments": {"driver":
"xen-pci-passthrough", "id": "xen-pci-pt_0000-03-00.0", "hostaddr":
"0000:00:01.00", "machine_addr": "0000:03:00.0", "permissive": true}}
[00:07.0] xen_pt_realize: Assigning real physical device 00:01.0 to devfn 0x38
[00:07.0] xen_pt_register_regions: IO region 0 registered
(size=0x00002000 base_addr=0xe1900000 type: 0x4)
[00:07.0] xen_pt_config_reg_init: Offset 0x000e mismatch!
Emulated=0x0080, host=0x0000, syncing to 0x0000.
[00:07.0] xen_pt_config_reg_init: Offset 0x0010 mismatch!
Emulated=0x0000, host=0xe1900004, syncing to 0xe1900004.
[00:07.0] xen_pt_config_reg_init: Offset 0x0052 mismatch!
Emulated=0x0000, host=0x01c3, syncing to 0x0003.
[00:07.0] xen_pt_config_reg_init: Offset 0x0072 mismatch!
Emulated=0x0000, host=0x0086, syncing to 0x0080.
[00:07.0] xen_pt_config_reg_init: Offset 0x00a4 mismatch!
Emulated=0x0000, host=0x8fc0, syncing to 0x8fc0.
[00:07.0] xen_pt_config_reg_init: Offset 0x00aa mismatch!
Emulated=0x0000, host=0x0010, syncing to 0x0010.
[00:07.0] xen_pt_config_reg_init: Offset 0x00b2 mismatch!
Emulated=0x0000, host=0x1011, syncing to 0x1011.
[00:07.0] xen_pt_msix_init: get MSI-X table BAR base 0xe1900000
[00:07.0] xen_pt_msix_init: table_off = 0x1000, total_entries = 8
[00:07.0] xen_pt_msix_init: mapping physical MSI-X table to 0x7ad6a82e4000
[00:07.0] xen_pt_config_reg_init: Offset 0x0092 mismatch!
Emulated=0x0000, host=0x0007, syncing to 0x0007.
[00:07.0] xen_pt_pci_intx: intx=1
[00:07.0] xen_pt_realize: Real physical device 00:01.0 registered successfully
{"return": {}}

hw/xen/xen_pt_msi.c:xen_pt_msix_init() calls open(/dev/mem) and mmaps
it, so I had to add CONFIG_DEVMEM to the stubdom linux .config.
Without /dev/mem:
[00:07.0] xen_pt_msix_init: get MSI-X table BAR base 0xe1900000
[00:07.0] xen_pt_msix_init: Error: Can't open /dev/mem: No such file
or directory
[00:07.0] xen_pt_msix_size_init: Error: Internal error: Invalid
xen_pt_msix_init.
Failed to initialize 12/15, type = 0x1, rc: -2
[00:07.0] xen_pt_msi_set_enable: disabling MSI.
free(): double free detected in tcache 2

[user@sys-usb ~]$ sudo lspci -v -s 00:07.0
00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
Controller (rev 04) (prog-if 30 [XHCI])
    Subsystem: Lenovo Device 21d2
    Physical Slot: 7
    Flags: bus master, fast devsel, latency 0, IRQ 44
    Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
    Capabilities: [50] Power Management version 3
    Capabilities: [70] MSI: Enable- Count=1/1 Maskable- 64bit+
    Capabilities: [90] MSI-X: Enable+ Count=8 Masked-
    Capabilities: [a0] Express Endpoint, MSI 00
    Kernel driver in use: xhci_hcd
    Kernel modules: xhci_pci

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: Move xenstore initialization to common location

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 11 March 2019 18:02
> To: qemu-devel@nongnu.org
> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> 
> For the xen stubdom case, we'll want xenstore initialized, but we'll
> want to skip the rest of xen_be_init.  Move the initialization to
> xen_hvm_init so we can conditionalize calling xen_be_init.
> 
> xs_domain_open() is deprecated for xs_open(0), so make the replacement
> as well.

Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is implementing a PV backend?

  Paul

> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  hw/i386/xen/xen-hvm.c       | 8 ++++++++
>  hw/xen/xen-legacy-backend.c | 8 --------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 2939122e7c..c20c4b27f6 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> 
>      xen_bus_init();
> 
> +    xenstore = xs_open(0);
> +    if (!xenstore) {
> +        error_report("Can't connect to xenstored");
> +        goto err;
> +    }
> +
> +    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
> +
>      /* Initialize backend core & drivers */
>      if (xen_be_init() != 0) {
>          error_report("xen backend core setup failed");
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 36fd1e9b09..bdf2fa917f 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -683,14 +683,6 @@ int xen_be_init(void)
>  {
>      xengnttab_handle *gnttabdev;
> 
> -    xenstore = xs_daemon_open();
> -    if (!xenstore) {
> -        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
> -        return -1;
> -    }
> -
> -    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
> -
>      if (xen_xc == NULL || xen_fmem == NULL) {
>          /* Check if xen_init() have been called */
>          goto err;
> --
> 2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen: Skip backend initialization for stubdom

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 11 March 2019 18:02
> To: qemu-devel@nongnu.org
> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: [PATCH 3/6] xen: Skip backend initialization for stubdom
> 
> When QEMU is running in a stubdom, it does not provide any
> Paravirtualized backends.  Those still run in dom0 or another driver
> domain.  Therefore we skip backend initialization (xen_bus_init and
> xen_be_init) for the stubdom case.
> 
> Original patch by Anthony PERARD <anthony.perard@citrix.com>
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  hw/i386/xen/xen-hvm.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index c20c4b27f6..4b62f070cb 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1485,8 +1485,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
>      QLIST_INIT(&state->dev_list);
>      device_listener_register(&state->device_listener);
> 
> -    xen_bus_init();
> -
>      xenstore = xs_open(0);
>      if (!xenstore) {
>          error_report("Can't connect to xenstored");
> @@ -1495,12 +1493,16 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> 
>      qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
> 
> -    /* Initialize backend core & drivers */
> -    if (xen_be_init() != 0) {
> -        error_report("xen backend core setup failed");
> -        goto err;
> +    if (!xen_stubdom_enabled()) {
> +        xen_bus_init();
> +
> +        /* Initialize backend core & drivers */
> +        if (xen_be_init() != 0) {
> +            error_report("xen backend core setup failed");
> +            goto err;
> +        }
> +        xen_be_register_common();
>      }
> -    xen_be_register_common();
> 
>      QLIST_INIT(&xen_physmap);
>      xen_read_physmap(state);
> --
> 2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 11 March 2019 18:02
> To: qemu-devel@nongnu.org
> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> 
> From: Simon Gaiser <simon@invisiblethingslab.com>
> 
> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> an address which is not page aligned.

IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be tolerating BARs smaller than that?

  Paul

> This breaks the memory mapping via
> xc_domain_memory_mapping since this function is page based and the
> "offset" is therefore lost.
> 
> Without this patch you will see error like this in the stubdom log:
> 
>   [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0000000000000004
> 
> QubesOS/qubes-issues#2849
> 
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  hw/xen/xen_pt.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 5539d56c3a..7f680442ee 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>      /* Register PIO/MMIO BARs */
>      for (i = 0; i < PCI_ROM_SLOT; i++) {
>          XenHostPCIIORegion *r = &d->io_regions[i];
> +        pcibus_t r_size = r->size;
>          uint8_t type;
> 
> -        if (r->base_addr == 0 || r->size == 0) {
> +        if (r->base_addr == 0 || r_size == 0) {
>              continue;
>          }
> 
> @@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>                  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>              }
>              *cmd |= PCI_COMMAND_MEMORY;
> +
> +            /* Round up to a full page for the hypercall. */
> +            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
>          }
> 
>          memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
> -                              "xen-pci-pt-bar", r->size);
> +                              "xen-pci-pt-bar", r_size);
>          pci_register_bar(&s->dev, i, type, &s->bar[i]);
> 
>          XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
>                     " base_addr=0x%08"PRIx64" type: %#x)\n",
> -                   i, r->size, r->base_addr, type);
> +                   i, r_size, r->base_addr, type);
>      }
> 
>      /* Register expansion ROM address */
> --
> 2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: Move xenstore initialization to common location

[Jason Andryuk]

On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-devel@nongnu.org
> > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> > <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> > <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> > Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> >
> > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > want to skip the rest of xen_be_init.  Move the initialization to
> > xen_hvm_init so we can conditionalize calling xen_be_init.
> >
> > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > as well.
>
> Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is implementing a PV backend?
>
>

Hi, Paul.  Thanks for reviewing.

I think you are right, that this basically shouldn't be needed if PV
backends are disabled.  This patch came out of OpenXT, where it is
needed for some out-of-tree patches.  But that doesn't make it
suitable for upstreaming.

However, while reviewing, it looks like the xen accelerator in
hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
xen_change_state_handler() uses the global xenstore handle and will
exit(1) if NULL.  I'm not sure how to get the XenIOState xenstore
handle over to the accelerator's xen_init.  Outside of that, I think
you are correct that xenstore_update doesn't need to be run when PV
backends are disabled.

Thanks,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen: Move xenstore initialization to common location

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 13 March 2019 18:12
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Stefano
> Stabellini <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: Re: [PATCH 2/6] xen: Move xenstore initialization to common location
> 
> On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > Sent: 11 March 2019 18:02
> > > To: qemu-devel@nongnu.org
> > > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> > > <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> > > Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> > >
> > > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > > want to skip the rest of xen_be_init.  Move the initialization to
> > > xen_hvm_init so we can conditionalize calling xen_be_init.
> > >
> > > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > > as well.
> >
> > Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already
> opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is
> implementing a PV backend?
> >
> >
> 
> Hi, Paul.  Thanks for reviewing.
> 
> I think you are right, that this basically shouldn't be needed if PV
> backends are disabled.  This patch came out of OpenXT, where it is
> needed for some out-of-tree patches.  But that doesn't make it
> suitable for upstreaming.
> 
> However, while reviewing, it looks like the xen accelerator in
> hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
> xen_change_state_handler() uses the global xenstore handle and will
> exit(1) if NULL.

I see it yes. TBH signalling state via xenstore should go away as it is incompatible with deprivileging, and I think Anthony might have some patches for that? In the meantime I suggest just doing a local xs_open(0) in that function.

> I'm not sure how to get the XenIOState xenstore
> handle over to the accelerator's xen_init.

That would not be appropriate as the machine type may not be xenfv and hence xen_hvm_init() may not have been called.

  Paul

>  Outside of that, I think
> you are correct that xenstore_update doesn't need to be run when PV
> backends are disabled.
> 
> Thanks,
> Jason
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-devel@nongnu.org
> > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > <Paul.Durrant@citrix.com>
> > Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >
> > From: Simon Gaiser <simon@invisiblethingslab.com>
> >
> > If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > an address which is not page aligned.
>
> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be tolerating BARs smaller than that?
>
>   Paul
>

Hi, Paul.

Simon found this, so it affects a real device.  Simon, do you recall
which device was affected?

I think BARs only need to be power-of-two size and aligned, and 4k is
not a minimum.  16bytes may be a minimum, but I don't know what the
spec says.

On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
00:16.0 Communication controller: Intel Corporation 7 Series/C210
Series Chipset Family MEI Controller #1 (rev 04)
   Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
   Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
SMBus Controller (rev 04)
   Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
Controller (rev 30)
   Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]

These examples are all 4K aligned, so this is not an issue on this machine.

Reviewing the code, I'm now wondering if the following in
hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
xc_domain_memory_mapping(xen_xc, xen_domid,
                                     XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
                                     XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
                                     XEN_PFN(size + XC_PAGE_SIZE - 1),
                                     op);

If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
in would be 0xd0501000 which is past the actual location.  Should the
call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?

BARs smaller than a page would also be a problem if BARs for different
devices shared the same page.

Regards,
Jason

> > This breaks the memory mapping via
> > xc_domain_memory_mapping since this function is page based and the
> > "offset" is therefore lost.
> >
> > Without this patch you will see error like this in the stubdom log:
> >
> >   [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0000000000000004
> >
> > QubesOS/qubes-issues#2849
> >
> > Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
> >  hw/xen/xen_pt.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index 5539d56c3a..7f680442ee 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> >      /* Register PIO/MMIO BARs */
> >      for (i = 0; i < PCI_ROM_SLOT; i++) {
> >          XenHostPCIIORegion *r = &d->io_regions[i];
> > +        pcibus_t r_size = r->size;
> >          uint8_t type;
> >
> > -        if (r->base_addr == 0 || r->size == 0) {
> > +        if (r->base_addr == 0 || r_size == 0) {
> >              continue;
> >          }
> >
> > @@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> >                  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> >              }
> >              *cmd |= PCI_COMMAND_MEMORY;
> > +
> > +            /* Round up to a full page for the hypercall. */
> > +            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
> >          }
> >
> >          memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
> > -                              "xen-pci-pt-bar", r->size);
> > +                              "xen-pci-pt-bar", r_size);
> >          pci_register_bar(&s->dev, i, type, &s->bar[i]);
> >
> >          XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
> >                     " base_addr=0x%08"PRIx64" type: %#x)\n",
> > -                   i, r->size, r->base_addr, type);
> > +                   i, r_size, r->base_addr, type);
> >      }
> >
> >      /* Register expansion ROM address */
> > --
> > 2.20.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Simon Gaiser]

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

Jason Andryuk:
> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>
>>> -----Original Message-----
>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>>> Sent: 11 March 2019 18:02
>>> To: qemu-devel@nongnu.org
>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
>>> <Paul.Durrant@citrix.com>
>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>
>>> From: Simon Gaiser <simon@invisiblethingslab.com>
>>>
>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
>>> an address which is not page aligned.
>>
>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be tolerating BARs smaller than that?
>>
>>   Paul
>>
> 
> Hi, Paul.
> 
> Simon found this, so it affects a real device.  Simon, do you recall
> which device was affected?

Not sure which one it was. Probably the USB controller or the SD host
controller. As your example below shows this is not so uncommon.

> I think BARs only need to be power-of-two size and aligned, and 4k is
> not a minimum.  16bytes may be a minimum, but I don't know what the
> spec says.
> 
> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> Series Chipset Family MEI Controller #1 (rev 04)
>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> SMBus Controller (rev 04)
>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> Controller (rev 30)
>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> 
> These examples are all 4K aligned, so this is not an issue on this machine.
> 
> Reviewing the code, I'm now wondering if the following in
> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> xc_domain_memory_mapping(xen_xc, xen_domid,
>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
>                                      op);
> 
> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> in would be 0xd0501000 which is past the actual location.  Should the
> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> 
> BARs smaller than a page would also be a problem if BARs for different
> devices shared the same page.
> 
> Regards,
> Jason
> 
>>> This breaks the memory mapping via
>>> xc_domain_memory_mapping since this function is page based and the
>>> "offset" is therefore lost.
>>>
>>> Without this patch you will see error like this in the stubdom log:
>>>
>>>   [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU. @0x0000000000000004
>>>
>>> QubesOS/qubes-issues#2849
>>>
>>> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
>>> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>>> ---
>>>  hw/xen/xen_pt.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>>> index 5539d56c3a..7f680442ee 100644
>>> --- a/hw/xen/xen_pt.c
>>> +++ b/hw/xen/xen_pt.c
>>> @@ -449,9 +449,10 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>>>      /* Register PIO/MMIO BARs */
>>>      for (i = 0; i < PCI_ROM_SLOT; i++) {
>>>          XenHostPCIIORegion *r = &d->io_regions[i];
>>> +        pcibus_t r_size = r->size;
>>>          uint8_t type;
>>>
>>> -        if (r->base_addr == 0 || r->size == 0) {
>>> +        if (r->base_addr == 0 || r_size == 0) {
>>>              continue;
>>>          }
>>>
>>> @@ -469,15 +470,18 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
>>>                  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>>>              }
>>>              *cmd |= PCI_COMMAND_MEMORY;
>>> +
>>> +            /* Round up to a full page for the hypercall. */
>>> +            r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
>>>          }
>>>
>>>          memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
>>> -                              "xen-pci-pt-bar", r->size);
>>> +                              "xen-pci-pt-bar", r_size);
>>>          pci_register_bar(&s->dev, i, type, &s->bar[i]);
>>>
>>>          XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
>>>                     " base_addr=0x%08"PRIx64" type: %#x)\n",
>>> -                   i, r->size, r->base_addr, type);
>>> +                   i, r_size, r->base_addr, type);
>>>      }
>>>
>>>      /* Register expansion ROM address */
>>> --
>>> 2.20.1


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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Andrew Cooper]

On 14/03/2019 19:22, Simon Gaiser wrote:
> Jason Andryuk:
>> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>> -----Original Message-----
>>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>>>> Sent: 11 March 2019 18:02
>>>> To: qemu-devel@nongnu.org
>>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
>>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
>>>> <Paul.Durrant@citrix.com>
>>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>>
>>>> From: Simon Gaiser <simon@invisiblethingslab.com>
>>>>
>>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
>>>> an address which is not page aligned.
>>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be tolerating BARs smaller than that?
>>>
>>>   Paul
>>>
>> Hi, Paul.
>>
>> Simon found this, so it affects a real device.  Simon, do you recall
>> which device was affected?
> Not sure which one it was. Probably the USB controller or the SD host
> controller. As your example below shows this is not so uncommon.

The minimum is 128 bytes, not 4k - I've just checked the PCIe spec.

Xen/Qemu definitely needs to cope with smaller than 4k if we want to be
spec compliant.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Simon Gaiser]

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

Jason Andryuk:
> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>
>>> -----Original Message-----
>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>>> Sent: 11 March 2019 18:02
>>> To: qemu-devel@nongnu.org
>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
>>> <Paul.Durrant@citrix.com>
>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>
>>> From: Simon Gaiser <simon@invisiblethingslab.com>
>>>
>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
>>> an address which is not page aligned.
>>
>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be tolerating BARs smaller than that?
>>
>>   Paul
>>
> 
> Hi, Paul.
> 
> Simon found this, so it affects a real device.  Simon, do you recall
> which device was affected?
> 
> I think BARs only need to be power-of-two size and aligned, and 4k is
> not a minimum.  16bytes may be a minimum, but I don't know what the
> spec says.
> 
> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> Series Chipset Family MEI Controller #1 (rev 04)
>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> SMBus Controller (rev 04)
>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> Controller (rev 30)
>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> 
> These examples are all 4K aligned, so this is not an issue on this machine.

I wrote this patch quite some time ago, so I might be misremembering
something but IIRC the problem was the address qemu allocates with
memory_region_init_io(). I.e. the address as seen from inside the VM, so
it does not help if the real address is aligned.


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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 14 March 2019 19:37
> To: Simon Gaiser <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; qemu-devel@nongnu.org; marmarek@invisiblethingslab.com
> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> 
> On 14/03/2019 19:22, Simon Gaiser wrote:
> > Jason Andryuk:
> >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >>>> -----Original Message-----
> >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> >>>> Sent: 11 March 2019 18:02
> >>>> To: qemu-devel@nongnu.org
> >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> >>>> <Paul.Durrant@citrix.com>
> >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >>>>
> >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> >>>>
> >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> >>>> an address which is not page aligned.
> >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even
> be tolerating BARs smaller than that?
> >>>
> >>>   Paul
> >>>
> >> Hi, Paul.
> >>
> >> Simon found this, so it affects a real device.  Simon, do you recall
> >> which device was affected?
> > Not sure which one it was. Probably the USB controller or the SD host
> > controller. As your example below shows this is not so uncommon.
> 
> The minimum is 128 bytes, not 4k - I've just checked the PCIe spec.
> 
> Xen/Qemu definitely needs to cope with smaller than 4k if we want to be
> spec compliant.

Well, we have a problem for pass-through if the BAR is smaller than 4k in that page protection is not going to isolate it. I don't see any other way that to trap and emulate such BARs if we want to pass through those devices at all.

  Paul

> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 14 March 2019 18:16
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> 
> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > Sent: 11 March 2019 18:02
> > > To: qemu-devel@nongnu.org
> > > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > <Paul.Durrant@citrix.com>
> > > Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > >
> > > From: Simon Gaiser <simon@invisiblethingslab.com>
> > >
> > > If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > an address which is not page aligned.
> >
> > IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> tolerating BARs smaller than that?
> >
> >   Paul
> >
> 
> Hi, Paul.
> 
> Simon found this, so it affects a real device.  Simon, do you recall
> which device was affected?
> 
> I think BARs only need to be power-of-two size and aligned, and 4k is
> not a minimum.  16bytes may be a minimum, but I don't know what the
> spec says.
> 
> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> Series Chipset Family MEI Controller #1 (rev 04)
>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> SMBus Controller (rev 04)
>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> Controller (rev 30)
>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> 
> These examples are all 4K aligned, so this is not an issue on this machine.
> 
> Reviewing the code, I'm now wondering if the following in
> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> xc_domain_memory_mapping(xen_xc, xen_domid,
>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
>                                      op);
> 
> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> in would be 0xd0501000 which is past the actual location.  Should the
> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> 
> BARs smaller than a page would also be a problem if BARs for different
> devices shared the same page.

Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.

  Paul 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Andrew Cooper]

On 15/03/2019 09:17, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>> Sent: 14 March 2019 18:16
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
>> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
>> <anthony.perard@citrix.com>
>> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>
>> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>>> -----Original Message-----
>>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>>>> Sent: 11 March 2019 18:02
>>>> To: qemu-devel@nongnu.org
>>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
>>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
>>>> <Paul.Durrant@citrix.com>
>>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>>
>>>> From: Simon Gaiser <simon@invisiblethingslab.com>
>>>>
>>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
>>>> an address which is not page aligned.
>>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
>> tolerating BARs smaller than that?
>>>   Paul
>>>
>> Hi, Paul.
>>
>> Simon found this, so it affects a real device.  Simon, do you recall
>> which device was affected?
>>
>> I think BARs only need to be power-of-two size and aligned, and 4k is
>> not a minimum.  16bytes may be a minimum, but I don't know what the
>> spec says.
>>
>> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
>> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
>> Series Chipset Family MEI Controller #1 (rev 04)
>>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
>> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
>> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
>>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
>> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
>> SMBus Controller (rev 04)
>>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
>> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
>> Controller (rev 30)
>>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
>>
>> These examples are all 4K aligned, so this is not an issue on this machine.
>>
>> Reviewing the code, I'm now wondering if the following in
>> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
>> xc_domain_memory_mapping(xen_xc, xen_domid,
>>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
>>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
>>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
>>                                      op);
>>
>> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
>> in would be 0xd0501000 which is past the actual location.  Should the
>> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
>>
>> BARs smaller than a page would also be a problem if BARs for different
>> devices shared the same page.
> Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.

It doesn't matter if the BAR is smaller than 4k, if there are holes next
to it.

Do we know what the case is in practice for these USB controllers?

If the worst comes to the worst, we can re-enumerate the PCI bus to
ensure that all bars smaller than 4k still have 4k alignment between
them.  That way we can safely pass them through even when they are smaller.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 15/03/2019 09:17, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> >> Sent: 14 March 2019 18:16
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> >> <anthony.perard@citrix.com>
> >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >>
> >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >>>> -----Original Message-----
> >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> >>>> Sent: 11 March 2019 18:02
> >>>> To: qemu-devel@nongnu.org
> >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> >>>> <Paul.Durrant@citrix.com>
> >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >>>>
> >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> >>>>
> >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> >>>> an address which is not page aligned.
> >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> >> tolerating BARs smaller than that?
> >>>   Paul
> >>>
> >> Hi, Paul.
> >>
> >> Simon found this, so it affects a real device.  Simon, do you recall
> >> which device was affected?
> >>
> >> I think BARs only need to be power-of-two size and aligned, and 4k is
> >> not a minimum.  16bytes may be a minimum, but I don't know what the
> >> spec says.
> >>
> >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> >> Series Chipset Family MEI Controller #1 (rev 04)
> >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> >> SMBus Controller (rev 04)
> >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> >> Controller (rev 30)
> >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> >>
> >> These examples are all 4K aligned, so this is not an issue on this machine.
> >>
> >> Reviewing the code, I'm now wondering if the following in
> >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> >> xc_domain_memory_mapping(xen_xc, xen_domid,
> >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> >>                                      op);
> >>
> >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> >> in would be 0xd0501000 which is past the actual location.  Should the
> >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> >>
> >> BARs smaller than a page would also be a problem if BARs for different
> >> devices shared the same page.
> > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
>
> It doesn't matter if the BAR is smaller than 4k, if there are holes next
> to it.
>
> Do we know what the case is in practice for these USB controllers?
>
> If the worst comes to the worst, we can re-enumerate the PCI bus to
> ensure that all bars smaller than 4k still have 4k alignment between
> them.  That way we can safely pass them through even when they are smaller.

Andrew, thanks for checking the spec on the minimum BAR size.

Dropping the Round PCI region patch from QMEU, the guest HVM will have:

00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
    Memory at f2028800 (32-bit, non-prefetchable) [size=256]
00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
Controller (rev 04) (prog-if 30 [XHCI])
    Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
    Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
    Memory at f2028400 (32-bit, non-prefetchable) [size=1K]

00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
working.  With some added debugging output, you'll see that the same
page* is used for three of the BARs.

[00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
0xe1a30000 mfn 0xe1a30
[00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
0xe0800000 mfn 0xe0800
[00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
0xe1900000 mfn 0xe1900
[00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
0xe1a2f000 mfn 0xe1a2f

(*These print statements don't round up the addresses like
xen_pt_region_update currently does when calling
xc_domain_memory_mapping.  I think that rounding is incorrect since
address 0xf2028400 in pfn 0xf2028 would actually round to 0xf2029.)

I haven't tracked down all the following, but the guest controls where
the BARs get located, right?  So rounding up all BARs to at least 1
page prevent the guest locating mutliple BARs in the same page.  The
other approach would be to make the Guest OS provide a minimum page
size alignment to each.  Since we're dealing with HVMs that aren't
necessarily aware of Xen, making QEMU enforce the minimum size avoids
BARs sharing a page.  But then is it an issue if QEMU advertises a bar
size greater than the device's real size?

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Roger Pau Monné]

On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
> >
> > On 15/03/2019 09:17, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > >> Sent: 14 March 2019 18:16
> > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > >> <anthony.perard@citrix.com>
> > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > >>
> > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > >>>> -----Original Message-----
> > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > >>>> Sent: 11 March 2019 18:02
> > >>>> To: qemu-devel@nongnu.org
> > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > >>>> <Paul.Durrant@citrix.com>
> > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > >>>>
> > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > >>>>
> > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > >>>> an address which is not page aligned.
> > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > >> tolerating BARs smaller than that?
> > >>>   Paul
> > >>>
> > >> Hi, Paul.
> > >>
> > >> Simon found this, so it affects a real device.  Simon, do you recall
> > >> which device was affected?
> > >>
> > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > >> spec says.
> > >>
> > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > >> Series Chipset Family MEI Controller #1 (rev 04)
> > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > >> SMBus Controller (rev 04)
> > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > >> Controller (rev 30)
> > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > >>
> > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > >>
> > >> Reviewing the code, I'm now wondering if the following in
> > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > >>                                      op);
> > >>
> > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > >> in would be 0xd0501000 which is past the actual location.  Should the
> > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > >>
> > >> BARs smaller than a page would also be a problem if BARs for different
> > >> devices shared the same page.
> > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> >
> > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > to it.
> >
> > Do we know what the case is in practice for these USB controllers?
> >
> > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > ensure that all bars smaller than 4k still have 4k alignment between
> > them.  That way we can safely pass them through even when they are smaller.
> 
> Andrew, thanks for checking the spec on the minimum BAR size.
> 
> Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> 
> 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
>     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> Controller (rev 04) (prog-if 30 [XHCI])
>     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
>     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
>     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> 
> 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> working.  With some added debugging output, you'll see that the same
> page* is used for three of the BARs.
> 
> [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> 0xe1a30000 mfn 0xe1a30
> [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> 0xe0800000 mfn 0xe0800
> [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> 0xe1900000 mfn 0xe1900
> [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> 0xe1a2f000 mfn 0xe1a2f

The patch below should prevent hvmloader from placing multiple BARs on
the same page, could you give it a try?

Note that this is not going to prevent the guest from moving those
BARs around and place them in the same page, thus breaking the initial
placement done by hvmloader.

Thanks, Roger.

---8<---
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 0b708bf578..c433b34cd6 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -489,6 +489,10 @@ void pci_setup(void)
 
         resource->base = base;
 
+        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
+             PCI_BASE_ADDRESS_SPACE_MEMORY )
+            resource->base = ROUNDUP(resource->base, PAGE_SIZE);
+
         pci_writel(devfn, bar_reg, bar_data);
         if (using_64bar)
             pci_writel(devfn, bar_reg + 4, bar_data_upper);
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 7bca6418d2..b5554b5844 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -51,6 +51,8 @@ void __bug(char *file, int line) __attribute__((noreturn));
 #define MB(mb) (mb##ULL << 20)
 #define GB(gb) (gb##ULL << 30)
 
+#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
 static inline int test_bit(unsigned int b, const void *p)
 {
     return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > <andrew.cooper3@citrix.com> wrote:
> > >
> > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > >> -----Original Message-----
> > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > >> Sent: 14 March 2019 18:16
> > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > >> <anthony.perard@citrix.com>
> > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > >>
> > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > >>>> -----Original Message-----
> > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > >>>> Sent: 11 March 2019 18:02
> > > >>>> To: qemu-devel@nongnu.org
> > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > >>>> <Paul.Durrant@citrix.com>
> > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > >>>>
> > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > >>>>
> > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > >>>> an address which is not page aligned.
> > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > >> tolerating BARs smaller than that?
> > > >>>   Paul
> > > >>>
> > > >> Hi, Paul.
> > > >>
> > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > >> which device was affected?
> > > >>
> > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > >> spec says.
> > > >>
> > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > >> SMBus Controller (rev 04)
> > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > >> Controller (rev 30)
> > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > >>
> > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > >>
> > > >> Reviewing the code, I'm now wondering if the following in
> > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > >>                                      op);
> > > >>
> > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > >>
> > > >> BARs smaller than a page would also be a problem if BARs for different
> > > >> devices shared the same page.
> > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > >
> > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > to it.
> > >
> > > Do we know what the case is in practice for these USB controllers?
> > >
> > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > ensure that all bars smaller than 4k still have 4k alignment between
> > > them.  That way we can safely pass them through even when they are smaller.
> >
> > Andrew, thanks for checking the spec on the minimum BAR size.
> >
> > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> >
> > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > Controller (rev 04) (prog-if 30 [XHCI])
> >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> >
> > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > working.  With some added debugging output, you'll see that the same
> > page* is used for three of the BARs.
> >
> > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > 0xe1a30000 mfn 0xe1a30
> > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > 0xe0800000 mfn 0xe0800
> > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > 0xe1900000 mfn 0xe1900
> > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > 0xe1a2f000 mfn 0xe1a2f
>
> The patch below should prevent hvmloader from placing multiple BARs on
> the same page, could you give it a try?
>
> Note that this is not going to prevent the guest from moving those
> BARs around and place them in the same page, thus breaking the initial
> placement done by hvmloader.
>
> Thanks, Roger.

Hi, Roger.

I've minimally tested this.  Yes, this patch seems to place small BARs
into separate pages.  The linux stubdom and QEMU then use the spacing
as provided by hvmloader.

Thanks,
Jason


> ---8<---
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 0b708bf578..c433b34cd6 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -489,6 +489,10 @@ void pci_setup(void)
>
>          resource->base = base;
>
> +        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
> +             PCI_BASE_ADDRESS_SPACE_MEMORY )
> +            resource->base = ROUNDUP(resource->base, PAGE_SIZE);
> +
>          pci_writel(devfn, bar_reg, bar_data);
>          if (using_64bar)
>              pci_writel(devfn, bar_reg + 4, bar_data_upper);
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 7bca6418d2..b5554b5844 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -51,6 +51,8 @@ void __bug(char *file, int line) __attribute__((noreturn));
>  #define MB(mb) (mb##ULL << 20)
>  #define GB(gb) (gb##ULL << 30)
>
> +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +
>  static inline int test_bit(unsigned int b, const void *p)
>  {
>      return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > <andrew.cooper3@citrix.com> wrote:
> > > >
> > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > >> -----Original Message-----
> > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > >> Sent: 14 March 2019 18:16
> > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > >> <anthony.perard@citrix.com>
> > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > >>
> > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > >>>> -----Original Message-----
> > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > >>>> Sent: 11 March 2019 18:02
> > > > >>>> To: qemu-devel@nongnu.org
> > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > >>>> <Paul.Durrant@citrix.com>
> > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > >>>>
> > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > >>>>
> > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > >>>> an address which is not page aligned.
> > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > >> tolerating BARs smaller than that?
> > > > >>>   Paul
> > > > >>>
> > > > >> Hi, Paul.
> > > > >>
> > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > >> which device was affected?
> > > > >>
> > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > >> spec says.
> > > > >>
> > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > >> SMBus Controller (rev 04)
> > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > >> Controller (rev 30)
> > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > >>
> > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > >>
> > > > >> Reviewing the code, I'm now wondering if the following in
> > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > >>                                      op);
> > > > >>
> > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > >>
> > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > >> devices shared the same page.
> > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > >
> > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > to it.
> > > >
> > > > Do we know what the case is in practice for these USB controllers?
> > > >
> > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > them.  That way we can safely pass them through even when they are smaller.
> > >
> > > Andrew, thanks for checking the spec on the minimum BAR size.
> > >
> > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > >
> > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > Controller (rev 04) (prog-if 30 [XHCI])
> > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > >
> > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > working.  With some added debugging output, you'll see that the same
> > > page* is used for three of the BARs.
> > >
> > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > 0xe1a30000 mfn 0xe1a30
> > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > 0xe0800000 mfn 0xe0800
> > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > 0xe1900000 mfn 0xe1900
> > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > 0xe1a2f000 mfn 0xe1a2f
> >
> > The patch below should prevent hvmloader from placing multiple BARs on
> > the same page, could you give it a try?
> >
> > Note that this is not going to prevent the guest from moving those
> > BARs around and place them in the same page, thus breaking the initial
> > placement done by hvmloader.
> >
> > Thanks, Roger.
>
> Hi, Roger.
>
> I've minimally tested this.  Yes, this patch seems to place small BARs
> into separate pages.  The linux stubdom and QEMU then use the spacing
> as provided by hvmloader.

Roger,

Would you mind submitting this patch to Xen?

Thanks,
Jason

>
>
> > ---8<---
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index 0b708bf578..c433b34cd6 100644
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -489,6 +489,10 @@ void pci_setup(void)
> >
> >          resource->base = base;
> >
> > +        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
> > +             PCI_BASE_ADDRESS_SPACE_MEMORY )
> > +            resource->base = ROUNDUP(resource->base, PAGE_SIZE);
> > +
> >          pci_writel(devfn, bar_reg, bar_data);
> >          if (using_64bar)
> >              pci_writel(devfn, bar_reg + 4, bar_data_upper);
> > diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> > index 7bca6418d2..b5554b5844 100644
> > --- a/tools/firmware/hvmloader/util.h
> > +++ b/tools/firmware/hvmloader/util.h
> > @@ -51,6 +51,8 @@ void __bug(char *file, int line) __attribute__((noreturn));
> >  #define MB(mb) (mb##ULL << 20)
> >  #define GB(gb) (gb##ULL << 30)
> >
> > +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > +
> >  static inline int test_bit(unsigned int b, const void *p)
> >  {
> >      return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
> >

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > <andrew.cooper3@citrix.com> wrote:
> > > >
> > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > >> -----Original Message-----
> > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > >> Sent: 14 March 2019 18:16
> > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > >> <anthony.perard@citrix.com>
> > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > >>
> > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > >>>> -----Original Message-----
> > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > >>>> Sent: 11 March 2019 18:02
> > > > >>>> To: qemu-devel@nongnu.org
> > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > >>>> <Paul.Durrant@citrix.com>
> > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > >>>>
> > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > >>>>
> > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > >>>> an address which is not page aligned.
> > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > >> tolerating BARs smaller than that?
> > > > >>>   Paul
> > > > >>>
> > > > >> Hi, Paul.
> > > > >>
> > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > >> which device was affected?
> > > > >>
> > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > >> spec says.
> > > > >>
> > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > >> SMBus Controller (rev 04)
> > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > >> Controller (rev 30)
> > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > >>
> > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > >>
> > > > >> Reviewing the code, I'm now wondering if the following in
> > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > >>                                      op);
> > > > >>
> > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > >>
> > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > >> devices shared the same page.
> > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > >
> > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > to it.
> > > >
> > > > Do we know what the case is in practice for these USB controllers?
> > > >
> > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > them.  That way we can safely pass them through even when they are smaller.
> > >
> > > Andrew, thanks for checking the spec on the minimum BAR size.
> > >
> > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > >
> > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > Controller (rev 04) (prog-if 30 [XHCI])
> > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > >
> > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > working.  With some added debugging output, you'll see that the same
> > > page* is used for three of the BARs.
> > >
> > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > 0xe1a30000 mfn 0xe1a30
> > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > 0xe0800000 mfn 0xe0800
> > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > 0xe1900000 mfn 0xe1900
> > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > 0xe1a2f000 mfn 0xe1a2f
> >
> > The patch below should prevent hvmloader from placing multiple BARs on
> > the same page, could you give it a try?
> >
> > Note that this is not going to prevent the guest from moving those
> > BARs around and place them in the same page, thus breaking the initial
> > placement done by hvmloader.
> >
> > Thanks, Roger.
>
> Hi, Roger.
>
> I've minimally tested this.  Yes, this patch seems to place small BARs
> into separate pages.  The linux stubdom and QEMU then use the spacing
> as provided by hvmloader.

Roger,

Would you mind submitting this patch to Xen?

Thanks,
Jason

>
>
> > ---8<---
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index 0b708bf578..c433b34cd6 100644
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -489,6 +489,10 @@ void pci_setup(void)
> >
> >          resource->base = base;
> >
> > +        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
> > +             PCI_BASE_ADDRESS_SPACE_MEMORY )
> > +            resource->base = ROUNDUP(resource->base, PAGE_SIZE);
> > +
> >          pci_writel(devfn, bar_reg, bar_data);
> >          if (using_64bar)
> >              pci_writel(devfn, bar_reg + 4, bar_data_upper);
> > diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> > index 7bca6418d2..b5554b5844 100644
> > --- a/tools/firmware/hvmloader/util.h
> > +++ b/tools/firmware/hvmloader/util.h
> > @@ -51,6 +51,8 @@ void __bug(char *file, int line) __attribute__((noreturn));
> >  #define MB(mb) (mb##ULL << 20)
> >  #define GB(gb) (gb##ULL << 30)
> >
> > +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > +
> >  static inline int test_bit(unsigned int b, const void *p)
> >  {
> >      return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Roger Pau Monné]

On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > > <andrew.cooper3@citrix.com> wrote:
> > > > >
> > > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > > >> -----Original Message-----
> > > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > >> Sent: 14 March 2019 18:16
> > > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > > >> <anthony.perard@citrix.com>
> > > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > >>
> > > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > > >>>> -----Original Message-----
> > > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > >>>> Sent: 11 March 2019 18:02
> > > > > >>>> To: qemu-devel@nongnu.org
> > > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > > >>>> <Paul.Durrant@citrix.com>
> > > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > >>>>
> > > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > > >>>>
> > > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > > >>>> an address which is not page aligned.
> > > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > > >> tolerating BARs smaller than that?
> > > > > >>>   Paul
> > > > > >>>
> > > > > >> Hi, Paul.
> > > > > >>
> > > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > > >> which device was affected?
> > > > > >>
> > > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > > >> spec says.
> > > > > >>
> > > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > > >> SMBus Controller (rev 04)
> > > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > > >> Controller (rev 30)
> > > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > > >>
> > > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > > >>
> > > > > >> Reviewing the code, I'm now wondering if the following in
> > > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > > >>                                      op);
> > > > > >>
> > > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > > >>
> > > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > > >> devices shared the same page.
> > > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > > >
> > > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > > to it.
> > > > >
> > > > > Do we know what the case is in practice for these USB controllers?
> > > > >
> > > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > > them.  That way we can safely pass them through even when they are smaller.
> > > >
> > > > Andrew, thanks for checking the spec on the minimum BAR size.
> > > >
> > > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > > >
> > > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > > Controller (rev 04) (prog-if 30 [XHCI])
> > > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > > >
> > > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > > working.  With some added debugging output, you'll see that the same
> > > > page* is used for three of the BARs.
> > > >
> > > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > > 0xe1a30000 mfn 0xe1a30
> > > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > > 0xe0800000 mfn 0xe0800
> > > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > > 0xe1900000 mfn 0xe1900
> > > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > > 0xe1a2f000 mfn 0xe1a2f
> > >
> > > The patch below should prevent hvmloader from placing multiple BARs on
> > > the same page, could you give it a try?
> > >
> > > Note that this is not going to prevent the guest from moving those
> > > BARs around and place them in the same page, thus breaking the initial
> > > placement done by hvmloader.
> > >
> > > Thanks, Roger.
> >
> > Hi, Roger.
> >
> > I've minimally tested this.  Yes, this patch seems to place small BARs
> > into separate pages.  The linux stubdom and QEMU then use the spacing
> > as provided by hvmloader.
> 
> Roger,
> 
> Would you mind submitting this patch to Xen?

Hm, I'm half minded regarding this patch. It feels more like a bandaid
than a proper solution. Mapping BARs not multiple of page-sizes is
dangerous because AFAIK there's no entity that asserts there isn't any
other BAR from a different device on the same page, and hence you
might end up mapping some MMIO region from another device
inadvertently.

Anyway, I can formally submit the patch since it's no worse than
what's currently done, but I would clearly state this is not safe in
it's current state.

Thanks, Roger.

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Roger Pau Monné]

On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > > <andrew.cooper3@citrix.com> wrote:
> > > > >
> > > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > > >> -----Original Message-----
> > > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > >> Sent: 14 March 2019 18:16
> > > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > > >> <anthony.perard@citrix.com>
> > > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > >>
> > > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > > >>>> -----Original Message-----
> > > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > >>>> Sent: 11 March 2019 18:02
> > > > > >>>> To: qemu-devel@nongnu.org
> > > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > > >>>> <Paul.Durrant@citrix.com>
> > > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > >>>>
> > > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > > >>>>
> > > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > > >>>> an address which is not page aligned.
> > > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > > >> tolerating BARs smaller than that?
> > > > > >>>   Paul
> > > > > >>>
> > > > > >> Hi, Paul.
> > > > > >>
> > > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > > >> which device was affected?
> > > > > >>
> > > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > > >> spec says.
> > > > > >>
> > > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > > >> SMBus Controller (rev 04)
> > > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > > >> Controller (rev 30)
> > > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > > >>
> > > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > > >>
> > > > > >> Reviewing the code, I'm now wondering if the following in
> > > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > > >>                                      op);
> > > > > >>
> > > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > > >>
> > > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > > >> devices shared the same page.
> > > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > > >
> > > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > > to it.
> > > > >
> > > > > Do we know what the case is in practice for these USB controllers?
> > > > >
> > > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > > them.  That way we can safely pass them through even when they are smaller.
> > > >
> > > > Andrew, thanks for checking the spec on the minimum BAR size.
> > > >
> > > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > > >
> > > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > > Controller (rev 04) (prog-if 30 [XHCI])
> > > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > > >
> > > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > > working.  With some added debugging output, you'll see that the same
> > > > page* is used for three of the BARs.
> > > >
> > > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > > 0xe1a30000 mfn 0xe1a30
> > > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > > 0xe0800000 mfn 0xe0800
> > > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > > 0xe1900000 mfn 0xe1900
> > > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > > 0xe1a2f000 mfn 0xe1a2f
> > >
> > > The patch below should prevent hvmloader from placing multiple BARs on
> > > the same page, could you give it a try?
> > >
> > > Note that this is not going to prevent the guest from moving those
> > > BARs around and place them in the same page, thus breaking the initial
> > > placement done by hvmloader.
> > >
> > > Thanks, Roger.
> >
> > Hi, Roger.
> >
> > I've minimally tested this.  Yes, this patch seems to place small BARs
> > into separate pages.  The linux stubdom and QEMU then use the spacing
> > as provided by hvmloader.
> 
> Roger,
> 
> Would you mind submitting this patch to Xen?

Hm, I'm half minded regarding this patch. It feels more like a bandaid
than a proper solution. Mapping BARs not multiple of page-sizes is
dangerous because AFAIK there's no entity that asserts there isn't any
other BAR from a different device on the same page, and hence you
might end up mapping some MMIO region from another device
inadvertently.

Anyway, I can formally submit the patch since it's no worse than
what's currently done, but I would clearly state this is not safe in
it's current state.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Tue, Jan 14, 2020 at 5:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> > On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > >
> > > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > > > <andrew.cooper3@citrix.com> wrote:
> > > > > >
> > > > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > > > >> -----Original Message-----
> > > > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > > >> Sent: 14 March 2019 18:16
> > > > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > > > >> <anthony.perard@citrix.com>
> > > > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > > >>
> > > > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > > > >>>> -----Original Message-----
> > > > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > > >>>> Sent: 11 March 2019 18:02
> > > > > > >>>> To: qemu-devel@nongnu.org
> > > > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > > > >>>> <Paul.Durrant@citrix.com>
> > > > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > > >>>>
> > > > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > > > >>>>
> > > > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > > > >>>> an address which is not page aligned.
> > > > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > > > >> tolerating BARs smaller than that?
> > > > > > >>>   Paul
> > > > > > >>>
> > > > > > >> Hi, Paul.
> > > > > > >>
> > > > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > > > >> which device was affected?
> > > > > > >>
> > > > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > > > >> spec says.
> > > > > > >>
> > > > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > > > >> SMBus Controller (rev 04)
> > > > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > > > >> Controller (rev 30)
> > > > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > > > >>
> > > > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > > > >>
> > > > > > >> Reviewing the code, I'm now wondering if the following in
> > > > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > > > >>                                      op);
> > > > > > >>
> > > > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > > > >>
> > > > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > > > >> devices shared the same page.
> > > > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > > > >
> > > > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > > > to it.
> > > > > >
> > > > > > Do we know what the case is in practice for these USB controllers?
> > > > > >
> > > > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > > > them.  That way we can safely pass them through even when they are smaller.
> > > > >
> > > > > Andrew, thanks for checking the spec on the minimum BAR size.
> > > > >
> > > > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > > > >
> > > > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > > > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > > > Controller (rev 04) (prog-if 30 [XHCI])
> > > > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > > > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > > > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > > > >
> > > > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > > > working.  With some added debugging output, you'll see that the same
> > > > > page* is used for three of the BARs.
> > > > >
> > > > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > > > 0xe1a30000 mfn 0xe1a30
> > > > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > > > 0xe0800000 mfn 0xe0800
> > > > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > > > 0xe1900000 mfn 0xe1900
> > > > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > > > 0xe1a2f000 mfn 0xe1a2f
> > > >
> > > > The patch below should prevent hvmloader from placing multiple BARs on
> > > > the same page, could you give it a try?
> > > >
> > > > Note that this is not going to prevent the guest from moving those
> > > > BARs around and place them in the same page, thus breaking the initial
> > > > placement done by hvmloader.
> > > >
> > > > Thanks, Roger.
> > >
> > > Hi, Roger.
> > >
> > > I've minimally tested this.  Yes, this patch seems to place small BARs
> > > into separate pages.  The linux stubdom and QEMU then use the spacing
> > > as provided by hvmloader.
> >
> > Roger,
> >
> > Would you mind submitting this patch to Xen?
>
> Hm, I'm half minded regarding this patch. It feels more like a bandaid
> than a proper solution. Mapping BARs not multiple of page-sizes is
> dangerous because AFAIK there's no entity that asserts there isn't any
> other BAR from a different device on the same page, and hence you
> might end up mapping some MMIO region from another device
> inadvertently.

We have the guest, linux stubdom with qemu, & dom0. Are you concerned
that all of them need a minimum of page alignment?

Linux PCI subsytem has an option resource_alignment that can be
applied to either a single device or all devices.  Booting with
pci=resource_aligment=4096 will align each device to a page.  Do you
think pciback should force resource_alignment=4096 for dom0?  Are
there other MMIO ranges to be concerned about adjacent to BARs?

On my one test machine with a BAR smaller than 4096, the firmware
already sets an alignment of 4096.  Linux dom0 seems to keep the
firmware BAR alignment by default.

> Anyway, I can formally submit the patch since it's no worse than
> what's currently done, but I would clearly state this is not safe in
> it's current state.

Regards,
Jason

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Jason Andryuk]

On Tue, Jan 14, 2020 at 5:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> > On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > >
> > > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 01:28:47PM -0400, Jason Andryuk wrote:
> > > > > On Fri, Mar 15, 2019 at 12:28 PM Andrew Cooper
> > > > > <andrew.cooper3@citrix.com> wrote:
> > > > > >
> > > > > > On 15/03/2019 09:17, Paul Durrant wrote:
> > > > > > >> -----Original Message-----
> > > > > > >> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > > >> Sent: 14 March 2019 18:16
> > > > > > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > > >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon
> > > > > > >> Gaiser <simon@invisiblethingslab.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > > > > >> <anthony.perard@citrix.com>
> > > > > > >> Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > > >>
> > > > > > >> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> > > > > > >>>> -----Original Message-----
> > > > > > >>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > > > > >>>> Sent: 11 March 2019 18:02
> > > > > > >>>> To: qemu-devel@nongnu.org
> > > > > > >>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
> > > > > > >>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
> > > > > > >>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> > > > > > >>>> <Paul.Durrant@citrix.com>
> > > > > > >>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> > > > > > >>>>
> > > > > > >>>> From: Simon Gaiser <simon@invisiblethingslab.com>
> > > > > > >>>>
> > > > > > >>>> If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > > > > > >>>> an address which is not page aligned.
> > > > > > >>> IIRC the PCI spec says that the minimum memory region size should be at least 4k. Should we even be
> > > > > > >> tolerating BARs smaller than that?
> > > > > > >>>   Paul
> > > > > > >>>
> > > > > > >> Hi, Paul.
> > > > > > >>
> > > > > > >> Simon found this, so it affects a real device.  Simon, do you recall
> > > > > > >> which device was affected?
> > > > > > >>
> > > > > > >> I think BARs only need to be power-of-two size and aligned, and 4k is
> > > > > > >> not a minimum.  16bytes may be a minimum, but I don't know what the
> > > > > > >> spec says.
> > > > > > >>
> > > > > > >> On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
> > > > > > >> 00:16.0 Communication controller: Intel Corporation 7 Series/C210
> > > > > > >> Series Chipset Family MEI Controller #1 (rev 04)
> > > > > > >>    Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
> > > > > > >> 00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
> > > > > > >> Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
> > > > > > >>    Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
> > > > > > >> 00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
> > > > > > >> SMBus Controller (rev 04)
> > > > > > >>    Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
> > > > > > >> 02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
> > > > > > >> Controller (rev 30)
> > > > > > >>    Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
> > > > > > >>
> > > > > > >> These examples are all 4K aligned, so this is not an issue on this machine.
> > > > > > >>
> > > > > > >> Reviewing the code, I'm now wondering if the following in
> > > > > > >> hw/xen/xen_pt.c:xen_pt_region_update is wrong:        rc =
> > > > > > >> xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > > > >>                                      XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
> > > > > > >>                                      XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
> > > > > > >>                                      XEN_PFN(size + XC_PAGE_SIZE - 1),
> > > > > > >>                                      op);
> > > > > > >>
> > > > > > >> If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
> > > > > > >> in would be 0xd0501000 which is past the actual location.  Should the
> > > > > > >> call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
> > > > > > >>
> > > > > > >> BARs smaller than a page would also be a problem if BARs for different
> > > > > > >> devices shared the same page.
> > > > > > > Exactly. We cannot pass them through with any degree of safety (not that passthrough of an arbitrary device is a particularly safe thing to do anyway). The xen-pt code would instead need to trap those BARs and perform the accesses to the real BAR itself. Ultimately though I think we should be retiring the xen-pt code in favour of a standalone emulator.
> > > > > >
> > > > > > It doesn't matter if the BAR is smaller than 4k, if there are holes next
> > > > > > to it.
> > > > > >
> > > > > > Do we know what the case is in practice for these USB controllers?
> > > > > >
> > > > > > If the worst comes to the worst, we can re-enumerate the PCI bus to
> > > > > > ensure that all bars smaller than 4k still have 4k alignment between
> > > > > > them.  That way we can safely pass them through even when they are smaller.
> > > > >
> > > > > Andrew, thanks for checking the spec on the minimum BAR size.
> > > > >
> > > > > Dropping the Round PCI region patch from QMEU, the guest HVM will have:
> > > > >
> > > > > 00:06.0 SD Host controller: Ricoh Co Ltd PCIe SDXC/MMC Host Controller (rev 07)
> > > > >     Memory at f2028800 (32-bit, non-prefetchable) [size=256]
> > > > > 00:07.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> > > > > Controller (rev 04) (prog-if 30 [XHCI])
> > > > >     Memory at f2024000 (64-bit, non-prefetchable) [size=8K]
> > > > > 00:08.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > > Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> > > > >     Memory at f2028000 (32-bit, non-prefetchable) [size=1K]
> > > > > 00:09.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset
> > > > > Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> > > > >     Memory at f2028400 (32-bit, non-prefetchable) [size=1K]
> > > > >
> > > > > 00:09.0, 00:08.0 & 00:06.0 all share the same page.  Only 00:08.0 is
> > > > > working.  With some added debugging output, you'll see that the same
> > > > > page* is used for three of the BARs.
> > > > >
> > > > > [00:06.0] mapping guest_addr 0xf2028800 gfn 0xf2028 to maddr
> > > > > 0xe1a30000 mfn 0xe1a30
> > > > > [00:07.0] mapping guest_addr 0xf2024000 gfn 0xf2024 to maddr
> > > > > 0xe0800000 mfn 0xe0800
> > > > > [00:09.0] mapping guest_addr 0xf2028400 gfn 0xf2028 to maddr
> > > > > 0xe1900000 mfn 0xe1900
> > > > > [00:08.0] mapping guest_addr 0xf2028000 gfn 0xf2028 to maddr
> > > > > 0xe1a2f000 mfn 0xe1a2f
> > > >
> > > > The patch below should prevent hvmloader from placing multiple BARs on
> > > > the same page, could you give it a try?
> > > >
> > > > Note that this is not going to prevent the guest from moving those
> > > > BARs around and place them in the same page, thus breaking the initial
> > > > placement done by hvmloader.
> > > >
> > > > Thanks, Roger.
> > >
> > > Hi, Roger.
> > >
> > > I've minimally tested this.  Yes, this patch seems to place small BARs
> > > into separate pages.  The linux stubdom and QEMU then use the spacing
> > > as provided by hvmloader.
> >
> > Roger,
> >
> > Would you mind submitting this patch to Xen?
>
> Hm, I'm half minded regarding this patch. It feels more like a bandaid
> than a proper solution. Mapping BARs not multiple of page-sizes is
> dangerous because AFAIK there's no entity that asserts there isn't any
> other BAR from a different device on the same page, and hence you
> might end up mapping some MMIO region from another device
> inadvertently.

We have the guest, linux stubdom with qemu, & dom0. Are you concerned
that all of them need a minimum of page alignment?

Linux PCI subsytem has an option resource_alignment that can be
applied to either a single device or all devices.  Booting with
pci=resource_aligment=4096 will align each device to a page.  Do you
think pciback should force resource_alignment=4096 for dom0?  Are
there other MMIO ranges to be concerned about adjacent to BARs?

On my one test machine with a BAR smaller than 4096, the firmware
already sets an alignment of 4096.  Linux dom0 seems to keep the
firmware BAR alignment by default.

> Anyway, I can formally submit the patch since it's no worse than
> what's currently done, but I would clearly state this is not safe in
> it's current state.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Roger Pau Monné]

On Tue, Jan 14, 2020 at 09:41:46AM -0500, Jason Andryuk wrote:
> On Tue, Jan 14, 2020 at 5:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> > > On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > > >
> > > > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > > >
> > > > > The patch below should prevent hvmloader from placing multiple BARs on
> > > > > the same page, could you give it a try?
> > > > >
> > > > > Note that this is not going to prevent the guest from moving those
> > > > > BARs around and place them in the same page, thus breaking the initial
> > > > > placement done by hvmloader.
> > > > >
> > > > > Thanks, Roger.
> > > >
> > > > Hi, Roger.
> > > >
> > > > I've minimally tested this.  Yes, this patch seems to place small BARs
> > > > into separate pages.  The linux stubdom and QEMU then use the spacing
> > > > as provided by hvmloader.
> > >
> > > Roger,
> > >
> > > Would you mind submitting this patch to Xen?
> >
> > Hm, I'm half minded regarding this patch. It feels more like a bandaid
> > than a proper solution. Mapping BARs not multiple of page-sizes is
> > dangerous because AFAIK there's no entity that asserts there isn't any
> > other BAR from a different device on the same page, and hence you
> > might end up mapping some MMIO region from another device
> > inadvertently.
> 
> We have the guest, linux stubdom with qemu, & dom0. Are you concerned
> that all of them need a minimum of page alignment?

No, not really. The hardware domain (dom0 in normal deployments)
should be the one that makes sure there are no BARs sharing physical
pages.

> Linux PCI subsytem has an option resource_alignment that can be
> applied to either a single device or all devices.  Booting with
> pci=resource_aligment=4096 will align each device to a page.  Do you
> think pciback should force resource_alignment=4096 for dom0?

Ideally Xen should keep track of the BARs position and size and refuse
to passthrough devices that have BARs sharing a page with other
devices BARs.

> Are
> there other MMIO ranges to be concerned about adjacent to BARs?

IIRC you can have two BARs of different devices in the same 4K page,
BARs are only aligned to it's size, so BARs smaller than 4K are not
required to be page aligned.

> On my one test machine with a BAR smaller than 4096, the firmware
> already sets an alignment of 4096.  Linux dom0 seems to keep the
> firmware BAR alignment by default.

The PCI spec recommend BARs to be sized to a multiple of a page size, but
sadly that's not a mandatory requirement.

Will submit the patch now, thanks for the ping, I completely forgot
about this TBH.

Roger.

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Roger Pau Monné]

On Tue, Jan 14, 2020 at 09:41:46AM -0500, Jason Andryuk wrote:
> On Tue, Jan 14, 2020 at 5:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Mon, Jan 13, 2020 at 02:01:47PM -0500, Jason Andryuk wrote:
> > > On Fri, Mar 22, 2019 at 3:43 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> > > >
> > > > On Thu, Mar 21, 2019 at 11:09 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > > >
> > > > > The patch below should prevent hvmloader from placing multiple BARs on
> > > > > the same page, could you give it a try?
> > > > >
> > > > > Note that this is not going to prevent the guest from moving those
> > > > > BARs around and place them in the same page, thus breaking the initial
> > > > > placement done by hvmloader.
> > > > >
> > > > > Thanks, Roger.
> > > >
> > > > Hi, Roger.
> > > >
> > > > I've minimally tested this.  Yes, this patch seems to place small BARs
> > > > into separate pages.  The linux stubdom and QEMU then use the spacing
> > > > as provided by hvmloader.
> > >
> > > Roger,
> > >
> > > Would you mind submitting this patch to Xen?
> >
> > Hm, I'm half minded regarding this patch. It feels more like a bandaid
> > than a proper solution. Mapping BARs not multiple of page-sizes is
> > dangerous because AFAIK there's no entity that asserts there isn't any
> > other BAR from a different device on the same page, and hence you
> > might end up mapping some MMIO region from another device
> > inadvertently.
> 
> We have the guest, linux stubdom with qemu, & dom0. Are you concerned
> that all of them need a minimum of page alignment?

No, not really. The hardware domain (dom0 in normal deployments)
should be the one that makes sure there are no BARs sharing physical
pages.

> Linux PCI subsytem has an option resource_alignment that can be
> applied to either a single device or all devices.  Booting with
> pci=resource_aligment=4096 will align each device to a page.  Do you
> think pciback should force resource_alignment=4096 for dom0?

Ideally Xen should keep track of the BARs position and size and refuse
to passthrough devices that have BARs sharing a page with other
devices BARs.

> Are
> there other MMIO ranges to be concerned about adjacent to BARs?

IIRC you can have two BARs of different devices in the same 4K page,
BARs are only aligned to it's size, so BARs smaller than 4K are not
required to be page aligned.

> On my one test machine with a BAR smaller than 4096, the firmware
> already sets an alignment of 4096.  Linux dom0 seems to keep the
> firmware BAR alignment by default.

The PCI spec recommend BARs to be sized to a multiple of a page size, but
sadly that's not a mandatory requirement.

Will submit the patch now, thanks for the ping, I completely forgot
about this TBH.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

RE: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Durrant, Paul]

> -----Original Message-----
> 
> > Linux PCI subsytem has an option resource_alignment that can be
> > applied to either a single device or all devices.  Booting with
> > pci=resource_aligment=4096 will align each device to a page.  Do you
> > think pciback should force resource_alignment=4096 for dom0?
>

That sounds like a good idea.
 
> Ideally Xen should keep track of the BARs position and size and refuse
> to passthrough devices that have BARs sharing a page with other
> devices BARs.
> 
> > Are
> > there other MMIO ranges to be concerned about adjacent to BARs?
> 
> IIRC you can have two BARs of different devices in the same 4K page,
> BARs are only aligned to it's size, so BARs smaller than 4K are not
> required to be page aligned.

If we had a notion of assignment groups for this, as well as devices sharing requester id, then Xen would not need to refuse pass-through, it would just require that all devices sharing the page were passed through as a unit.

  Paul

Re: [Xen-devel] [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE

[Durrant, Paul]

> -----Original Message-----
> 
> > Linux PCI subsytem has an option resource_alignment that can be
> > applied to either a single device or all devices.  Booting with
> > pci=resource_aligment=4096 will align each device to a page.  Do you
> > think pciback should force resource_alignment=4096 for dom0?
>

That sounds like a good idea.
 
> Ideally Xen should keep track of the BARs position and size and refuse
> to passthrough devices that have BARs sharing a page with other
> devices BARs.
> 
> > Are
> > there other MMIO ranges to be concerned about adjacent to BARs?
> 
> IIRC you can have two BARs of different devices in the same 4K page,
> BARs are only aligned to it's size, so BARs smaller than 4K are not
> required to be page aligned.

If we had a notion of assignment groups for this, as well as devices sharing requester id, then Xen would not need to refuse pass-through, it would just require that all devices sharing the page were passed through as a unit.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel