All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] x86/hvm: PIRQ related cleanup and a fix
@ 2022-03-03 10:30 Roger Pau Monne
  2022-03-03 10:30 ` [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported Roger Pau Monne
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Roger Pau Monne @ 2022-03-03 10:30 UTC (permalink / raw)
  To: xen-devel
  Cc: Roger Pau Monne, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu

Hello,

First two patches are cleanup related to the usage of PIRQs from HVM
guests, and shouldn't result in any functional change.

Patch 3 allows the usage of PHYSDEVOP_{un,}map_pirq for HVM control
domains even when lacking support to route PIRQs over event channels.
This is done in order to allow setup of device interrupts assigned to
different guests for passthrough support. Note that using passthrough
from a PVH dom0 with vPCI in a safe way will at least require proper
locking around PCI devices, and likely other fixes.

Roger Pau Monne (3):
  evtchn/hvm: do not allow binding PIRQs unless supported
  hvm/irq: tighten check in hvm_domain_use_pirq
  hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq

 xen/arch/x86/hvm/hypercall.c | 7 +++++++
 xen/arch/x86/hvm/irq.c       | 2 +-
 xen/common/event_channel.c   | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported
  2022-03-03 10:30 [PATCH 0/3] x86/hvm: PIRQ related cleanup and a fix Roger Pau Monne
@ 2022-03-03 10:30 ` Roger Pau Monne
  2022-03-03 12:36   ` Jan Beulich
  2022-03-03 10:30 ` [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq Roger Pau Monne
  2022-03-03 10:30 ` [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq Roger Pau Monne
  2 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monne @ 2022-03-03 10:30 UTC (permalink / raw)
  To: xen-devel
  Cc: Roger Pau Monne, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu

HVM (or PVH) domain not having PIRQ support won't be allowed to map PIRQs in
the first place, but would still be allowed usage of
EVTCHNOP_bind_pirq. Such hypercall won't have any practical effect on
the domain, as the event channels would never be bound to PIRQs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/event_channel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index ffb042a241..bc4985706a 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -556,6 +556,9 @@ static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
     int            port = 0, rc;
     unsigned int   pirq = bind->pirq;
 
+    if ( is_hvm_domain(d) && !has_pirq(d) )
+        return -ENOSYS;
+
     if ( pirq >= d->nr_pirqs )
         return -EINVAL;
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq
  2022-03-03 10:30 [PATCH 0/3] x86/hvm: PIRQ related cleanup and a fix Roger Pau Monne
  2022-03-03 10:30 ` [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported Roger Pau Monne
@ 2022-03-03 10:30 ` Roger Pau Monne
  2022-03-03 12:44   ` Jan Beulich
  2022-03-03 10:30 ` [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq Roger Pau Monne
  2 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monne @ 2022-03-03 10:30 UTC (permalink / raw)
  To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu

hvm_domain_use_pirq checking whether the passed domain is an HVM
guests is pointless, as all calls originate from HVM only paths.
Instead check whether the domain has PIRQ support in order to avoid
further checks.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 5a7f39b54f..7c5dfd3c3a 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -30,7 +30,7 @@
 
 bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
 {
-    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
+    return has_pirq(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
 }
 
 /* Must be called with hvm_domain->irq_lock hold */
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 10:30 [PATCH 0/3] x86/hvm: PIRQ related cleanup and a fix Roger Pau Monne
  2022-03-03 10:30 ` [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported Roger Pau Monne
  2022-03-03 10:30 ` [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq Roger Pau Monne
@ 2022-03-03 10:30 ` Roger Pau Monne
  2022-03-03 10:45   ` Andrew Cooper
  2022-03-03 16:45   ` Alex Olson
  2 siblings, 2 replies; 15+ messages in thread
From: Roger Pau Monne @ 2022-03-03 10:30 UTC (permalink / raw)
  To: xen-devel
  Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu, Alex Olson,
	Andrew Cooper

Control domains (including domains having control over a single other
guest) need access to PHYSDEVOP_{un,}map_pirq in order to setup
bindings of interrupts from devices assigned to the controlled guest.

As such relax the check for HVM based guests and allow the usage of
the hypercalls for any control domains. Note that further safety
checks will be performed in order to assert that the current domain
has the right permissions against the target of the hypercall.

Reported-by: Alex Olson <this.is.a0lson@gmail.com>
Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/hypercall.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 030243810e..9128e4d025 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
+        /*
+         * Control domain (and domains controlling others) need to use
+         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for passthrough
+         * devices on behalf of other guests.
+         */
+        if ( is_control_domain(currd) || currd->target )
+            break;
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 10:30 ` [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq Roger Pau Monne
@ 2022-03-03 10:45   ` Andrew Cooper
  2022-03-03 10:59     ` Jan Beulich
  2022-03-03 11:40     ` Roger Pau Monné
  2022-03-03 16:45   ` Alex Olson
  1 sibling, 2 replies; 15+ messages in thread
From: Andrew Cooper @ 2022-03-03 10:45 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich, Wei Liu, Alex Olson

On 03/03/2022 10:30, Roger Pau Monne wrote:
> Control domains (including domains having control over a single other
> guest) need access to PHYSDEVOP_{un,}map_pirq in order to setup
> bindings of interrupts from devices assigned to the controlled guest.
>
> As such relax the check for HVM based guests and allow the usage of
> the hypercalls for any control domains. Note that further safety
> checks will be performed in order to assert that the current domain
> has the right permissions against the target of the hypercall.
>
> Reported-by: Alex Olson <this.is.a0lson@gmail.com>
> Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/hypercall.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 030243810e..9128e4d025 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        /*
> +         * Control domain (and domains controlling others) need to use
> +         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for passthrough
> +         * devices on behalf of other guests.
> +         */
> +        if ( is_control_domain(currd) || currd->target )
> +            break;

Hmm.  In a split control/hardware domain model, then qemu is in the
hardware domain rather than the control domain.  I suspect this wants
extending with || is_hardware_domain(currd).

Also, the sentence about later safety checks really ought to be in this
source comment too.

~Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 10:45   ` Andrew Cooper
@ 2022-03-03 10:59     ` Jan Beulich
  2022-03-03 11:40     ` Roger Pau Monné
  1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-03 10:59 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Alex Olson, xen-devel, Roger Pau Monne

On 03.03.2022 11:45, Andrew Cooper wrote:
> On 03/03/2022 10:30, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>      {
>>      case PHYSDEVOP_map_pirq:
>>      case PHYSDEVOP_unmap_pirq:
>> +        /*
>> +         * Control domain (and domains controlling others) need to use
>> +         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for passthrough
>> +         * devices on behalf of other guests.
>> +         */
>> +        if ( is_control_domain(currd) || currd->target )
>> +            break;
> 
> Hmm.  In a split control/hardware domain model, then qemu is in the
> hardware domain rather than the control domain.

Interesting. I would have expected it to be the other way around, with
qemu for domains with pass-through devices living in a stubdom.

Jan

>  I suspect this wants
> extending with || is_hardware_domain(currd).
> 
> Also, the sentence about later safety checks really ought to be in this
> source comment too.
> 
> ~Andrew



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 10:45   ` Andrew Cooper
  2022-03-03 10:59     ` Jan Beulich
@ 2022-03-03 11:40     ` Roger Pau Monné
  2022-03-03 11:43       ` Jan Beulich
  1 sibling, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2022-03-03 11:40 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Jan Beulich, Wei Liu, Alex Olson

On Thu, Mar 03, 2022 at 10:45:54AM +0000, Andrew Cooper wrote:
> On 03/03/2022 10:30, Roger Pau Monne wrote:
> > Control domains (including domains having control over a single other
> > guest) need access to PHYSDEVOP_{un,}map_pirq in order to setup
> > bindings of interrupts from devices assigned to the controlled guest.
> >
> > As such relax the check for HVM based guests and allow the usage of
> > the hypercalls for any control domains. Note that further safety
> > checks will be performed in order to assert that the current domain
> > has the right permissions against the target of the hypercall.
> >
> > Reported-by: Alex Olson <this.is.a0lson@gmail.com>
> > Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/hvm/hypercall.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> > index 030243810e..9128e4d025 100644
> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >      case PHYSDEVOP_map_pirq:
> >      case PHYSDEVOP_unmap_pirq:
> > +        /*
> > +         * Control domain (and domains controlling others) need to use
> > +         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for passthrough
> > +         * devices on behalf of other guests.
> > +         */
> > +        if ( is_control_domain(currd) || currd->target )
> > +            break;
> 
> Hmm.  In a split control/hardware domain model, then qemu is in the
> hardware domain rather than the control domain.  I suspect this wants
> extending with || is_hardware_domain(currd).

The binding of GSIs is exclusively done by the control domain because
those are static and known at domain creation.  The mapping and
binding of MSI interrupts is however done by QEMU at runtime, so it
needs extending to the hardware domain.

However just extending here won't be enough: we would also need to
modify xsm_default_action, as currently XSM_DM_PRIV will only be
allowed if src->target == target or is_control_domain(src).

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 58afc1d589..ac40a24a22 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -88,7 +88,8 @@ static always_inline int xsm_default_action(
         }
         /* fall through */
     case XSM_DM_PRIV:
-        if ( target && evaluate_nospec(src->target == target) )
+        if ( is_hardware_domain(src) ||
+             (target && evaluate_nospec(src->target == target)) )
             return 0;
         /* fall through */
     case XSM_PRIV:

That would however also give the hardware domain access to XSM_TARGET
and XSM_XS_PRIV operations that where previously forbidden.

Or do we just require people with split control/hardware domain model
to also use a different XSM policy?

> Also, the sentence about later safety checks really ought to be in this
> source comment too.

Will add.

Thanks, Roger.


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 11:40     ` Roger Pau Monné
@ 2022-03-03 11:43       ` Jan Beulich
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-03 11:43 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Alex Olson, Andrew Cooper

On 03.03.2022 12:40, Roger Pau Monné wrote:
> Or do we just require people with split control/hardware domain model
> to also use a different XSM policy?

I've been under the impression that this is the intended model, yes.

Jan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported
  2022-03-03 10:30 ` [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported Roger Pau Monne
@ 2022-03-03 12:36   ` Jan Beulich
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-03 12:36 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
	Wei Liu, xen-devel

On 03.03.2022 11:30, Roger Pau Monne wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -556,6 +556,9 @@ static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
>      int            port = 0, rc;
>      unsigned int   pirq = bind->pirq;
>  
> +    if ( is_hvm_domain(d) && !has_pirq(d) )

Arm doesn't have has_pirq(), so some further conditional will be needed,
or has_pirq() needs adding there.

Doesn't this want further accompanying with adjustments to the checks in
physdev_{,un}map_pirq()?

> +        return -ENOSYS;

-EOPNOTSUPP please.

Jan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq
  2022-03-03 10:30 ` [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq Roger Pau Monne
@ 2022-03-03 12:44   ` Jan Beulich
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-03 12:44 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel

On 03.03.2022 11:30, Roger Pau Monne wrote:
> hvm_domain_use_pirq checking whether the passed domain is an HVM
> guests is pointless, as all calls originate from HVM only paths.
> Instead check whether the domain has PIRQ support in order to avoid
> further checks.

I agree with this, but I wonder ...

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -30,7 +30,7 @@
>  
>  bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>  {
> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> +    return has_pirq(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;

... whether there can be a non-NULL pirq in the first place for a
!has_pirq() domain. Judging from e.g. hvm_inject_msi() it looks like
this might be possible, but perhaps wrongly so?

Jan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 10:30 ` [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq Roger Pau Monne
  2022-03-03 10:45   ` Andrew Cooper
@ 2022-03-03 16:45   ` Alex Olson
  2022-03-03 16:47     ` Jan Beulich
  1 sibling, 1 reply; 15+ messages in thread
From: Alex Olson @ 2022-03-03 16:45 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich, Andrew Cooper, Wei Liu

Hi Roger,

Thanks for the patches.  In trying them out, I found some other PHYSDEVOP
commands that were being blocked by the "default" case and were being failed
with -ENOSYS... 

Would something like the change below make sense?  Or is defaulting to failure
incorrect?   (I saw denials for the "add" commands, and also added the
remove/release commands for symmetry).

With this change, I was able to achieve a functional virtual function passed
through to a HVM domain with PVH dom0.

Thanks

-Alex


diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index b8becab475..6abaa626a3 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -84,6 +84,17 @@ static long hvm_physdev_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
 
     switch ( cmd )
     {
+
+    case PHYSDEVOP_manage_pci_add:
+    case PHYSDEVOP_manage_pci_remove:
+    case PHYSDEVOP_pci_device_add:
+    case PHYSDEVOP_pci_device_remove:
+    case PHYSDEVOP_manage_pci_add_ext:
+    case PHYSDEVOP_prepare_msix:
+    case PHYSDEVOP_release_msix:
+        if ( is_control_domain(currd) )
+            break;
+
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
         /*



On Thu, 2022-03-03 at 11:30 +0100, Roger Pau Monne wrote:
> Control domains (including domains having control over a single other
> guest) need access to PHYSDEVOP_{un,}map_pirq in order to setup
> bindings of interrupts from devices assigned to the controlled guest.
> 
> As such relax the check for HVM based guests and allow the usage of
> the hypercalls for any control domains. Note that further safety
> checks will be performed in order to assert that the current domain
> has the right permissions against the target of the hypercall.
> 
> Reported-by: Alex Olson <this.is.a0lson@gmail.com>
> Reported-by: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/hypercall.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 030243810e..9128e4d025 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -87,6 +87,13 @@ static long hvm_physdev_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        /*
> +         * Control domain (and domains controlling others) need to use
> +         * PHYSDEVOP_{un,}map_pirq in order to setup interrupts for
> passthrough
> +         * devices on behalf of other guests.
> +         */
> +        if ( is_control_domain(currd) || currd->target )
> +            break;
>      case PHYSDEVOP_eoi:
>      case PHYSDEVOP_irq_status_query:
>      case PHYSDEVOP_get_free_pirq:



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 16:45   ` Alex Olson
@ 2022-03-03 16:47     ` Jan Beulich
  2022-03-03 17:14       ` Alex Olson
  2022-03-04 11:47       ` Roger Pau Monné
  0 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-03 16:47 UTC (permalink / raw)
  To: Alex Olson; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne, xen-devel

On 03.03.2022 17:45, Alex Olson wrote:
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -84,6 +84,17 @@ static long hvm_physdev_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      switch ( cmd )
>      {
> +
> +    case PHYSDEVOP_manage_pci_add:
> +    case PHYSDEVOP_manage_pci_remove:
> +    case PHYSDEVOP_pci_device_add:
> +    case PHYSDEVOP_pci_device_remove:
> +    case PHYSDEVOP_manage_pci_add_ext:
> +    case PHYSDEVOP_prepare_msix:
> +    case PHYSDEVOP_release_msix:
> +        if ( is_control_domain(currd) )
> +            break;

These are all operations which I think are purposefully permitted to
be invoked by the hardware domain only. That's where all the devices
live when they're not passed through to guests.

Jan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 16:47     ` Jan Beulich
@ 2022-03-03 17:14       ` Alex Olson
  2022-03-04  7:02         ` Jan Beulich
  2022-03-04 11:47       ` Roger Pau Monné
  1 sibling, 1 reply; 15+ messages in thread
From: Alex Olson @ 2022-03-03 17:14 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne, xen-devel

I wasn't sure of the distinction between hardware domain and control domain for these commands, but they appear to be blocked at the moment when dom0 executes them, including a lot at boot.  Are you suggesting to use is_hardware_domain(currd) instead in my diff?    

Or should the hardware domain always be able to execute any physdev op command? (such as to bypass the switch statement entirely)


It looks like hvm_physdev_op() is the only real caller of do_physdev_op(), and  several other commands (besides the ones in the diff below) are also being blocked by the default case of hvm_physdev_op.

PHYSDEVOP_pirq_eoi_gmfn_v2
PHYSDEVOP_pirq_eoi_gmfn_v1
PHYSDEVOP_IRQ_UNMASK_NOTIFY // legacy?
PHYSDEVOP_apic_read
PHYSDEVOP_apic_write
PHYSDEVOP_alloc_irq_vector
PHYSDEVOP_set_iopl
PHYSDEVOP_set_iobitmap
PHYSDEVOP_restore_msi
PHYSDEVOP_restore_msi_ext
PHYSDEVOP_setup_gsi
PHYSDEVOP_get_free_pirq
PHYSDEVOP_dbgp_op

Thanks

-Alex

On Thu, 2022-03-03 at 17:47 +0100, Jan Beulich wrote:
> On 03.03.2022 17:45, Alex Olson wrote:
> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -84,6 +84,17 @@ static long hvm_physdev_op(int cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >  
> >      switch ( cmd )
> >      {
> > +
> > +    case PHYSDEVOP_manage_pci_add:
> > +    case PHYSDEVOP_manage_pci_remove:
> > +    case PHYSDEVOP_pci_device_add:
> > +    case PHYSDEVOP_pci_device_remove:
> > +    case PHYSDEVOP_manage_pci_add_ext:
> > +    case PHYSDEVOP_prepare_msix:
> > +    case PHYSDEVOP_release_msix:
> > +        if ( is_control_domain(currd) )
> > +            break;
> 
> These are all operations which I think are purposefully permitted to
> be invoked by the hardware domain only. That's where all the devices
> live when they're not passed through to guests.
> 
> Jan
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 17:14       ` Alex Olson
@ 2022-03-04  7:02         ` Jan Beulich
  0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2022-03-04  7:02 UTC (permalink / raw)
  To: Alex Olson; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monne, xen-devel

On 03.03.2022 18:14, Alex Olson wrote:
> I wasn't sure of the distinction between hardware domain and control domain for these commands, but they appear to be blocked at the moment when dom0 executes them, including a lot at boot.  Are you suggesting to use is_hardware_domain(currd) instead in my diff?    
> 
> Or should the hardware domain always be able to execute any physdev op command? (such as to bypass the switch statement entirely)

No, certainly not. It was on purpose to restrict PVH Dom0. Only PV
Dom0 is supposed to be able to access all sub-ops.

> It looks like hvm_physdev_op() is the only real caller of do_physdev_op(), and  several other commands (besides the ones in the diff below) are also being blocked by the default case of hvm_physdev_op.
> 
> PHYSDEVOP_pirq_eoi_gmfn_v2
> PHYSDEVOP_pirq_eoi_gmfn_v1
> PHYSDEVOP_IRQ_UNMASK_NOTIFY // legacy?
> PHYSDEVOP_apic_read
> PHYSDEVOP_apic_write
> PHYSDEVOP_alloc_irq_vector
> PHYSDEVOP_set_iopl
> PHYSDEVOP_set_iobitmap
> PHYSDEVOP_restore_msi
> PHYSDEVOP_restore_msi_ext
> PHYSDEVOP_setup_gsi
> PHYSDEVOP_get_free_pirq
> PHYSDEVOP_dbgp_op
> 
> Thanks
> 
> -Alex

Also - please don't top-post.

Jan

> On Thu, 2022-03-03 at 17:47 +0100, Jan Beulich wrote:
>> On 03.03.2022 17:45, Alex Olson wrote:
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -84,6 +84,17 @@ static long hvm_physdev_op(int cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>  
>>>      switch ( cmd )
>>>      {
>>> +
>>> +    case PHYSDEVOP_manage_pci_add:
>>> +    case PHYSDEVOP_manage_pci_remove:
>>> +    case PHYSDEVOP_pci_device_add:
>>> +    case PHYSDEVOP_pci_device_remove:
>>> +    case PHYSDEVOP_manage_pci_add_ext:
>>> +    case PHYSDEVOP_prepare_msix:
>>> +    case PHYSDEVOP_release_msix:
>>> +        if ( is_control_domain(currd) )
>>> +            break;
>>
>> These are all operations which I think are purposefully permitted to
>> be invoked by the hardware domain only. That's where all the devices
>> live when they're not passed through to guests.
>>
>> Jan
>>
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq
  2022-03-03 16:47     ` Jan Beulich
  2022-03-03 17:14       ` Alex Olson
@ 2022-03-04 11:47       ` Roger Pau Monné
  1 sibling, 0 replies; 15+ messages in thread
From: Roger Pau Monné @ 2022-03-04 11:47 UTC (permalink / raw)
  To: Jan Beulich, Alex Olson; +Cc: Andrew Cooper, Wei Liu, xen-devel

On Thu, Mar 03, 2022 at 05:47:48PM +0100, Jan Beulich wrote:
> On 03.03.2022 17:45, Alex Olson wrote:
> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -84,6 +84,17 @@ static long hvm_physdev_op(int cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >  
> >      switch ( cmd )
> >      {
> > +
> > +    case PHYSDEVOP_manage_pci_add:
> > +    case PHYSDEVOP_manage_pci_remove:
> > +    case PHYSDEVOP_pci_device_add:
> > +    case PHYSDEVOP_pci_device_remove:

The add/remove options are already available to a PVH hardware
domain.

> > +    case PHYSDEVOP_manage_pci_add_ext:

This shouldn't be needed in principle for a PVH hardware domain, as
the plan it to emulate accesses to the SR-IOV capability and detect
VFs that way.

> > +    case PHYSDEVOP_prepare_msix:
> > +    case PHYSDEVOP_release_msix:

Those two are likely fine to use for a PVH hardware domain (not the
control domain). AFAICT they shouldn't interact badly with vPCI.

> > +        if ( is_control_domain(currd) )
> > +            break;
> 
> These are all operations which I think are purposefully permitted to
> be invoked by the hardware domain only. That's where all the devices
> live when they're not passed through to guests.

Indeed. I think it's only the {prepare,release}_msix operations that
needs adding (but would need confirmation they actually work as
intended in a PVH setup).

Thanks, Roger.


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-03-04 11:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-03 10:30 [PATCH 0/3] x86/hvm: PIRQ related cleanup and a fix Roger Pau Monne
2022-03-03 10:30 ` [PATCH 1/3] evtchn/hvm: do not allow binding PIRQs unless supported Roger Pau Monne
2022-03-03 12:36   ` Jan Beulich
2022-03-03 10:30 ` [PATCH 2/3] hvm/irq: tighten check in hvm_domain_use_pirq Roger Pau Monne
2022-03-03 12:44   ` Jan Beulich
2022-03-03 10:30 ` [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq Roger Pau Monne
2022-03-03 10:45   ` Andrew Cooper
2022-03-03 10:59     ` Jan Beulich
2022-03-03 11:40     ` Roger Pau Monné
2022-03-03 11:43       ` Jan Beulich
2022-03-03 16:45   ` Alex Olson
2022-03-03 16:47     ` Jan Beulich
2022-03-03 17:14       ` Alex Olson
2022-03-04  7:02         ` Jan Beulich
2022-03-04 11:47       ` Roger Pau Monné

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.