Re: [PATCH 3/3] hvm/pirq: allow control domains usage of PHYSDEVOP_{un,}map_pirq

[Alex Olson <this.is.a0lson@xxxxxxxxx>]

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@xxxxxxxxx>
> Reported-by: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  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: