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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Feb 08, 2019 at 11:17:05AM +0100, Marek Marczykowski-Górecki wrote:
> Stubdomains need to be given sufficient privilege over the guest which it
> provides emulation for in order for PCI passthrough to work correctly.
> When a HVM domain try to enable MSI, QEMU in stubdomain calls
> PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
> part of xc_domain_update_msi_irq. Allow for that as part of
> PHYSDEVOP_map_pirq.
> 
> This is not needed for PCI INTx, because IRQ in that case is known
> beforehand and the stubdomain is given permissions over this IRQ by
> libxl__device_pci_add (there's a do_pci_add against the stubdomain).
> 
> create_irq() already grant IRQ access to hardware_domain, with
> assumption the device model (something managing this IRQ) lives there.
> Modify create_irq() to take additional parameter pointing at device
> model domain - which may be dom0 or stubdomain. Do the same with
> destroy_irq() (and msi_free_irq() which calls it) to reverse the
> operation.
> 
> Then, adjust all callers to provide the parameter. In case of calls not
> related to stubdomain-initiated allocations, give it hardware_domain, so
> the behavior is unchanged there.
> 
> Inspired by 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>

Thanks for working on this! I've got some comments below.

> ---
> Changes in v3:
>  - extend commit message
> Changes in v4:
>  - add missing destroy_irq on error path
> Changes in v4.1:
>  - move irq_{grant,revoke}_access() to {create,destroy}_irq(), which
>    basically make it a different patch
> 
> There is one code path where I haven't managed to properly extract
> possible stubdomain in use:
> pci_remove_device()
>  -> pci_cleanup_msi()
>    -> msi_free_irqs()
>      -> msi_free_irq()
>        -> destroy_irq()
> 
> For now I've hardcoded hardware_domain there (in msi_free_irqs). Can it happen
> when device is still assigned to some domU?
> ---
>  xen/arch/x86/hpet.c                      |  5 +--
>  xen/arch/x86/irq.c                       | 46 ++++++++++++++----------
>  xen/arch/x86/msi.c                       |  6 ++--
>  xen/drivers/char/ns16550.c               |  6 ++--
>  xen/drivers/passthrough/amd/iommu_init.c |  4 +--
>  xen/drivers/passthrough/vtd/iommu.c      |  7 ++--
>  xen/include/asm-x86/irq.h                |  4 +--
>  xen/include/asm-x86/msi.h                |  2 +-
>  8 files changed, 46 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 4b08488ef1..6db71dfd71 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -11,6 +11,7 @@
>  #include <xen/softirq.h>
>  #include <xen/irq.h>
>  #include <xen/numa.h>
> +#include <xen/sched.h>
>  #include <asm/fixmap.h>
>  #include <asm/div64.h>
>  #include <asm/hpet.h>
> @@ -368,13 +369,13 @@ static int __init hpet_assign_irq(struct hpet_event_channel *ch)
>  {
>      int irq;
>  
> -    if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
> +    if ( (irq = create_irq(NUMA_NO_NODE, hardware_domain)) < 0 )
>          return irq;
>  
>      ch->msi.irq = irq;
>      if ( hpet_setup_msi_irq(ch) )
>      {
> -        destroy_irq(irq);
> +        destroy_irq(irq, hardware_domain);

Hm, here you should use an explicit NULL instead of hardware_domain
(which will also be NULL by the time this gets called). This irq is
used by Xen, and it doesn't make sense logically to give permissions
over it to the hardware domain.

>          return -EINVAL;
>      }
>  
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 8b44d6ce0b..d41b32b2f4 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -157,7 +157,7 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
>  /*
>   * Dynamic irq allocate and deallocation for MSI
>   */
> -int create_irq(nodeid_t node)
> +int create_irq(nodeid_t node, struct domain *dm_domain)

Using plain 'd' would be fine for me here, same below for
destroy_irq.

>  {
>      int irq, ret;
>      struct irq_desc *desc;
> @@ -190,19 +190,19 @@ int create_irq(nodeid_t node)
>          desc->arch.used = IRQ_UNUSED;
>          irq = ret;
>      }
> -    else if ( hardware_domain )
> +    else if ( dm_domain )

Can you guarantee that dm_domain is always current->domain?

I think you need to assert for this, or else take a reference to
dm_domain (get_domain) before accessing it's fields, or else you risk
the domain being destroyed while modifying it's fields.

>      {
> -        ret = irq_permit_access(hardware_domain, irq);
> +        ret = irq_permit_access(dm_domain, irq);
>          if ( ret )
>              printk(XENLOG_G_ERR
> -                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
> -                   irq, ret);
> +                   "Could not grant Dom%u access to IRQ%d (error %d)\n",
> +                   dm_domain->domain_id, irq, ret);
>      }
>  
>      return irq;
>  }
>  
> -void destroy_irq(unsigned int irq)
> +void destroy_irq(unsigned int irq, struct domain *dm_domain)
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
>      unsigned long flags;
> @@ -210,14 +210,14 @@ void destroy_irq(unsigned int irq)
>  
>      BUG_ON(!MSI_IRQ(irq));
>  
> -    if ( hardware_domain )
> +    if ( dm_domain )
>      {
> -        int err = irq_deny_access(hardware_domain, irq);
> +        int err = irq_deny_access(dm_domain, irq);
>  
>          if ( err )
>              printk(XENLOG_G_ERR
> -                   "Could not revoke Dom0 access to IRQ%u (error %d)\n",
> -                   irq, err);
> +                   "Could not revoke Dom%u access to IRQ%u (error %d)\n",
> +                   dm_domain->domain_id, irq, err);
>      }
>  
>      spin_lock_irqsave(&desc->lock, flags);
> @@ -2010,7 +2010,9 @@ int map_domain_pirq(
>                      d->domain_id, irq);
>              pci_disable_msi(msi_desc);
>              msi_desc->irq = -1;
> -            msi_free_irq(msi_desc);
> +            msi_free_irq(msi_desc,
> +                         current->domain->target == d ? current->domain
> +                                                      : hardware_domain);
>              ret = -EBUSY;
>              goto done;
>          }
> @@ -2038,7 +2040,9 @@ int map_domain_pirq(
>              spin_unlock_irqrestore(&desc->lock, flags);
>  
>              info = NULL;
> -            irq = create_irq(NUMA_NO_NODE);
> +            irq = create_irq(NUMA_NO_NODE,
> +                             current->domain->target == d ? current->domain
> +                                                          : hardware_domain);
>              ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
>                             : irq;
>              if ( ret < 0 )
> @@ -2095,7 +2099,9 @@ int map_domain_pirq(
>                  irq = info->arch.irq;
>              }
>              msi_desc->irq = -1;
> -            msi_free_irq(msi_desc);
> +            msi_free_irq(msi_desc,
> +                         current->domain->target == d ? current->domain
> +                                                      : hardware_domain);
>              goto done;
>          }
>  
> @@ -2255,7 +2261,9 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>      }
>  
>      if (msi_desc)
> -        msi_free_irq(msi_desc);
> +        msi_free_irq(msi_desc,
> +                     current->domain->target == d ? current->domain
> +                                                  : hardware_domain);
>  
>   done:
>      return ret;
> @@ -2671,10 +2679,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
>              msi->entry_nr = 1;
>          irq = index;
>          if ( irq == -1 )
> -        {
>      case MAP_PIRQ_TYPE_MULTI_MSI:
> -            irq = create_irq(NUMA_NO_NODE);
> -        }
> +            irq = create_irq(NUMA_NO_NODE,
> +                             current->domain->target == d ? current->domain
> +                                                          : hardware_domain);
>  
>          if ( irq < nr_irqs_gsi || irq >= nr_irqs )
>          {
> @@ -2717,7 +2725,9 @@ 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:
> -                destroy_irq(irq);
> +                destroy_irq(irq,
> +                            current->domain->target == d ? current->domain
> +                                                         : hardware_domain);

This pattern seems common enough to get a helper, maybe get_dm_domain?

>              break;
>          }
>      }
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc4147c4..66026e3ca5 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -633,7 +633,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
>      return ret;
>  }
>  
> -int msi_free_irq(struct msi_desc *entry)
> +int msi_free_irq(struct msi_desc *entry, struct domain *dm_domain)
>  {
>      unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
>                        ? entry->msi.nvec : 1;
> @@ -641,7 +641,7 @@ int msi_free_irq(struct msi_desc *entry)
>      while ( nr-- )
>      {
>          if ( entry[nr].irq >= 0 )
> -            destroy_irq(entry[nr].irq);
> +            destroy_irq(entry[nr].irq, dm_domain);
>  
>          /* Free the unused IRTE if intr remap enabled */
>          if ( iommu_intremap )
> @@ -1280,7 +1280,7 @@ static void msi_free_irqs(struct pci_dev* dev)
>      list_for_each_entry_safe( entry, tmp, &dev->msi_list, list )
>      {
>          pci_disable_msi(entry);
> -        msi_free_irq(entry);
> +        msi_free_irq(entry, hardware_domain);

This likely requires some comment to clarify why is the hardcoding of
hardware_domain correct here. AFAICT this will be called by
pci_remove_device, which I assume assures that the device has been
deassigned from any domain before attempting to remove it, hence it
can only have irqs assigned to the hardware_domain if any?

>      }
>  }
>  
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 189e121b7e..2037bbbf08 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -719,7 +719,7 @@ static void __init ns16550_init_irq(struct serial_port *port)
>      struct ns16550 *uart = port->uart;
>  
>      if ( uart->msi )
> -        uart->irq = create_irq(0);
> +        uart->irq = create_irq(0, hardware_domain);
>  #endif
>  }
>  
> @@ -812,9 +812,9 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>                  {
>                      uart->irq = 0;
>                      if ( msi_desc )
> -                        msi_free_irq(msi_desc);
> +                        msi_free_irq(msi_desc, hardware_domain);
>                      else
> -                        destroy_irq(msi.irq);
> +                        destroy_irq(msi.irq, hardware_domain);

All this calls should pass a NULL domain IMO, since this is the serial
console driver used by Xen.

>                  }
>              }
>  
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 17f39552a9..423c473a63 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -780,7 +780,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
>      hw_irq_controller *handler;
>      u16 control;
>  
> -    irq = create_irq(NUMA_NO_NODE);
> +    irq = create_irq(NUMA_NO_NODE, hardware_domain);
>      if ( irq <= 0 )
>      {
>          dprintk(XENLOG_ERR, "IOMMU: no irqs\n");
> @@ -816,7 +816,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
>          ret = request_irq(irq, 0, iommu_interrupt_handler, "amd_iommu", iommu);
>      if ( ret )
>      {
> -        destroy_irq(irq);
> +        destroy_irq(irq, hardware_domain);

Same here, this is the iommu used by Xen, hence the domain parameter
should be NULL.

>          AMD_IOMMU_DEBUG("can't request irq\n");
>          return 0;
>      }
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 50a0e25224..73cf16c145 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1138,7 +1138,8 @@ static int __init iommu_set_interrupt(struct acpi_drhd_unit *drhd)
>      struct irq_desc *desc;
>  
>      irq = create_irq(rhsa ? pxm_to_node(rhsa->proximity_domain)
> -                          : NUMA_NO_NODE);
> +                          : NUMA_NO_NODE,
> +                     hardware_domain);
>      if ( irq <= 0 )
>      {
>          dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no irq available!\n");
> @@ -1151,7 +1152,7 @@ static int __init iommu_set_interrupt(struct acpi_drhd_unit *drhd)
>      if ( ret )
>      {
>          desc->handler = &no_irq_type;
> -        destroy_irq(irq);
> +        destroy_irq(irq, hardware_domain);
>          dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: can't request irq\n");
>          return ret;
>      }
> @@ -1286,7 +1287,7 @@ void __init iommu_free(struct acpi_drhd_unit *drhd)
>  
>      free_intel_iommu(iommu->intel);
>      if ( iommu->msi.irq >= 0 )
> -        destroy_irq(iommu->msi.irq);
> +        destroy_irq(iommu->msi.irq, hardware_domain);

Same here, should be all NULL.

Thanks, Roger.

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