[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v3][PATCH 04/16] xen/passthrough: extend hypercall to support rdm reservation policy



> From: Chen, Tiejun
> Sent: Thursday, June 11, 2015 9:15 AM
> 
> This patch extends the existing hypercall to support rdm reservation policy.
> We return error or just throw out a warning message depending on whether
> the policy is "strict" or "relaxed" when reserving RDM regions in pfn space.
> Note in some special cases, e.g. add a device to hwdomain, and remove a
> device from user domain, 'relaxed' is fine enough since this is always safe
> to hwdomain.

could you elaborate " add a device to hwdomain, and remove a device 
from user domain "? move a device from user domain to hwdomain
or completely irrelevant?

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> ---
>  xen/arch/x86/mm/p2m.c                       |  8 +++++++-
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  3 ++-
>  xen/drivers/passthrough/arm/smmu.c          |  2 +-
>  xen/drivers/passthrough/device_tree.c       | 11 ++++++++++-
>  xen/drivers/passthrough/pci.c               | 10 ++++++----
>  xen/drivers/passthrough/vtd/iommu.c         | 20 ++++++++++++--------
>  xen/include/asm-x86/p2m.h                   |  2 +-
>  xen/include/public/domctl.h                 |  5 +++++
>  xen/include/xen/iommu.h                     |  2 +-
>  9 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index c7198a5..3fcdcac 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -899,7 +899,7 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
> gfn,
> mfn_t mfn,
>  }
> 
>  int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> -                           p2m_access_t p2ma)
> +                           p2m_access_t p2ma, u32 flag)
>  {
>      p2m_type_t p2mt;
>      p2m_access_t a;
> @@ -924,6 +924,12 @@ int set_identity_p2m_entry(struct domain *d, unsigned 
> long gfn,
>              printk(XENLOG_G_WARNING
>                     "Cannot identity map d%d:%lx, already mapped to %lx.\n",
>                     d->domain_id, gfn, mfn_x(mfn));
> +
> +            if ( flag == XEN_DOMCTL_DEV_RDM_RELAXED )
> +            {
> +                ret = 0;
> +                printk(XENLOG_G_WARNING "Some devices may work failed.\n");

Do you need this extra printk? The warning message is already given
several lines above and here you just need to change return value
for relaxed policy.

> +            }
>          }
> 
>          gfn_unlock(p2m, gfn, 0);
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index e83bb35..920b35a 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -394,7 +394,8 @@ static int reassign_device(struct domain *source, struct 
> domain
> *target,
>  }
> 
>  static int amd_iommu_assign_device(struct domain *d, u8 devfn,
> -                                   struct pci_dev *pdev)
> +                                   struct pci_dev *pdev,
> +                                   u32 flag)
>  {
>      struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(pdev->seg);
>      int bdf = PCI_BDF2(pdev->bus, devfn);
> diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> index 6cc4394..9a667e9 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2605,7 +2605,7 @@ static void arm_smmu_destroy_iommu_domain(struct
> iommu_domain *domain)
>  }
> 
>  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
> -                            struct device *dev)
> +                            struct device *dev, u32 flag)
>  {
>       struct iommu_domain *domain;
>       struct arm_smmu_xen_domain *xen_domain;
> diff --git a/xen/drivers/passthrough/device_tree.c
> b/xen/drivers/passthrough/device_tree.c
> index 5d3842a..ea85645 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -52,7 +52,8 @@ int iommu_assign_dt_device(struct domain *d, struct
> dt_device_node *dev)
>              goto fail;
>      }
> 
> -    rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev));
> +    rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev),
> +                                         XEN_DOMCTL_DEV_NO_RDM);
> 
>      if ( rc )
>          goto fail;
> @@ -148,6 +149,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
> domain *d,
>          if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>              break;
> 
> +        if ( domctl->u.assign_device.flag == XEN_DOMCTL_DEV_NO_RDM )
> +        {
> +            printk(XENLOG_G_ERR "XEN_DOMCTL_assign_device: assign \"%s\""
> +                   " to dom%u failed (%d) since we don't support RDM.\n",
> +                   dt_node_full_name(dev), d->domain_id, ret);
> +            break;
> +        }
> +
>          if ( unlikely(d->is_dying) )
>          {
>              ret = -EINVAL;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index e30be43..557c87e 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1335,7 +1335,7 @@ static int device_assigned(u16 seg, u8 bus, u8 devfn)
>      return pdev ? 0 : -EBUSY;
>  }
> 
> -static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
> +static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 
> flag)
>  {
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>      struct pci_dev *pdev;
> @@ -1371,7 +1371,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8
> devfn)
> 
>      pdev->fault.count = 0;
> 
> -    if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev))) )
> +    if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
> flag)) )
>          goto done;
> 
>      for ( ; pdev->phantom_stride; rc = 0 )
> @@ -1379,7 +1379,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8
> devfn)
>          devfn += pdev->phantom_stride;
>          if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
>              break;
> -        rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev));
> +        rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
> flag);
>          if ( rc )
>              printk(XENLOG_G_WARNING "d%d: assign %04x:%02x:%02x.%u failed
> (%d)\n",
>                     d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> @@ -1496,6 +1496,7 @@ int iommu_do_pci_domctl(
>  {
>      u16 seg;
>      u8 bus, devfn;
> +    u32 flag;
>      int ret = 0;
>      uint32_t machine_sbdf;
> 
> @@ -1577,9 +1578,10 @@ int iommu_do_pci_domctl(
>          seg = machine_sbdf >> 16;
>          bus = PCI_BUS(machine_sbdf);
>          devfn = PCI_DEVFN2(machine_sbdf);
> +        flag = domctl->u.assign_device.flag;
> 
>          ret = device_assigned(seg, bus, devfn) ?:
> -              assign_device(d, seg, bus, devfn);
> +              assign_device(d, seg, bus, devfn, flag);
>          if ( ret == -ERESTART )
>              ret = hypercall_create_continuation(__HYPERVISOR_domctl,
>                                                  "h", u_domctl);
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 31ce1af..d7c9e1c 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1808,7 +1808,8 @@ static void iommu_set_pgd(struct domain *d)
>  }
> 
>  static int rmrr_identity_mapping(struct domain *d, bool_t map,
> -                                 const struct acpi_rmrr_unit *rmrr)
> +                                 const struct acpi_rmrr_unit *rmrr,
> +                                 u32 flag)
>  {
>      unsigned long base_pfn = rmrr->base_address >> PAGE_SHIFT_4K;
>      unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >>
> PAGE_SHIFT_4K;
> @@ -1856,7 +1857,7 @@ static int rmrr_identity_mapping(struct domain *d, 
> bool_t map,
> 
>      while ( base_pfn < end_pfn )
>      {
> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw);
> +        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> 
>          if ( err )
>              return err;
> @@ -1899,7 +1900,8 @@ static int intel_iommu_add_device(u8 devfn, struct 
> pci_dev
> *pdev)
>               PCI_BUS(bdf) == pdev->bus &&
>               PCI_DEVFN2(bdf) == devfn )
>          {
> -            ret = rmrr_identity_mapping(pdev->domain, 1, rmrr);
> +            ret = rmrr_identity_mapping(pdev->domain, 1, rmrr,
> +                                        XEN_DOMCTL_DEV_RDM_RELAXED);

Why did you hardcode relax policy here? Shouldn't the policy come
from hypercall flag?

>              if ( ret )
>                  dprintk(XENLOG_ERR VTDPREFIX, "d%d: RMRR mapping failed\n",
>                          pdev->domain->domain_id);
> @@ -1940,7 +1942,8 @@ static int intel_iommu_remove_device(u8 devfn, struct 
> pci_dev
> *pdev)
>               PCI_DEVFN2(bdf) != devfn )
>              continue;
> 
> -        rmrr_identity_mapping(pdev->domain, 0, rmrr);
> +        rmrr_identity_mapping(pdev->domain, 0, rmrr,
> +                              XEN_DOMCTL_DEV_RDM_RELAXED);

ditto

>      }
> 
>      return domain_context_unmap(pdev->domain, devfn, pdev);
> @@ -2098,7 +2101,7 @@ static void __hwdom_init setup_hwdom_rmrr(struct domain 
> *d)
>      spin_lock(&pcidevs_lock);
>      for_each_rmrr_device ( rmrr, bdf, i )
>      {
> -        ret = rmrr_identity_mapping(d, 1, rmrr);
> +        ret = rmrr_identity_mapping(d, 1, rmrr, XEN_DOMCTL_DEV_RDM_RELAXED);
>          if ( ret )
>              dprintk(XENLOG_ERR VTDPREFIX,
>                       "IOMMU: mapping reserved region failed\n");
> @@ -2241,7 +2244,8 @@ static int reassign_device_ownership(
>                   PCI_BUS(bdf) == pdev->bus &&
>                   PCI_DEVFN2(bdf) == devfn )
>              {
> -                ret = rmrr_identity_mapping(source, 0, rmrr);
> +                ret = rmrr_identity_mapping(source, 0, rmrr,
> +                                            XEN_DOMCTL_DEV_RDM_RELAXED);
>                  if ( ret != -ENOENT )
>                      return ret;
>              }
> @@ -2265,7 +2269,7 @@ static int reassign_device_ownership(
>  }
> 
>  static int intel_iommu_assign_device(
> -    struct domain *d, u8 devfn, struct pci_dev *pdev)
> +    struct domain *d, u8 devfn, struct pci_dev *pdev, u32 flag)
>  {
>      struct acpi_rmrr_unit *rmrr;
>      int ret = 0, i;
> @@ -2294,7 +2298,7 @@ static int intel_iommu_assign_device(
>               PCI_BUS(bdf) == bus &&
>               PCI_DEVFN2(bdf) == devfn )
>          {
> -            ret = rmrr_identity_mapping(d, 1, rmrr);
> +            ret = rmrr_identity_mapping(d, 1, rmrr, flag);
>              if ( ret )
>              {
>                  reassign_device_ownership(d, hardware_domain, devfn, pdev);
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 95b6266..a80b4f8 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -545,7 +545,7 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long 
> gfn,
> mfn_t mfn);
> 
>  /* Set identity addresses in the p2m table (for pass-through) */
>  int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> -                           p2m_access_t p2ma);
> +                           p2m_access_t p2ma, u32 flag);
> 
>  /* Add foreign mapping to the guest's p2m table. */
>  int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bc45ea5..2f9e40e 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -478,6 +478,11 @@ struct xen_domctl_assign_device {
>              XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node 
> */
>          } dt;
>      } u;
> +    /* IN */
> +#define XEN_DOMCTL_DEV_NO_RDM           0
> +#define XEN_DOMCTL_DEV_RDM_RELAXED      1
> +#define XEN_DOMCTL_DEV_RDM_STRICT       2

I don't understand why we require a NO_RDM flag. Whether there
is RDM associated with a given device, it's reported by system 
BIOS or through cmdline extension in coming patch. Why do we
require the hypercall to ask NO_RDM to hypervisor? The only flags
we want to pass to hypervisor is relaxed/strict policy, so hypervisor
know whether to fail or warn upon caught conflicts of identity
mapping...

> +    uint32_t  flag;   /* flag of assigned device */
>  };
>  typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t);
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index e2f584d..02b2b02 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -140,7 +140,7 @@ struct iommu_ops {
>      int (*add_device)(u8 devfn, device_t *dev);
>      int (*enable_device)(device_t *dev);
>      int (*remove_device)(u8 devfn, device_t *dev);
> -    int (*assign_device)(struct domain *, u8 devfn, device_t *dev);
> +    int (*assign_device)(struct domain *, u8 devfn, device_t *dev, u32 flag);
>      int (*reassign_device)(struct domain *s, struct domain *t,
>                             u8 devfn, device_t *dev);
>  #ifdef HAS_PCI
> --
> 1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.