[PATCH v3 0/6] Fix PCI passthrough for HVM with stubdomain

[PATCH v3 0/6] Fix PCI passthrough for HVM with stubdomain

[Marek Marczykowski-Górecki]

In this version, I add PHYSDEVOP_msi_msix_set_enable to allow stubdomain
enabling MSI after mapping it. This patch is rather RFC and probably will
need adjustments (see comments after commit message there), if physdevop will
be the way to go.

Related article:
https://www.qubes-os.org/news/2017/10/18/msi-support/

Changes in v2:
 - new "xen/x86: Allow stubdom access to irq created for msi" patch
 - applied review comments from v1
Changes is v3:
 - apply suggestions by Roger
 - add PHYSDEVOP_msi_msix_set_enable

Marek Marczykowski-Górecki (5):
  libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
  libxl: attach PCI device to qemu only after setting pciback/pcifront
  libxl: don't try to manipulate json config for stubdomain
  xen/x86: add PHYSDEVOP_msi_msix_set_enable
  tools/libxc: add wrapper for PHYSDEVOP_msi_msix_set_enable

Simon Gaiser (1):
  xen/x86: Allow stubdom access to irq created for msi.

 tools/libxc/include/xenctrl.h |  7 ++++-
 tools/libxc/xc_physdev.c      | 21 ++++++++++++-
 tools/libxl/libxl_pci.c       | 61 ++++++++++++++++++++++++------------
 xen/arch/x86/irq.c            | 23 ++++++++++++++-
 xen/arch/x86/msi.c            | 16 +++++++++-
 xen/arch/x86/physdev.c        | 33 +++++++++++++++++++-
 xen/include/asm-x86/msi.h     |  1 +-
 xen/include/public/physdev.h  | 13 ++++++++-
 8 files changed, 155 insertions(+), 20 deletions(-)

base-commit: 93a62c544e20ba9e141e411bbaae3d65259d13a3
-- 
git-series 0.9.1

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

[PATCH v3 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Marek Marczykowski-Górecki]

HVM domains use IOMMU and device model assistance for communicating with
PCI devices, xen-pcifront/pciback isn't directly needed by HVM domain.
But pciback serve also second function - it reset the device when it is
deassigned from the guest and for this reason pciback needs to be used
with HVM domain too.
When HVM domain has device model in stubdomain, attaching xen-pciback to
the target domain itself may prevent attaching xen-pciback to the
(PV) stubdomain, effectively breaking PCI passthrough.

Fix this by attaching pciback only to one domain: if PV stubdomain is in
use, let it be stubdomain (the commit prevents attaching device to target
HVM in this case); otherwise, attach it to the target domain.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
 - previously called "libxl: attach xen-pciback only to PV domains"
 - instead of excluding all HVMs, change the condition to what actually
   matters here - check if stubdomain is in use; this way xen-pciback is
   always in use (either for the target domain, or it's stubdomain),
   fixing PCI reset by xen-pciback concerns
Changes in v3:
 - adjust commit message
---
 tools/libxl/libxl_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 87afa03..3b6b23c 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1106,7 +1106,7 @@ out:
         }
     }
 
-    if (!starting)
+    if (!starting && !libxl_get_stubdom_id(CTX, domid))
         rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
     else
         rc = 0;
@@ -1302,7 +1302,7 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
         }
     }
 
-    if (d_config->num_pcidevs > 0) {
+    if (d_config->num_pcidevs > 0 && !libxl_get_stubdom_id(CTX, domid)) {
         rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
             d_config->num_pcidevs);
         if (rc < 0) {
-- 
git-series 0.9.1

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

Re: [PATCH v3 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:12AM +0100, Marek Marczykowski-Górecki wrote:
> HVM domains use IOMMU and device model assistance for communicating with
> PCI devices, xen-pcifront/pciback isn't directly needed by HVM domain.
> But pciback serve also second function - it reset the device when it is
> deassigned from the guest and for this reason pciback needs to be used
> with HVM domain too.
> When HVM domain has device model in stubdomain, attaching xen-pciback to
> the target domain itself may prevent attaching xen-pciback to the
> (PV) stubdomain, effectively breaking PCI passthrough.
> 
> Fix this by attaching pciback only to one domain: if PV stubdomain is in
> use, let it be stubdomain (the commit prevents attaching device to target
> HVM in this case); otherwise, attach it to the target domain.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v3 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Marek Marczykowski-Górecki]

When qemu is running in stubdomain, handling "pci-ins" command will fail
if pcifront is not initialized already. Fix this by sending such command
only after confirming that pciback/front is running.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
- Fixed code style since previous version.
---
 tools/libxl/libxl_pci.c |  9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 3b6b23c..1bde537 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1191,6 +1191,7 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int orig_vdev, pfunc_mask;
+    char *be_path;
     libxl_device_pci *assigned;
     int num_assigned, i, rc;
     int stubdomid = 0;
@@ -1245,6 +1246,14 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
         rc = do_pci_add(gc, stubdomid, &pcidev_s, 0);
         if ( rc )
             goto out;
+        /* Wait for the device actually being connected, otherwise device model
+         * running there will fail to find the device. */
+        be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0",
+                                 libxl__xs_get_dompath(gc, 0), stubdomid);
+        rc = libxl__wait_for_backend(gc, be_path,
+                                     GCSPRINTF("%d", XenbusStateConnected));
+        if (rc)
+            goto out;
     }
 
     orig_vdev = pcidev->vdevfn & ~7U;
-- 
git-series 0.9.1

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

Re: [PATCH v3 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:13AM +0100, Marek Marczykowski-Górecki wrote:
> When qemu is running in stubdomain, handling "pci-ins" command will fail
> if pcifront is not initialized already. Fix this by sending such command
> only after confirming that pciback/front is running.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v3 3/6] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki]

Stubdomain do not have it's own config file - its configuration is
derived from target domains. Do not try to manipulate it when attaching
PCI device.

This bug prevented starting HVM with stubdomain and PCI passthrough
device attached.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
 - skip libxl__dm_check_start too, as stubdomain is guaranteed to be
   running at this stage already
 - do not init d_config at all, as it is used only for json manipulation
---
 tools/libxl/libxl_pci.c | 48 ++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 1bde537..8d159cf 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -120,10 +120,14 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     libxl_domain_config d_config;
     libxl_device_pci pcidev_saved;
     libxl__domain_userdata_lock *lock = NULL;
+    bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_pci_init(&pcidev_saved);
-    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
+    /* Stubdomain doesn't have own config. */
+    if (!is_stubdomain) {
+        libxl_domain_config_init(&d_config);
+        libxl_device_pci_init(&pcidev_saved);
+        libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
+    }
 
     be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
                                                 LIBXL__DEVICE_KIND_PCI);
@@ -152,27 +156,33 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     GCNEW(device);
     libxl__device_from_pcidev(gc, domid, pcidev, device);
 
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
-        rc = ERROR_LOCK_FAIL;
-        goto out;
-    }
+    /* Stubdomain config is derived from its target domain, it doesn't have
+       its own file */
+    if (!is_stubdomain) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
 
-    rc = libxl__get_domain_configuration(gc, domid, &d_config);
-    if (rc) goto out;
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
 
-    device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
-                             &pcidev_saved);
+        device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
+                                 &pcidev_saved);
 
-    rc = libxl__dm_check_start(gc, &d_config, domid);
-    if (rc) goto out;
+        rc = libxl__dm_check_start(gc, &d_config, domid);
+        if (rc) goto out;
+    }
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
 
-        rc = libxl__set_domain_configuration(gc, domid, &d_config);
-        if (rc) goto out;
+        if (lock) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
 
         libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
 
@@ -184,8 +194,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
 out:
     libxl__xs_transaction_abort(gc, &t);
     if (lock) libxl__unlock_domain_userdata(lock);
-    libxl_device_pci_dispose(&pcidev_saved);
-    libxl_domain_config_dispose(&d_config);
+    if (!is_stubdomain) {
+        libxl_device_pci_dispose(&pcidev_saved);
+        libxl_domain_config_dispose(&d_config);
+    }
     return rc;
 }
 
-- 
git-series 0.9.1

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

Re: [PATCH v3 3/6] libxl: don't try to manipulate json config for stubdomain

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:14AM +0100, Marek Marczykowski-Górecki wrote:
> Stubdomain do not have it's own config file - its configuration is
> derived from target domains. Do not try to manipulate it when attaching
> PCI device.
> 

So if we add the same configuration to stubdom as well, what would
happen? I guess libxl will try to attach the PCI devices to both the
stubdom and DomU?

> This bug prevented starting HVM with stubdomain and PCI passthrough
> device attached.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v3:
>  - skip libxl__dm_check_start too, as stubdomain is guaranteed to be
>    running at this stage already
>  - do not init d_config at all, as it is used only for json manipulation
> ---
>  tools/libxl/libxl_pci.c | 48 ++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 1bde537..8d159cf 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -120,10 +120,14 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
>      libxl_domain_config d_config;
>      libxl_device_pci pcidev_saved;
>      libxl__domain_userdata_lock *lock = NULL;
> +    bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
>  
> -    libxl_domain_config_init(&d_config);
> -    libxl_device_pci_init(&pcidev_saved);
> -    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> +    /* Stubdomain doesn't have own config. */
> +    if (!is_stubdomain) {
> +        libxl_domain_config_init(&d_config);
> +        libxl_device_pci_init(&pcidev_saved);
> +        libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> +    }
>  
>      be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
>                                                  LIBXL__DEVICE_KIND_PCI);
> @@ -152,27 +156,33 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
>      GCNEW(device);
>      libxl__device_from_pcidev(gc, domid, pcidev, device);
>  
> -    lock = libxl__lock_domain_userdata(gc, domid);
> -    if (!lock) {
> -        rc = ERROR_LOCK_FAIL;
> -        goto out;
> -    }
> +    /* Stubdomain config is derived from its target domain, it doesn't have
> +       its own file */

Although comment style isn't specified in CODING_STYLE, I would like to
fix this to 

    /*
     * Stubdom ...
     * ... own file.
     */

> +    if (!is_stubdomain) {
> +        lock = libxl__lock_domain_userdata(gc, domid);
> +        if (!lock) {
> +            rc = ERROR_LOCK_FAIL;
> +            goto out;
> +        }
>  
> -    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> -    if (rc) goto out;
> +        rc = libxl__get_domain_configuration(gc, domid, &d_config);
> +        if (rc) goto out;
>  
> -    device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> -                             &pcidev_saved);
> +        device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> +                                 &pcidev_saved);
>  
> -    rc = libxl__dm_check_start(gc, &d_config, domid);
> -    if (rc) goto out;
> +        rc = libxl__dm_check_start(gc, &d_config, domid);
> +        if (rc) goto out;
> +    }
>  
>      for (;;) {
>          rc = libxl__xs_transaction_start(gc, &t);
>          if (rc) goto out;
>  
> -        rc = libxl__set_domain_configuration(gc, domid, &d_config);
> -        if (rc) goto out;
> +        if (lock) {
> +            rc = libxl__set_domain_configuration(gc, domid, &d_config);
> +            if (rc) goto out;
> +        }
>  
>          libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
>  
> @@ -184,8 +194,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
>  out:
>      libxl__xs_transaction_abort(gc, &t);
>      if (lock) libxl__unlock_domain_userdata(lock);
> -    libxl_device_pci_dispose(&pcidev_saved);
> -    libxl_domain_config_dispose(&d_config);
> +    if (!is_stubdomain) {
> +        libxl_device_pci_dispose(&pcidev_saved);
> +        libxl_domain_config_dispose(&d_config);
> +    }
>      return rc;
>  }
>  
> -- 
> git-series 0.9.1

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

Re: [PATCH v3 3/6] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki]

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

On Mon, Jan 28, 2019 at 02:41:15PM +0000, Wei Liu wrote:
> On Sat, Jan 26, 2019 at 03:31:14AM +0100, Marek Marczykowski-Górecki wrote:
> > Stubdomain do not have it's own config file - its configuration is
> > derived from target domains. Do not try to manipulate it when attaching
> > PCI device.
> > 
> 
> So if we add the same configuration to stubdom as well, what would
> happen? I guess libxl will try to attach the PCI devices to both the
> stubdom and DomU?

I'm not sure if I understand you here. You mean adding configuration file for
stubdomain, the one managed by
libxl__get_domain_configuration/libxl__set_domain_configuration? In
theory it would work just fine, but in practice I fear all kind of
desynchronization bugs, like adding device to target domain's config,
but not stubdomain's one in some failure handling case. We'd have 4
things to care about:
 - attaching device to target domain
 - attaching device to stubdomain
 - saving device to target domain's config
 - saving device to stubdomain's config

Handling all the failure cases properly will become quite complex,
especially if we add async callbacks into the mix.
Since stubdomain config is deterministically build based on target
domian's config, I don't think adding such complexity is a good idea.

> > This bug prevented starting HVM with stubdomain and PCI passthrough
> > device attached.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Changes in v3:
> >  - skip libxl__dm_check_start too, as stubdomain is guaranteed to be
> >    running at this stage already
> >  - do not init d_config at all, as it is used only for json manipulation
> > ---
> >  tools/libxl/libxl_pci.c | 48 ++++++++++++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 1bde537..8d159cf 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -120,10 +120,14 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
> >      libxl_domain_config d_config;
> >      libxl_device_pci pcidev_saved;
> >      libxl__domain_userdata_lock *lock = NULL;
> > +    bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
> >  
> > -    libxl_domain_config_init(&d_config);
> > -    libxl_device_pci_init(&pcidev_saved);
> > -    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> > +    /* Stubdomain doesn't have own config. */
> > +    if (!is_stubdomain) {
> > +        libxl_domain_config_init(&d_config);
> > +        libxl_device_pci_init(&pcidev_saved);
> > +        libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> > +    }
> >  
> >      be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
> >                                                  LIBXL__DEVICE_KIND_PCI);
> > @@ -152,27 +156,33 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
> >      GCNEW(device);
> >      libxl__device_from_pcidev(gc, domid, pcidev, device);
> >  
> > -    lock = libxl__lock_domain_userdata(gc, domid);
> > -    if (!lock) {
> > -        rc = ERROR_LOCK_FAIL;
> > -        goto out;
> > -    }
> > +    /* Stubdomain config is derived from its target domain, it doesn't have
> > +       its own file */
> 
> Although comment style isn't specified in CODING_STYLE, I would like to
> fix this to 
> 
>     /*
>      * Stubdom ...
>      * ... own file.
>      */

Ok.

> > +    if (!is_stubdomain) {
> > +        lock = libxl__lock_domain_userdata(gc, domid);
> > +        if (!lock) {
> > +            rc = ERROR_LOCK_FAIL;
> > +            goto out;
> > +        }
> >  
> > -    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > -    if (rc) goto out;
> > +        rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > +        if (rc) goto out;
> >  
> > -    device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> > -                             &pcidev_saved);
> > +        device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> > +                                 &pcidev_saved);
> >  
> > -    rc = libxl__dm_check_start(gc, &d_config, domid);
> > -    if (rc) goto out;
> > +        rc = libxl__dm_check_start(gc, &d_config, domid);
> > +        if (rc) goto out;
> > +    }
> >  
> >      for (;;) {
> >          rc = libxl__xs_transaction_start(gc, &t);
> >          if (rc) goto out;
> >  
> > -        rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > -        if (rc) goto out;
> > +        if (lock) {
> > +            rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > +            if (rc) goto out;
> > +        }
> >  
> >          libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
> >  
> > @@ -184,8 +194,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
> >  out:
> >      libxl__xs_transaction_abort(gc, &t);
> >      if (lock) libxl__unlock_domain_userdata(lock);
> > -    libxl_device_pci_dispose(&pcidev_saved);
> > -    libxl_domain_config_dispose(&d_config);
> > +    if (!is_stubdomain) {
> > +        libxl_device_pci_dispose(&pcidev_saved);
> > +        libxl_domain_config_dispose(&d_config);
> > +    }
> >      return rc;
> >  }
> >  
> > -- 
> > git-series 0.9.1

-- 
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

[PATCH v3 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

From: Simon Gaiser <simon@invisiblethingslab.com>

Stubdomains need to be given sufficient privilege over the guest which it
provides emulation for in order for PCI passthrough to work correctly.
When a HVM domain try to enable MSI, QEMU in stubdomain calls
PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
part of xc_domain_update_msi_irq. Allow for that as part of
PHYSDEVOP_map_pirq.

This is not needed for PCI INTx, because IRQ in that case is known
beforehand and the stubdomain is given permissions over this IRQ by
libxl__device_pci_add (there's a do_pci_add against the stubdomain).

Based on https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <chanudete@ainfosec.com>.

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
 - extend commit message

With this patch, stubdomain will be able to create and map multiple irq
(DoS possibility?), as only target domain is validated in practice. Is
that ok? If not, what additional limits could be applied here?
In INTx case the problem doesn't apply, because toolstack grant access
to particular IRQ and no allocation happen on stubdomain request. But in
MSI case, it isn't that easy as IRQ number isn't known before (as
explained in the commit message).
---
 xen/arch/x86/irq.c     | 23 +++++++++++++++++++++++
 xen/arch/x86/physdev.c |  9 +++++++++
 2 files changed, 32 insertions(+)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 8b44d6c..67c67d4 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2674,6 +2674,21 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
         {
     case MAP_PIRQ_TYPE_MULTI_MSI:
             irq = create_irq(NUMA_NO_NODE);
+            if ( !(irq < nr_irqs_gsi || irq >= nr_irqs) &&
+                    current->domain->target == d )
+            {
+                ret = irq_permit_access(current->domain, irq);
+                if ( ret ) {
+                    dprintk(XENLOG_G_ERR,
+                            "dom%d: can't grant it's stubdom (%d) access to "
+                            "irq %d for msi: %d!\n",
+                            d->domain_id,
+                            current->domain->domain_id,
+                            irq,
+                            ret);
+                    return ret;
+                }
+            }
         }
 
         if ( irq < nr_irqs_gsi || irq >= nr_irqs )
@@ -2717,7 +2732,15 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
         case MAP_PIRQ_TYPE_MSI:
             if ( index == -1 )
         case MAP_PIRQ_TYPE_MULTI_MSI:
+            {
+                if ( current->domain->target == d &&
+                        irq_deny_access(current->domain, irq) )
+                    dprintk(XENLOG_G_ERR,
+                            "dom%d: can't revoke stubdom's access to irq %d!\n",
+                            d->domain_id,
+                            irq);
                 destroy_irq(irq);
+            }
             break;
         }
     }
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a3c158..de59e39 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -164,6 +164,15 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
 
     pcidevs_lock();
     spin_lock(&d->event_lock);
+    if ( current->domain->target == d)
+    {
+        int irq = domain_pirq_to_irq(d, pirq);
+        if ( irq <= 0 || irq_deny_access(current->domain, irq) )
+            dprintk(XENLOG_G_ERR,
+                    "dom%d: can't revoke stubdom's access to irq %d!\n",
+                    d->domain_id,
+                    irq);
+    }
     ret = unmap_domain_pirq(d, pirq);
     spin_unlock(&d->event_lock);
     pcidevs_unlock();
-- 
git-series 0.9.1

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

Re: [PATCH v3 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:15AM +0100, Marek Marczykowski-Górecki wrote:
> From: Simon Gaiser <simon@invisiblethingslab.com>
> 
> Stubdomains need to be given sufficient privilege over the guest which it
> provides emulation for in order for PCI passthrough to work correctly.
> When a HVM domain try to enable MSI, QEMU in stubdomain calls
> PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
> part of xc_domain_update_msi_irq. Allow for that as part of
> PHYSDEVOP_map_pirq.
> 
> This is not needed for PCI INTx, because IRQ in that case is known
> beforehand and the stubdomain is given permissions over this IRQ by
> libxl__device_pci_add (there's a do_pci_add against the stubdomain).
> 
> Based on https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <chanudete@ainfosec.com>.
> 
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v3:
>  - extend commit message
> 
> With this patch, stubdomain will be able to create and map multiple irq
> (DoS possibility?), as only target domain is validated in practice. Is
> that ok? If not, what additional limits could be applied here?
> In INTx case the problem doesn't apply, because toolstack grant access
> to particular IRQ and no allocation happen on stubdomain request. But in
> MSI case, it isn't that easy as IRQ number isn't known before (as
> explained in the commit message).
> ---
>  xen/arch/x86/irq.c     | 23 +++++++++++++++++++++++
>  xen/arch/x86/physdev.c |  9 +++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 8b44d6c..67c67d4 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2674,6 +2674,21 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
>          {
>      case MAP_PIRQ_TYPE_MULTI_MSI:
>              irq = create_irq(NUMA_NO_NODE);
> +            if ( !(irq < nr_irqs_gsi || irq >= nr_irqs) &&
> +                    current->domain->target == d )
> +            {
> +                ret = irq_permit_access(current->domain, irq);
> +                if ( ret ) {
> +                    dprintk(XENLOG_G_ERR,
> +                            "dom%d: can't grant it's stubdom (%d) access to "
> +                            "irq %d for msi: %d!\n",
> +                            d->domain_id,
> +                            current->domain->domain_id,
> +                            irq,
> +                            ret);
> +                    return ret;

Don't you need to deallocate the irq before returning?

Wei.

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

Re: [PATCH v3 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

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

On Mon, Jan 28, 2019 at 02:50:00PM +0000, Wei Liu wrote:
> On Sat, Jan 26, 2019 at 03:31:15AM +0100, Marek Marczykowski-Górecki wrote:
> > From: Simon Gaiser <simon@invisiblethingslab.com>
> > 
> > Stubdomains need to be given sufficient privilege over the guest which it
> > provides emulation for in order for PCI passthrough to work correctly.
> > When a HVM domain try to enable MSI, QEMU in stubdomain calls
> > PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
> > part of xc_domain_update_msi_irq. Allow for that as part of
> > PHYSDEVOP_map_pirq.
> > 
> > This is not needed for PCI INTx, because IRQ in that case is known
> > beforehand and the stubdomain is given permissions over this IRQ by
> > libxl__device_pci_add (there's a do_pci_add against the stubdomain).
> > 
> > Based on https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <chanudete@ainfosec.com>.
> > 
> > Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Changes in v3:
> >  - extend commit message
> > 
> > With this patch, stubdomain will be able to create and map multiple irq
> > (DoS possibility?), as only target domain is validated in practice. Is
> > that ok? If not, what additional limits could be applied here?
> > In INTx case the problem doesn't apply, because toolstack grant access
> > to particular IRQ and no allocation happen on stubdomain request. But in
> > MSI case, it isn't that easy as IRQ number isn't known before (as
> > explained in the commit message).
> > ---
> >  xen/arch/x86/irq.c     | 23 +++++++++++++++++++++++
> >  xen/arch/x86/physdev.c |  9 +++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > index 8b44d6c..67c67d4 100644
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -2674,6 +2674,21 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
> >          {
> >      case MAP_PIRQ_TYPE_MULTI_MSI:
> >              irq = create_irq(NUMA_NO_NODE);
> > +            if ( !(irq < nr_irqs_gsi || irq >= nr_irqs) &&
> > +                    current->domain->target == d )
> > +            {
> > +                ret = irq_permit_access(current->domain, irq);
> > +                if ( ret ) {
> > +                    dprintk(XENLOG_G_ERR,
> > +                            "dom%d: can't grant it's stubdom (%d) access to "
> > +                            "irq %d for msi: %d!\n",
> > +                            d->domain_id,
> > +                            current->domain->domain_id,
> > +                            irq,
> > +                            ret);
> > +                    return ret;
> 
> Don't you need to deallocate the irq before returning?

Yes, indeed.

-- 
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

[PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

[Marek Marczykowski-Górecki]

Allow device model running in stubdomain to enable/disable MSI(-X),
bypassing pciback. While pciback is still used to access config space
from within stubdomain, it refuse to write to
PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
is the right thing to do for PV domain (the main use case for pciback),
as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
those commands are not good for stubdomain use, as they configure MSI in
dom0's kernel too, which should not happen for HVM domain.

This new physdevop is allowed only for stubdomain controlling the domain
which own the device.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
 - new patch

This is rather RFC. Any suggestions for shorter name? Also, I'm not sure
if physdev_msi_msix_set_enable.flag is the best name/idea.

Should it be plugged into XSM? Any suggestions how exactly? New
function with XSM_DM_PRIV default action? Should it get target domain
only, or also machine_bdf?
---
 xen/arch/x86/msi.c           | 16 ++++++++++++++++
 xen/arch/x86/physdev.c       | 24 ++++++++++++++++++++++++
 xen/include/asm-x86/msi.h    |  1 +
 xen/include/public/physdev.h | 13 +++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index babc414..9ba934c 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
     return 0;
 }
 
+int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
+{
+    if ( !current->domain->target || pdev->domain != current->domain->target )
+        return -EPERM;
+
+    switch ( flag ) {
+        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI:
+            msi_set_enable(pdev, enable);
+            break;
+        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX:
+            msix_set_enable(pdev, enable);
+            break;
+    }
+    return 0;
+}
+
 void __init early_msi_init(void)
 {
     if ( use_msi < 0 )
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index de59e39..822846a 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -671,6 +671,30 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case PHYSDEVOP_msi_msix_set_enable: {
+        struct physdev_msi_msix_set_enable op;
+        struct pci_dev *pdev;
+
+        ret = -EFAULT;
+        if ( copy_from_guest(&op, arg, 1) )
+            break;
+
+        ret = -EINVAL;
+        if ( op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI &&
+                op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX )
+            break;
+
+        pcidevs_lock();
+        pdev = pci_get_pdev(op.pci.seg, op.pci.bus, op.pci.devfn);
+        if ( pdev )
+            ret = msi_msix_set_enable(pdev, op.flag, !!op.enable);
+        else
+            ret = -ENODEV;
+        pcidevs_unlock();
+        break;
+
+    }
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 10387dc..080bf24 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
 void ack_nonmaskable_msi_irq(struct irq_desc *);
 void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
 void set_msi_affinity(struct irq_desc *, const cpumask_t *);
+int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable);
 
 #endif /* __ASM_MSI_H */
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index b6faf83..fd797c6 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -344,6 +344,19 @@ struct physdev_dbgp_op {
 typedef struct physdev_dbgp_op physdev_dbgp_op_t;
 DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
 
+#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI  0
+#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX 1
+
+#define PHYSDEVOP_msi_msix_set_enable   32
+struct physdev_msi_msix_set_enable {
+    /* IN */
+    struct physdev_pci_device pci;
+    uint8_t flag;
+    uint8_t enable;
+};
+typedef struct physdev_msi_msix_set_enable physdev_msi_msix_set_enable_t;
+DEFINE_XEN_GUEST_HANDLE(physdev_msi_msix_set_enable_t);
+
 /*
  * Notify that some PIRQ-bound event channels have been unmasked.
  * ** This command is obsolete since interface version 0x00030202 and is **
-- 
git-series 0.9.1

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

Re: [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:16AM +0100, Marek Marczykowski-Górecki wrote:
> Allow device model running in stubdomain to enable/disable MSI(-X),
> bypassing pciback. While pciback is still used to access config space
> from within stubdomain, it refuse to write to
> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> is the right thing to do for PV domain (the main use case for pciback),
> as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> those commands are not good for stubdomain use, as they configure MSI in
> dom0's kernel too, which should not happen for HVM domain.
> 
> This new physdevop is allowed only for stubdomain controlling the domain
> which own the device.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v3:
>  - new patch
> 
> This is rather RFC. Any suggestions for shorter name? Also, I'm not sure
> if physdev_msi_msix_set_enable.flag is the best name/idea.

I'm bad at naming things, so I will refrain from commenting on this.

> 
> Should it be plugged into XSM? Any suggestions how exactly? New
> function with XSM_DM_PRIV default action? Should it get target domain
> only, or also machine_bdf?

It should be hooked into XSM -- other sub-ops already do that.

> ---
>  xen/arch/x86/msi.c           | 16 ++++++++++++++++
>  xen/arch/x86/physdev.c       | 24 ++++++++++++++++++++++++
>  xen/include/asm-x86/msi.h    |  1 +
>  xen/include/public/physdev.h | 13 +++++++++++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc414..9ba934c 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
> +{
> +    if ( !current->domain->target || pdev->domain != current->domain->target )
> +        return -EPERM;
> +
> +    switch ( flag ) {

{ should be on a new line.

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

Re: [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

[Roger Pau Monné]

On Sat, Jan 26, 2019 at 03:31:16AM +0100, Marek Marczykowski-Górecki wrote:
> Allow device model running in stubdomain to enable/disable MSI(-X),
> bypassing pciback. While pciback is still used to access config space
> from within stubdomain, it refuse to write to
> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> is the right thing to do for PV domain (the main use case for pciback),
> as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> those commands are not good for stubdomain use, as they configure MSI in
> dom0's kernel too, which should not happen for HVM domain.
> 
> This new physdevop is allowed only for stubdomain controlling the domain
> which own the device.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks!

> ---
> Changes in v3:
>  - new patch
> 
> This is rather RFC. Any suggestions for shorter name? Also, I'm not sure
> if physdev_msi_msix_set_enable.flag is the best name/idea.

I've made some comments below.

> Should it be plugged into XSM? Any suggestions how exactly? New
> function with XSM_DM_PRIV default action? Should it get target domain
> only, or also machine_bdf?

You should Cc the XSM maintainer I think, which I've done now.

> ---
>  xen/arch/x86/msi.c           | 16 ++++++++++++++++
>  xen/arch/x86/physdev.c       | 24 ++++++++++++++++++++++++
>  xen/include/asm-x86/msi.h    |  1 +
>  xen/include/public/physdev.h | 13 +++++++++++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc414..9ba934c 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
> +{
> +    if ( !current->domain->target || pdev->domain != current->domain->target )
> +        return -EPERM;
> +
> +    switch ( flag ) {
> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI:
> +            msi_set_enable(pdev, enable);
> +            break;

Please add a newline here.

> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX:
> +            msix_set_enable(pdev, enable);
> +            break;
> +    }
> +    return 0;
> +}
> +
>  void __init early_msi_init(void)
>  {
>      if ( use_msi < 0 )
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index de59e39..822846a 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -671,6 +671,30 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_msi_msix_set_enable: {
> +        struct physdev_msi_msix_set_enable op;
> +        struct pci_dev *pdev;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&op, arg, 1) )
> +            break;
> +
> +        ret = -EINVAL;
> +        if ( op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI &&
> +                op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX )

Align.

> +            break;
> +
> +        pcidevs_lock();
> +        pdev = pci_get_pdev(op.pci.seg, op.pci.bus, op.pci.devfn);
> +        if ( pdev )
> +            ret = msi_msix_set_enable(pdev, op.flag, !!op.enable);
> +        else
> +            ret = -ENODEV;
> +        pcidevs_unlock();
> +        break;
> +
> +    }
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index 10387dc..080bf24 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
>  void ack_nonmaskable_msi_irq(struct irq_desc *);
>  void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
>  void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable);
>  
>  #endif /* __ASM_MSI_H */
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index b6faf83..fd797c6 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -344,6 +344,19 @@ struct physdev_dbgp_op {
>  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
>  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>  
> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI  0
> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX 1
> +
> +#define PHYSDEVOP_msi_msix_set_enable   32

There's no need for the 'msi_msix' name, there are already other
hypercalls that deal with both msi and msix and just have msi in the
name: PHYSDEVOP_msi_set_enable.

> +struct physdev_msi_msix_set_enable {
> +    /* IN */
> +    struct physdev_pci_device pci;
> +    uint8_t flag;

But this is not really a flags field, I would rather rename this
to 'mode' maybe.

> +    uint8_t enable;
> +};
> +typedef struct physdev_msi_msix_set_enable physdev_msi_msix_set_enable_t;
> +DEFINE_XEN_GUEST_HANDLE(physdev_msi_msix_set_enable_t);

I think you need to add the new hypercall to include/xlat.lst, AFAICT
it requires no translation, so you should add it as '?'.

Thanks, Roger.

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

Re: [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

[Jan Beulich]

>>> On 30.01.19 at 14:51, <roger.pau@citrix.com> wrote:
> On Sat, Jan 26, 2019 at 03:31:16AM +0100, Marek Marczykowski-Górecki wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
>> +{
>> +    if ( !current->domain->target || pdev->domain != current->domain->target )
>> +        return -EPERM;
>> +
>> +    switch ( flag ) {
>> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI:
>> +            msi_set_enable(pdev, enable);
>> +            break;
> 
> Please add a newline here.

And also please correct indentation. The misplaced { was
already pointed out iirc.

>> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX:
>> +            msix_set_enable(pdev, enable);
>> +            break;
>> +    }
>> +    return 0;

There's another blank line missing above here.

>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -344,6 +344,19 @@ struct physdev_dbgp_op {
>>  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>>  
>> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI  0
>> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX 1
>> +
>> +#define PHYSDEVOP_msi_msix_set_enable   32
> 
> There's no need for the 'msi_msix' name, there are already other
> hypercalls that deal with both msi and msix and just have msi in the
> name: PHYSDEVOP_msi_set_enable.

And FAOD the same then also for the other two defines,
or alternatively

#define PHYSDEVOP_MSI_SET_ENABLE  0
#define PHYSDEVOP_MSIX_SET_ENABLE 1

>> +struct physdev_msi_msix_set_enable {
>> +    /* IN */
>> +    struct physdev_pci_device pci;
>> +    uint8_t flag;
> 
> But this is not really a flags field, I would rather rename this
> to 'mode' maybe.
> 
>> +    uint8_t enable;
>> +};
>> +typedef struct physdev_msi_msix_set_enable physdev_msi_msix_set_enable_t;
>> +DEFINE_XEN_GUEST_HANDLE(physdev_msi_msix_set_enable_t);
> 
> I think you need to add the new hypercall to include/xlat.lst, AFAICT
> it requires no translation, so you should add it as '?'.

Plus add invocation of the resulting macro.

Jan


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

Re: [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

[Daniel De Graaf]

On 1/30/19 8:51 AM, Roger Pau Monné wrote:
> On Sat, Jan 26, 2019 at 03:31:16AM +0100, Marek Marczykowski-Górecki wrote:
>> Allow device model running in stubdomain to enable/disable MSI(-X),
>> bypassing pciback. While pciback is still used to access config space
>> from within stubdomain, it refuse to write to
>> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
>> is the right thing to do for PV domain (the main use case for pciback),
>> as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
>> those commands are not good for stubdomain use, as they configure MSI in
>> dom0's kernel too, which should not happen for HVM domain.
>>
>> This new physdevop is allowed only for stubdomain controlling the domain
>> which own the device.
>>
>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Thanks!
> 
>> ---
>> Changes in v3:
>>   - new patch
>>
>> This is rather RFC. Any suggestions for shorter name? Also, I'm not sure
>> if physdev_msi_msix_set_enable.flag is the best name/idea.
> 
> I've made some comments below.
> 
>> Should it be plugged into XSM? Any suggestions how exactly? New
>> function with XSM_DM_PRIV default action? Should it get target domain
>> only, or also machine_bdf?
> 
> You should Cc the XSM maintainer I think, which I've done now.

Yes, I think you need to pass both the domain and machine_bdf; the hook
sounds like it should be similar to the existing map_domain_irq or
assign_device hooks: check that the device model has rights to do this
operation on the device, and also check that the device model has rights
to modify devices assigned to the device's owner (this check is why the
domain is needed).

Alternatively, you can hard-code the requirement to be the target domain,
but I think this prevents using the same hooks from a dom0 device model.
If that's already the case, this is less of an issue.

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

[PATCH v3 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_msix_set_enable

[Marek Marczykowski-Górecki]

Add libxc wrapper for PHYSDEVOP_msi_msix_set_enable introduced in
previous commit.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
 - new patch
---
 tools/libxc/include/xenctrl.h |  7 +++++++
 tools/libxc/xc_physdev.c      | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 31cdda7..2b86f4c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1639,6 +1639,13 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
                           uint32_t domid,
                           int pirq);
 
+int xc_physdev_msi_msix_set_enable(xc_interface *xch,
+                                   int seg,
+                                   int bus,
+                                   int devfn,
+                                   int flag,
+                                   int enable);
+
 /*
  *  LOGGING AND ERROR REPORTING
  */
diff --git a/tools/libxc/xc_physdev.c b/tools/libxc/xc_physdev.c
index 460a8e7..3530cb8 100644
--- a/tools/libxc/xc_physdev.c
+++ b/tools/libxc/xc_physdev.c
@@ -111,3 +111,24 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
     return rc;
 }
 
+int xc_physdev_msi_msix_set_enable(xc_interface *xch,
+                                   int seg,
+                                   int bus,
+                                   int devfn,
+                                   int flag,
+                                   int enable)
+{
+    int rc;
+    struct physdev_msi_msix_set_enable op;
+
+    memset(&op, 0, sizeof(struct physdev_msi_msix_set_enable));
+    op.pci.seg = seg;
+    op.pci.bus = bus;
+    op.pci.devfn = devfn;
+    op.flag = flag;
+    op.enable = enable;
+
+    rc = do_physdev_op(xch, PHYSDEVOP_msi_msix_set_enable, &op, sizeof(op));
+
+    return rc;
+}
-- 
git-series 0.9.1

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

Re: [PATCH v3 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_msix_set_enable

[Wei Liu]

On Sat, Jan 26, 2019 at 03:31:17AM +0100, Marek Marczykowski-Górecki wrote:
> Add libxc wrapper for PHYSDEVOP_msi_msix_set_enable introduced in
> previous commit.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Assuming the addition of physdev ops is accepted:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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