Re: [V10 PATCH 04/23] PVH xen: Move e820 fields out of pv_domain struct

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/07/13 02:59, Mukesh Rathor wrote:
> This patch moves fields out of the pv_domain struct as they are used by
> PVH also.
>
> Changes in V6:
>   - Don't base on guest type the initialization and cleanup.
>
> Changes in V7:
>   - If statement doesn't need to be split across lines anymore.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/domain.c        |   10 ++++------
>  xen/arch/x86/mm.c            |   26 ++++++++++++--------------
>  xen/include/asm-x86/domain.h |   10 +++++-----
>  3 files changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5de5e49..c361abf 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -553,6 +553,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>          if ( (rc = iommu_domain_init(d)) != 0 )
>              goto fail;
>      }
> +    spin_lock_init(&d->arch.e820_lock);
>  
>      if ( is_hvm_domain(d) )
>      {
> @@ -563,13 +564,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>          }
>      }
>      else
> -    {
>          /* 64-bit PV guest by default. */
>          d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>  
> -        spin_lock_init(&d->arch.pv_domain.e820_lock);
> -    }
> -
>      /* initialize default tsc behavior in case tools don't */
>      tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
>      spin_lock_init(&d->arch.vtsc_lock);
> @@ -592,8 +589,9 @@ void arch_domain_destroy(struct domain *d)
>  {
>      if ( is_hvm_domain(d) )
>          hvm_domain_destroy(d);
> -    else
> -        xfree(d->arch.pv_domain.e820);
> +
> +    if ( d->arch.e820 )
> +        xfree(d->arch.e820);

xfree() works correctly wrt null pointers; remove the if().  You appear
to have used in the next hunk.

~Andrew

>  
>      free_domain_pirqs(d);
>      if ( !is_idle_domain(d) )
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index c00841c..412971e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4763,11 +4763,11 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>          }
>  
> -        spin_lock(&d->arch.pv_domain.e820_lock);
> -        xfree(d->arch.pv_domain.e820);
> -        d->arch.pv_domain.e820 = e820;
> -        d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
> -        spin_unlock(&d->arch.pv_domain.e820_lock);
> +        spin_lock(&d->arch.e820_lock);
> +        xfree(d->arch.e820);
> +        d->arch.e820 = e820;
> +        d->arch.nr_e820 = fmap.map.nr_entries;
> +        spin_unlock(&d->arch.e820_lock);
>  
>          rcu_unlock_domain(d);
>          return rc;
> @@ -4781,26 +4781,24 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&map, arg, 1) )
>              return -EFAULT;
>  
> -        spin_lock(&d->arch.pv_domain.e820_lock);
> +        spin_lock(&d->arch.e820_lock);
>  
>          /* Backwards compatibility. */
> -        if ( (d->arch.pv_domain.nr_e820 == 0) ||
> -             (d->arch.pv_domain.e820 == NULL) )
> +        if ( (d->arch.nr_e820 == 0) || (d->arch.e820 == NULL) )
>          {
> -            spin_unlock(&d->arch.pv_domain.e820_lock);
> +            spin_unlock(&d->arch.e820_lock);
>              return -ENOSYS;
>          }
>  
> -        map.nr_entries = min(map.nr_entries, d->arch.pv_domain.nr_e820);
> -        if ( copy_to_guest(map.buffer, d->arch.pv_domain.e820,
> -                           map.nr_entries) ||
> +        map.nr_entries = min(map.nr_entries, d->arch.nr_e820);
> +        if ( copy_to_guest(map.buffer, d->arch.e820, map.nr_entries) ||
>               __copy_to_guest(arg, &map, 1) )
>          {
> -            spin_unlock(&d->arch.pv_domain.e820_lock);
> +            spin_unlock(&d->arch.e820_lock);
>              return -EFAULT;
>          }
>  
> -        spin_unlock(&d->arch.pv_domain.e820_lock);
> +        spin_unlock(&d->arch.e820_lock);
>          return 0;
>      }
>  
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index d79464d..c3f9f8e 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -234,11 +234,6 @@ struct pv_domain
>  
>      /* map_domain_page() mapping cache. */
>      struct mapcache_domain mapcache;
> -
> -    /* Pseudophysical e820 map (XENMEM_memory_map).  */
> -    spinlock_t e820_lock;
> -    struct e820entry *e820;
> -    unsigned int nr_e820;
>  };
>  
>  struct arch_domain
> @@ -313,6 +308,11 @@ struct arch_domain
>                                  (possibly other cases in the future */
>      uint64_t vtsc_kerncount; /* for hvm, counts all vtsc */
>      uint64_t vtsc_usercount; /* not used for hvm */
> +
> +    /* Pseudophysical e820 map (XENMEM_memory_map).  */
> +    spinlock_t e820_lock;
> +    struct e820entry *e820;
> +    unsigned int nr_e820;
>  } __cacheline_aligned;
>  
>  #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))