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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 15, 2019 at 04:36:31PM +0100, Marek Marczykowski-Górecki wrote:
> From: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>.
> 
> Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel