[PATCH v2 0/4] Fix PCI passthrough for HVM with stubdomain

[PATCH v2 0/4] Fix PCI passthrough for HVM with stubdomain

[Marek Marczykowski-Górecki]

This is yet another attempt to upstream a set of PCI passthrough related fixed
carried in Qubes OS packages for a long time already.
Some of them were already discussed previously, see another thread with the
same subject back in October 2016.

As for "xen/x86: Allow stubdom access to irq created for msi" patch, this is
only one part of fixing MSI with QEMU in stubdomain. The other part is allowing
stubdomain to actually enable MSI in PCI config space.  QEMU does that through
pcifront/back connected to the stubdomain (see
hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
write to that register.
Easy, less safe solution: enable permissive mode for the device.
Safer solution - enable access to this register for stubdomain only:
 - pciback patch: https://github.com/QubesOS/qubes-linux-kernel/blob/master/0015-xen-pciback-add-attribute-to-allow-MSI-enable-flag-w.patch
 - libxl patch: https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.8/patch-stubdom-allow-msi-enable.patch

The whole story:
https://www.qubes-os.org/news/2017/10/18/msi-support/

Any other ideas? Which one is preferred upstream?

Changes in v2:
 - new "xen/x86: Allow stubdom access to irq created for msi" patch
 - applied review comments from v1

Marek Marczykowski-Górecki (3):
  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

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

 tools/libxl/libxl_pci.c | 37 ++++++++++++++++++++++++++-----------
 xen/arch/x86/irq.c      | 23 +++++++++++++++++++++++
 xen/arch/x86/physdev.c  |  9 +++++++++
 3 files changed, 58 insertions(+), 11 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 v2 1/4] 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 is used only in PV domains.
When HVM domain has device model in stubdomain, attaching xen-pciback to
the target domain itself is not only useless, but also may prevent
attaching xen-pciback to the stubdomain, effectively breaking PCI
passthrough.

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
---
 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 v2 1/4] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Roger Pau Monné]

On Tue, Jan 15, 2019 at 04:36:28PM +0100, Marek Marczykowski-Górecki wrote:
> HVM domains use IOMMU and device model assistance for communicating with
> PCI devices, xen-pcifront/pciback is used only in PV domains.

You still need pciback in order to reset the device when it's
deassigned from the guest, so it's functionality is not only used by
PV guests.

> When HVM domain has device model in stubdomain, attaching xen-pciback to
> the target domain itself is not only useless, but also may prevent
> attaching xen-pciback to the stubdomain, effectively breaking PCI
> passthrough.

Right. When doing passthrough with a stubdomain you want the target
domain to have the memory and IO regions mapped, and the stubdomain to
handle the rest.

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

This change seems to assume that both libxl_domain_config for the
target and the stubdomain will have the assigned pci devices in the
pcidevs field. Yet I cannot see where the stubdomain
libxl_domain_config will get the pci devices from the target domain
assigned, I've looked in libxl__spawn_stub_dm but there doesn't seem
to be any copy from the target to the stubdom of the list of pci
devices. I guess I'm missing something.

Thanks, Roger.

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

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

[Marek Marczykowski-Górecki]

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

On Wed, Jan 16, 2019 at 05:47:19PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 15, 2019 at 04:36:28PM +0100, Marek Marczykowski-Górecki wrote:
> > HVM domains use IOMMU and device model assistance for communicating with
> > PCI devices, xen-pcifront/pciback is used only in PV domains.
> 
> You still need pciback in order to reset the device when it's
> deassigned from the guest, so it's functionality is not only used by
> PV guests.

Right, I'll update the commit message to match v2 code.

> > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > the target domain itself is not only useless, but also may prevent
> > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > passthrough.
> 
> Right. When doing passthrough with a stubdomain you want the target
> domain to have the memory and IO regions mapped, and the stubdomain to
> handle the rest.
> 
> > 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
> > ---
> >  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))
> 
> This change seems to assume that both libxl_domain_config for the
> target and the stubdomain will have the assigned pci devices in the
> pcidevs field. 

Not really. libxl__device_pci_add() calls do_pci_add() for both stubdomain
(if applicable) and target domain.

> Yet I cannot see where the stubdomain
> libxl_domain_config will get the pci devices from the target domain
> assigned, I've looked in libxl__spawn_stub_dm but there doesn't seem
> to be any copy from the target to the stubdom of the list of pci
> devices. I guess I'm missing something.
> 
> Thanks, Roger.

-- 
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 v2 1/4] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 06:00:33PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 05:47:19PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 15, 2019 at 04:36:28PM +0100, Marek Marczykowski-Górecki wrote:
> > > HVM domains use IOMMU and device model assistance for communicating with
> > > PCI devices, xen-pcifront/pciback is used only in PV domains.
> > 
> > You still need pciback in order to reset the device when it's
> > deassigned from the guest, so it's functionality is not only used by
> > PV guests.
> 
> Right, I'll update the commit message to match v2 code.
> 
> > > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > > the target domain itself is not only useless, but also may prevent
> > > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > > passthrough.
> > 
> > Right. When doing passthrough with a stubdomain you want the target
> > domain to have the memory and IO regions mapped, and the stubdomain to
> > handle the rest.
> > 
> > > 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
> > > ---
> > >  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))
> > 
> > This change seems to assume that both libxl_domain_config for the
> > target and the stubdomain will have the assigned pci devices in the
> > pcidevs field. 
> 
> Not really. libxl__device_pci_add() calls do_pci_add() for both stubdomain
> (if applicable) and target domain.

OK. From an architectural PoV I think it would make more sense to copy
the list of pci devices to the stubdom config in libxl__spawn_stub_dm,
but I'm not that familiar with pci handling in libxl, so there might
be a reason why things are done like this currently.

The change LGTM, albeit I found the pci handling code quite hard to
follow. I'm also not sure whether certain parts of the code are
correct, for example the PCI INTx seems to be mapped to both the
stubdomain and the target domain, when it's only the target domain the
one that actually uses it.

Thanks, Roger.

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

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

[Marek Marczykowski-Górecki]

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

On Thu, Jan 17, 2019 at 10:21:34AM +0100, Roger Pau Monné wrote:
> OK. From an architectural PoV I think it would make more sense to copy
> the list of pci devices to the stubdom config in libxl__spawn_stub_dm,
> but I'm not that familiar with pci handling in libxl, so there might
> be a reason why things are done like this currently.

libxl would refuse to attach the same device to two domains and also
will perform some operations twice (as for example device reset). As you
can see even right now some things needs to be disabled for stubdomain
or target domain, because of this. If device would be included in the
config for both target domain and its stubdomain, the code would need
some more exceptions, which I think would be even worse.

Other thing is the current code attach PCI devices when device-model is
already running (regardless of its version or using stubdomain). I guess
qemu doesn't setup those devices otherwise (there is nothing on qemu
command line about it and none of libxl part generate such options). I'm
not really sure if that couldn't be done differently, but I'm kind of
afraid changing to much in PCI passthrough related code...

> The change LGTM, albeit I found the pci handling code quite hard to
> follow. I'm also not sure whether certain parts of the code are
> correct, for example the PCI INTx seems to be mapped to both the
> stubdomain and the target domain, when it's only the target domain the
> one that actually uses it.

That's true. Generally, I think stubdomain needs only config space
access for its own, other things should be done for the target domain.
This include mapping interrupts of any kind. Stubdomain is given
permission to them only to be able to map them to the target domain.

This also may be the reason why PCI devices are not included in
stubdomain's config, but only do_pci_add() is called. It may be that in
the past some more parts of the device setup was skipped this way.

-- 
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 v2 2/4] 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 v2 2/4] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Roger Pau Monné]

On Tue, Jan 15, 2019 at 04:36:29PM +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>
> ---
> 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;

I think it would be better to use the async libxl functionality here,
see libxl__xswait_start. I will leave for the toolstack maintainers to
decide. Apart from that the change seems correct.

Thanks, Roger.

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

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

[Marek Marczykowski-Górecki]

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

On Thu, Jan 17, 2019 at 11:29:59AM +0100, Roger Pau Monné wrote:
> On Tue, Jan 15, 2019 at 04:36:29PM +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>
> > ---
> > 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;
> 
> I think it would be better to use the async libxl functionality here,
> see libxl__xswait_start. I will leave for the toolstack maintainers to
> decide. Apart from that the change seems correct.

libxl__device_pci_add() is not async-aware right now and it looks like
converting it is quite a bit of work. I'd leave it out of scope for this
patch series...

-- 
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 v2 3/4] 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>
---
 tools/libxl/libxl_pci.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 1bde537..e974f55 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -152,14 +152,18 @@ 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 (!libxl_is_stubdom(CTX, domid, NULL)) {
+        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);
@@ -171,8 +175,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
         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));
 
-- 
git-series 0.9.1

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

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

[Roger Pau Monné]

On Tue, Jan 15, 2019 at 04:36:30PM +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.
> 
> This bug prevented starting HVM with stubdomain and PCI passthrough
> device attached.

I would add a note that the pci device is added to the target domain,
which do have a valid config file.

> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_pci.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 1bde537..e974f55 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -152,14 +152,18 @@ 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 (!libxl_is_stubdom(CTX, domid, NULL)) {
> +        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);

I would put the device_add_domain_config inside of the non-stubdom
case. You could also move the call to libxl_domain_config_init inside
of the non-stubdom case, since d_config is only relevant when there's
no stubdom.

Thanks, Roger.

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

[PATCH v2 4/4] 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.

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>
---
This is only one part of fixing MSI with QEMU in stubdomain. The other
part is allowing stubdomain to actually enable MSI in PCI config space.
QEMU does that through pcifront/back connected to the stubdomain (see
hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
write to that register.
Easy, less safe solution: enable permissive mode for the device.
Safer solution - enable access to this register for stubdomain only
(pciback patch that add such flag + libxl patch to set it for relevant
 devices)
The whole story:
https://www.qubes-os.org/news/2017/10/18/msi-support/

Any other ideas? Which one is preferred upstream?
---
 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..123ca69 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 -EINVAL;
+                }
+            }
         }
 
         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 v2 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Tue, Jan 15, 2019 at 04:36:31PM +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.

I see, that's not a problem AFAICT for PCI INTx because the 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>
> ---
> This is only one part of fixing MSI with QEMU in stubdomain. The other
> part is allowing stubdomain to actually enable MSI in PCI config space.
> QEMU does that through pcifront/back connected to the stubdomain (see
> hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> write to that register.
> Easy, less safe solution: enable permissive mode for the device.
> Safer solution - enable access to this register for stubdomain only
> (pciback patch that add such flag + libxl patch to set it for relevant
>  devices)
> The whole story:
> https://www.qubes-os.org/news/2017/10/18/msi-support/
> 
> Any other ideas? Which one is preferred upstream?

IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
receive the PCI config space write to enable MSI, and since this
stub-QEMU runs in PV mode I think it should use the PV way to enable
MSI, ie: the same that Linux pcifront uses to enable MSI for
passed-through devices.

Is this something that sounds sensible?

> ---
>  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..123ca69 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) &&

This check is already performed below, maybe you could re-arrange the
code as:

case MAP_PIRQ_TYPE_MULTI_MSI:
        irq = create_irq(NUMA_NO_NODE);
    }

    if ( irq < nr_irqs_gsi || irq >= nr_irqs )
    {
        dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
                d->domain_id);
        return -EINVAL;
    }
    if ( current->domain->target == d )
        ...

But I wonder whether it would be better to place the irq_permit_access
in map_domain_pirq, together with the existing irq_permit_access that
grant the target domain permissions over the irq.

> +                 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 -EINVAL;

You should return ret here IMO, so that the error is propagated to the
caller (likely ENOMEM since irq_permit_access is just a wrapper around
rangeset_add).

> +                }
> +            }
>          }
>  
>          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) )

Same here, I think it would be more natural to place the
irq_deny_access in unmap_domain_pirq, together with the existing
irq_deny_access that revokes the permissions of the target domain.

Thanks, Roger.

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

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

[Marek Marczykowski-Górecki]

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

On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> On Tue, Jan 15, 2019 at 04:36:31PM +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.
> 
> I see, that's not a problem AFAICT for PCI INTx because the 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).

Exactly.

> > 
> > 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>
> > ---
> > This is only one part of fixing MSI with QEMU in stubdomain. The other
> > part is allowing stubdomain to actually enable MSI in PCI config space.
> > QEMU does that through pcifront/back connected to the stubdomain (see
> > hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> > write to that register.
> > Easy, less safe solution: enable permissive mode for the device.
> > Safer solution - enable access to this register for stubdomain only
> > (pciback patch that add such flag + libxl patch to set it for relevant
> >  devices)
> > The whole story:
> > https://www.qubes-os.org/news/2017/10/18/msi-support/
> > 
> > Any other ideas? Which one is preferred upstream?
> 
> IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
> receive the PCI config space write to enable MSI, and since this
> stub-QEMU runs in PV mode I think it should use the PV way to enable
> MSI, ie: the same that Linux pcifront uses to enable MSI for
> passed-through devices.
> 
> Is this something that sounds sensible?

We've considered this option too. Let me quote Simon on that (from the
link above):

    The enable command that pcifront sends is intended for the normal PV use
    case where the device is passed to the VM itself (via pcifront) rather
    than to the stub domain target. While the command is called enable_msi,
    pciback does much more than simply setting the enable flag. It also
    configures IRQ handling in the dom0 kernel, adapts the MSI masking, and
    more. This makes sense in the PV case, but in the HVM case, the MSI
    configuration is done by QEMU, so this most likely won’t work correctly.

> > ---
> >  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..123ca69 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) &&
> 
> This check is already performed below, maybe you could re-arrange the
> code as:
> 
> case MAP_PIRQ_TYPE_MULTI_MSI:
>         irq = create_irq(NUMA_NO_NODE);
>     }
> 
>     if ( irq < nr_irqs_gsi || irq >= nr_irqs )
>     {
>         dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
>                 d->domain_id);
>         return -EINVAL;
>     }
>     if ( current->domain->target == d )
>         ...
> 
> But I wonder whether it would be better to place the irq_permit_access
> in map_domain_pirq, together with the existing irq_permit_access that
> grant the target domain permissions over the irq.

That may be a good idea. Let me try that in v3. But I'll wait for a
feedback on libxl patches first.

> > +                 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 -EINVAL;
> 
> You should return ret here IMO, so that the error is propagated to the
> caller (likely ENOMEM since irq_permit_access is just a wrapper around
> rangeset_add).

Ok.

> > +                }
> > +            }
> >          }
> >  
> >          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) )
> 
> Same here, I think it would be more natural to place the
> irq_deny_access in unmap_domain_pirq, together with the existing
> irq_deny_access that revokes the permissions of the target domain.

Ok.

-- 
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 v2 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 11:52:18AM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 15, 2019 at 04:36:31PM +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.
> > 
> > I see, that's not a problem AFAICT for PCI INTx because the 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).
> 
> Exactly.

I would maybe consider adding something like this to the commit
message, so it's clear why PCI INTx works but not MSI interrupts.

> 
> > > 
> > > 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>
> > > ---
> > > This is only one part of fixing MSI with QEMU in stubdomain. The other
> > > part is allowing stubdomain to actually enable MSI in PCI config space.
> > > QEMU does that through pcifront/back connected to the stubdomain (see
> > > hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> > > write to that register.
> > > Easy, less safe solution: enable permissive mode for the device.
> > > Safer solution - enable access to this register for stubdomain only
> > > (pciback patch that add such flag + libxl patch to set it for relevant
> > >  devices)
> > > The whole story:
> > > https://www.qubes-os.org/news/2017/10/18/msi-support/
> > > 
> > > Any other ideas? Which one is preferred upstream?
> > 
> > IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
> > receive the PCI config space write to enable MSI, and since this
> > stub-QEMU runs in PV mode I think it should use the PV way to enable
> > MSI, ie: the same that Linux pcifront uses to enable MSI for
> > passed-through devices.
> > 
> > Is this something that sounds sensible?
> 
> We've considered this option too. Let me quote Simon on that (from the
> link above):
> 
>     The enable command that pcifront sends is intended for the normal PV use
>     case where the device is passed to the VM itself (via pcifront) rather
>     than to the stub domain target. While the command is called enable_msi,
>     pciback does much more than simply setting the enable flag. It also
>     configures IRQ handling in the dom0 kernel, adapts the MSI masking, and
>     more. This makes sense in the PV case, but in the HVM case, the MSI
>     configuration is done by QEMU, so this most likely won’t work correctly.

Oh great, that's unfortunate. Both pciback functions end up calling
into msi_capability_init in the Linux kernel, which does indeed more
than just toggling the PCI config space enable bit.

OTOH adding a bypass to pciback so the stubdom is able to write to the
PCI register in order to toggle the enable bit seems quite clumsy. Not
to mention that you would be required to update Dom0 kernel in order to
fix the issue.

Do you think it makes sense to add a domctl to enable/disable MSI(X)?

This way the bug could be fixed by just updating Xen (and the
stubdomain).

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 16.01.19 at 13:20, <roger.pau@citrix.com> wrote:
> Do you think it makes sense to add a domctl to enable/disable MSI(X)?

A domctl looks odd for an operation like this; I'd rather consider
adding a physdevop if a new (sub)hypercall is needed here (of
which I'm not yet convinced; I have yet to look at the patch).

Jan



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

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

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 05:57:04AM -0700, Jan Beulich wrote:
> >>> On 16.01.19 at 13:20, <roger.pau@citrix.com> wrote:
> > Do you think it makes sense to add a domctl to enable/disable MSI(X)?
> 
> A domctl looks odd for an operation like this; I'd rather consider
> adding a physdevop if a new (sub)hypercall is needed here (of
> which I'm not yet convinced; I have yet to look at the patch).

I suggested a domctl because this operation would be performed by
control domains, not by unprivileged guests themselves, much like
XEN_DOMCTL_{un}bind_pt_irq. Also it won't be a stable interface, and
we could get rid of it in the future if we change the way passthrough
works (ie: by using vpci for guests also).

But I see your point about making it a physdevop, that would seem like
the best fit context wise.

Thanks, Roger.

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

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

[Marek Marczykowski-Górecki]

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

On Wed, Jan 16, 2019 at 01:20:04PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 16, 2019 at 11:52:18AM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > > On Tue, Jan 15, 2019 at 04:36:31PM +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.
> > > 
> > > I see, that's not a problem AFAICT for PCI INTx because the 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).
> > 
> > Exactly.
> 
> I would maybe consider adding something like this to the commit
> message, so it's clear why PCI INTx works but not MSI interrupts.
> 
> > 
> > > > 
> > > > 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>
> > > > ---
> > > > This is only one part of fixing MSI with QEMU in stubdomain. The other
> > > > part is allowing stubdomain to actually enable MSI in PCI config space.
> > > > QEMU does that through pcifront/back connected to the stubdomain (see
> > > > hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> > > > write to that register.
> > > > Easy, less safe solution: enable permissive mode for the device.
> > > > Safer solution - enable access to this register for stubdomain only
> > > > (pciback patch that add such flag + libxl patch to set it for relevant
> > > >  devices)
> > > > The whole story:
> > > > https://www.qubes-os.org/news/2017/10/18/msi-support/
> > > > 
> > > > Any other ideas? Which one is preferred upstream?
> > > 
> > > IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
> > > receive the PCI config space write to enable MSI, and since this
> > > stub-QEMU runs in PV mode I think it should use the PV way to enable
> > > MSI, ie: the same that Linux pcifront uses to enable MSI for
> > > passed-through devices.
> > > 
> > > Is this something that sounds sensible?
> > 
> > We've considered this option too. Let me quote Simon on that (from the
> > link above):
> > 
> >     The enable command that pcifront sends is intended for the normal PV use
> >     case where the device is passed to the VM itself (via pcifront) rather
> >     than to the stub domain target. While the command is called enable_msi,
> >     pciback does much more than simply setting the enable flag. It also
> >     configures IRQ handling in the dom0 kernel, adapts the MSI masking, and
> >     more. This makes sense in the PV case, but in the HVM case, the MSI
> >     configuration is done by QEMU, so this most likely won’t work correctly.
> 
> Oh great, that's unfortunate. Both pciback functions end up calling
> into msi_capability_init in the Linux kernel, which does indeed more
> than just toggling the PCI config space enable bit.
> 
> OTOH adding a bypass to pciback so the stubdom is able to write to the
> PCI register in order to toggle the enable bit seems quite clumsy. Not
> to mention that you would be required to update Dom0 kernel in order to
> fix the issue.
> 
> Do you think it makes sense to add a domctl to enable/disable MSI(X)?
> 
> This way the bug could be fixed by just updating Xen (and the
> stubdomain).

Indeed in case of stubdomain, that would make sens, as other PCI passthrough
related operations already bypass pcifront/back anyway.

And I agree with Jan, that physdevop makes more sense, if going this way.

-- 
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 v2 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Wed, Jan 16, 2019 at 02:49:14PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 01:20:04PM +0100, Roger Pau Monné wrote:
> > On Wed, Jan 16, 2019 at 11:52:18AM +0100, Marek Marczykowski-Górecki wrote:
> > > On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > > > On Tue, Jan 15, 2019 at 04:36:31PM +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.
> > > > 
> > > > I see, that's not a problem AFAICT for PCI INTx because the 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).
> > > 
> > > Exactly.
> > 
> > I would maybe consider adding something like this to the commit
> > message, so it's clear why PCI INTx works but not MSI interrupts.
> > 
> > > 
> > > > > 
> > > > > 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>
> > > > > ---
> > > > > This is only one part of fixing MSI with QEMU in stubdomain. The other
> > > > > part is allowing stubdomain to actually enable MSI in PCI config space.
> > > > > QEMU does that through pcifront/back connected to the stubdomain (see
> > > > > hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> > > > > write to that register.
> > > > > Easy, less safe solution: enable permissive mode for the device.
> > > > > Safer solution - enable access to this register for stubdomain only
> > > > > (pciback patch that add such flag + libxl patch to set it for relevant
> > > > >  devices)
> > > > > The whole story:
> > > > > https://www.qubes-os.org/news/2017/10/18/msi-support/
> > > > > 
> > > > > Any other ideas? Which one is preferred upstream?
> > > > 
> > > > IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
> > > > receive the PCI config space write to enable MSI, and since this
> > > > stub-QEMU runs in PV mode I think it should use the PV way to enable
> > > > MSI, ie: the same that Linux pcifront uses to enable MSI for
> > > > passed-through devices.
> > > > 
> > > > Is this something that sounds sensible?
> > > 
> > > We've considered this option too. Let me quote Simon on that (from the
> > > link above):
> > > 
> > >     The enable command that pcifront sends is intended for the normal PV use
> > >     case where the device is passed to the VM itself (via pcifront) rather
> > >     than to the stub domain target. While the command is called enable_msi,
> > >     pciback does much more than simply setting the enable flag. It also
> > >     configures IRQ handling in the dom0 kernel, adapts the MSI masking, and
> > >     more. This makes sense in the PV case, but in the HVM case, the MSI
> > >     configuration is done by QEMU, so this most likely won’t work correctly.
> > 
> > Oh great, that's unfortunate. Both pciback functions end up calling
> > into msi_capability_init in the Linux kernel, which does indeed more
> > than just toggling the PCI config space enable bit.
> > 
> > OTOH adding a bypass to pciback so the stubdom is able to write to the
> > PCI register in order to toggle the enable bit seems quite clumsy. Not
> > to mention that you would be required to update Dom0 kernel in order to
> > fix the issue.
> > 
> > Do you think it makes sense to add a domctl to enable/disable MSI(X)?
> > 
> > This way the bug could be fixed by just updating Xen (and the
> > stubdomain).
> 
> Indeed in case of stubdomain, that would make sens, as other PCI passthrough
> related operations already bypass pcifront/back anyway.
> 
> And I agree with Jan, that physdevop makes more sense, if going this way.

While not against using physdevop if we agree that a new hypercall is
the way to go, I would prefer a domctl because this hypercall would
only be used by toolstack components, and thus doesn't need to be
added to the public stable ABI available to all guests, even if the
functionality is actually limited to stubdomains.

Anyway, not a big deal, and if others think physdevop is more suitable
I'm OK with it.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 17.01.19 at 09:57, <roger.pau@citrix.com> wrote:
> While not against using physdevop if we agree that a new hypercall is
> the way to go, I would prefer a domctl because this hypercall would
> only be used by toolstack components, and thus doesn't need to be
> added to the public stable ABI available to all guests, even if the
> functionality is actually limited to stubdomains.

But a new sub-op doesn't need to be part of the stable ABI.
See how e.g. various of the memory sub-ops are restricted to
be used by the tool stack, and hence not required to remain
unchanged.

Jan



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

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

[Roger Pau Monné]

On Thu, Jan 17, 2019 at 04:52:42AM -0700, Jan Beulich wrote:
> >>> On 17.01.19 at 09:57, <roger.pau@citrix.com> wrote:
> > While not against using physdevop if we agree that a new hypercall is
> > the way to go, I would prefer a domctl because this hypercall would
> > only be used by toolstack components, and thus doesn't need to be
> > added to the public stable ABI available to all guests, even if the
> > functionality is actually limited to stubdomains.
> 
> But a new sub-op doesn't need to be part of the stable ABI.
> See how e.g. various of the memory sub-ops are restricted to
> be used by the tool stack, and hence not required to remain
> unchanged.

Oh, then I'm all in for a physdevop limited to stubdomain only usage.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 17.01.19 at 12:56, <roger.pau@citrix.com> wrote:
> On Thu, Jan 17, 2019 at 04:52:42AM -0700, Jan Beulich wrote:
>> >>> On 17.01.19 at 09:57, <roger.pau@citrix.com> wrote:
>> > While not against using physdevop if we agree that a new hypercall is
>> > the way to go, I would prefer a domctl because this hypercall would
>> > only be used by toolstack components, and thus doesn't need to be
>> > added to the public stable ABI available to all guests, even if the
>> > functionality is actually limited to stubdomains.
>> 
>> But a new sub-op doesn't need to be part of the stable ABI.
>> See how e.g. various of the memory sub-ops are restricted to
>> be used by the tool stack, and hence not required to remain
>> unchanged.
> 
> Oh, then I'm all in for a physdevop limited to stubdomain only usage.

Hmm, stubdomain is different: How would you limit this in the
header? Also stub domains are allowed to rely on a stable
interface, so I'm afraid a domctl is out of scope here anyway.
It is bad enough that there are four domctl-s violating this
rule (see xsm/dummy.h:xsm_domctl()).

Jan



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

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

[Roger Pau Monné]

On Thu, Jan 17, 2019 at 05:20:18AM -0700, Jan Beulich wrote:
> >>> On 17.01.19 at 12:56, <roger.pau@citrix.com> wrote:
> > On Thu, Jan 17, 2019 at 04:52:42AM -0700, Jan Beulich wrote:
> >> >>> On 17.01.19 at 09:57, <roger.pau@citrix.com> wrote:
> >> > While not against using physdevop if we agree that a new hypercall is
> >> > the way to go, I would prefer a domctl because this hypercall would
> >> > only be used by toolstack components, and thus doesn't need to be
> >> > added to the public stable ABI available to all guests, even if the
> >> > functionality is actually limited to stubdomains.
> >> 
> >> But a new sub-op doesn't need to be part of the stable ABI.
> >> See how e.g. various of the memory sub-ops are restricted to
> >> be used by the tool stack, and hence not required to remain
> >> unchanged.
> > 
> > Oh, then I'm all in for a physdevop limited to stubdomain only usage.
> 
> Hmm, stubdomain is different: How would you limit this in the
> header?

Oh, I wasn't meaning to limit this in the header, but in the
implementation. Ie: by returning an error when called from
non-stubdomains.

I don't see a reason to allow non-stubdomains to make use of this new
hypercall if it's not required, that would just expand the attack
surface for no good reason IMO.

> Also stub domains are allowed to rely on a stable
> interface, so I'm afraid a domctl is out of scope here anyway.
> It is bad enough that there are four domctl-s violating this
> rule (see xsm/dummy.h:xsm_domctl()).

OK, so then the fact that stubdomains (QEMU) use the non-stable domctl
is not intended, then it's fine to make it a physdevop.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 17.01.19 at 13:32, <roger.pau@citrix.com> wrote:
> On Thu, Jan 17, 2019 at 05:20:18AM -0700, Jan Beulich wrote:
>> >>> On 17.01.19 at 12:56, <roger.pau@citrix.com> wrote:
>> > On Thu, Jan 17, 2019 at 04:52:42AM -0700, Jan Beulich wrote:
>> >> >>> On 17.01.19 at 09:57, <roger.pau@citrix.com> wrote:
>> >> > While not against using physdevop if we agree that a new hypercall is
>> >> > the way to go, I would prefer a domctl because this hypercall would
>> >> > only be used by toolstack components, and thus doesn't need to be
>> >> > added to the public stable ABI available to all guests, even if the
>> >> > functionality is actually limited to stubdomains.
>> >> 
>> >> But a new sub-op doesn't need to be part of the stable ABI.
>> >> See how e.g. various of the memory sub-ops are restricted to
>> >> be used by the tool stack, and hence not required to remain
>> >> unchanged.
>> > 
>> > Oh, then I'm all in for a physdevop limited to stubdomain only usage.
>> 
>> Hmm, stubdomain is different: How would you limit this in the
>> header?
> 
> Oh, I wasn't meaning to limit this in the header, but in the
> implementation. Ie: by returning an error when called from
> non-stubdomains.
> 
> I don't see a reason to allow non-stubdomains to make use of this new
> hypercall if it's not required, that would just expand the attack
> surface for no good reason IMO.

Restricting it in the implementation is certainly fine, and indeed
desirable. But the ABI stability guarantees are reflected by
__XEN_TOOLS__ conditionals in the public headers.

Jan



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

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

[Marek Marczykowski-Górecki]

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

On Wed, Jan 16, 2019 at 11:52:21AM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 15, 2019 at 04:36:31PM +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.
> > 
> > I see, that's not a problem AFAICT for PCI INTx because the 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).
> 
> Exactly.
> 
> > > 
> > > 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>
> > > ---
(...)
> > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > index 8b44d6c..123ca69 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) &&
> > 
> > This check is already performed below, maybe you could re-arrange the
> > code as:
> > 
> > case MAP_PIRQ_TYPE_MULTI_MSI:
> >         irq = create_irq(NUMA_NO_NODE);
> >     }
> > 
> >     if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> >     {
> >         dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> >                 d->domain_id);
> >         return -EINVAL;
> >     }
> >     if ( current->domain->target == d )
> >         ...
> > 
> > But I wonder whether it would be better to place the irq_permit_access
> > in map_domain_pirq, together with the existing irq_permit_access that
> > grant the target domain permissions over the irq.
> 
> That may be a good idea. Let me try that in v3. But I'll wait for a
> feedback on libxl patches first.

That doesn't work, as map_domain_pirq() check irq access earlier.
Which bring be to a question, what should be rules guarding stubdomain
access to PHYSDEVOP_map_pirq? 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).

-- 
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 v2 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

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

On Fri, Jan 25, 2019 at 08:43:59PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 11:52:21AM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > > On Tue, Jan 15, 2019 at 04:36:31PM +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.
> > > 
> > > I see, that's not a problem AFAICT for PCI INTx because the 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).
> > 
> > Exactly.
> > 
> > > > 
> > > > 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>
> > > > ---
> (...)
> > > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > > index 8b44d6c..123ca69 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) &&
> > > 
> > > This check is already performed below, maybe you could re-arrange the
> > > code as:
> > > 
> > > case MAP_PIRQ_TYPE_MULTI_MSI:
> > >         irq = create_irq(NUMA_NO_NODE);
> > >     }
> > > 
> > >     if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > >     {
> > >         dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> > >                 d->domain_id);
> > >         return -EINVAL;
> > >     }
> > >     if ( current->domain->target == d )
> > >         ...

Oh, and this won't fly either, as irq_permit_access() should happen only
when create_irq() was called, otherwise it will give access to arbitrary
irq.

> > > 
> > > But I wonder whether it would be better to place the irq_permit_access
> > > in map_domain_pirq, together with the existing irq_permit_access that
> > > grant the target domain permissions over the irq.
> > 
> > That may be a good idea. Let me try that in v3. But I'll wait for a
> > feedback on libxl patches first.
> 
> That doesn't work, as map_domain_pirq() check irq access earlier.
> Which bring be to a question, what should be rules guarding stubdomain
> access to PHYSDEVOP_map_pirq? 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).
> 



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