Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> wrote:
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -22,27 +22,66 @@ struct get_reserved_device_memory {
>      unsigned int used_entries;
>  };
>  
> -static int get_reserved_device_memory(xen_pfn_t start,
> -                                      xen_ulong_t nr, void *ctxt)
> +static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
> +                                      u32 id, void *ctxt)
>  {
>      struct get_reserved_device_memory *grdm = ctxt;
> +    struct domain *d;
> +    unsigned int i;
> +    u32 sbdf;
> +    struct compat_reserved_device_memory rdm = {
> +        .start_pfn = start, .nr_pages = nr
> +    };
>  
> -    if ( grdm->used_entries < grdm->map.nr_entries )
> -    {
> -        struct compat_reserved_device_memory rdm = {
> -            .start_pfn = start, .nr_pages = nr
> -        };
> +    if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> +        return -ERANGE;
>  
> -        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> -            return -ERANGE;
> +    d = rcu_lock_domain_by_any_id(grdm->map.domid);
> +    if ( d == NULL )
> +        return -ESRCH;

So why are you doing this in the call back (potentially many times)
instead of just once in compat_memory_op(), storing the pointer in
the context structure?

>  
> -        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
> -                                     &rdm, 1) )
> -            return -EFAULT;
> +    if ( d )
> +    {
> +        if ( d->arch.hvm_domain.pci_force )

You didn't verify that the domain is a HVM/PVH one.

> +        {
> +            if ( grdm->used_entries < grdm->map.nr_entries )
> +            {
> +                if ( __copy_to_compat_offset(grdm->map.buffer,
> +                                             grdm->used_entries,
> +                                             &rdm, 1) )
> +                {
> +                    rcu_unlock_domain(d);
> +                    return -EFAULT;
> +                }
> +            }
> +            ++grdm->used_entries;
> +        }
> +        else
> +        {
> +            for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
> +            {
> +                sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
> +                                 d->arch.hvm_domain.pcidevs[i].bus,
> +                                 d->arch.hvm_domain.pcidevs[i].devfn);
> +                if ( sbdf == id )
> +                {
> +                    if ( grdm->used_entries < grdm->map.nr_entries )
> +                    {
> +                        if ( __copy_to_compat_offset(grdm->map.buffer,
> +                                                     grdm->used_entries,
> +                                                     &rdm, 1) )
> +                        {
> +                            rcu_unlock_domain(d);
> +                            return -EFAULT;
> +                        }
> +                    }
> +                    ++grdm->used_entries;

break;

Also trying to fold code identical on the if and else branches would
seem pretty desirable.

> @@ -319,9 +358,13 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>  
>              if ( !rc && grdm.map.nr_entries < grdm.used_entries )
>                  rc = -ENOBUFS;
> +
>              grdm.map.nr_entries = grdm.used_entries;
> -            if ( __copy_to_guest(compat, &grdm.map, 1) )
> -                rc = -EFAULT;
> +            if ( grdm.map.nr_entries )
> +            {
> +                if ( __copy_to_guest(compat, &grdm.map, 1) )
> +                    rc = -EFAULT;
> +            }

Why do you need this change?

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -904,17 +904,33 @@ int platform_supports_x2apic(void)
>  
>  int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
>  {
> -    struct acpi_rmrr_unit *rmrr;
> +    struct acpi_rmrr_unit *rmrr, *rmrr_cur = NULL;
>      int rc = 0;
> +    unsigned int i;
> +    u16 bdf;
>  
> -    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> +    for_each_rmrr_device ( rmrr, bdf, i )
>      {
> -        rc = func(PFN_DOWN(rmrr->base_address),
> -                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
> -                  ctxt);
> -        if ( rc )
> -            break;
> +        if ( rmrr != rmrr_cur )
> +        {
> +            rc = func(PFN_DOWN(rmrr->base_address),
> +                      PFN_UP(rmrr->end_address) -
> +                        PFN_DOWN(rmrr->base_address),
> +                      PCI_SBDF(rmrr->segment, bdf),
> +                      ctxt);
> +
> +            if ( unlikely(rc < 0) )
> +                return rc;
> +
> +            /* Just go next. */
> +            if ( !rc )
> +                rmrr_cur = rmrr;
> +
> +            /* Now just return specific to user requirement. */
> +            if ( rc > 0 )
> +                return rc;

Nice that you check for that, but I can't see this case occurring
anymore. Did you lose some code? Also please don't write code
more complicated than necessary. The above two if()s could be


+            if ( rc > 0 )
+                return rc;
+
+            rmrr_cur = rmrr;

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -586,6 +586,11 @@ typedef struct xen_reserved_device_memory 
> xen_reserved_device_memory_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
>  
>  struct xen_reserved_device_memory_map {
> +    /*
> +     * Domain whose reservation is being changed.
> +     * Unprivileged domains can specify only DOMID_SELF.
> +     */
> +    domid_t        domid;
>      /* IN/OUT */
>      unsigned int nr_entries;
>      /* OUT */

Your addition lacks an IN annotation.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -31,6 +31,8 @@
>  #define PCI_DEVFN2(bdf) ((bdf) & 0xff)
>  #define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
>  #define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
> +#define PCI_SBDF(s,bdf) (((s & 0xffff) << 16) | (bdf & 0xffff))
> +#define PCI_SBDF2(s,b,df) (((s & 0xffff) << 16) | PCI_BDF2(b,df))

Missing several parentheses.

Jan

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