Re: [Xen-devel] [PATCH v3 4/5] x86/HVM: fix pinned cache attribute handling

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, April 07, 2014 6:10 PM
> 
> - make sure UC- is only used for PAT purposes (MTRRs and hence EPT
>   don't have this type)
> - add order input to "get", and properly handle conflict case (forcing
>   an EPT page split)
> - properly detect (and refuse) overlaps during "set"
> - properly use RCU constructs
> - support deleting ranges through a special type input to "set"
> - set ignore-PAT flag in epte_get_entry_emt() when "get" succeeds
> - set "get" output to ~0 (invalid) rather than 0 (UC) on error (the
>   caller shouldn't be looking at it anyway)
> - move struct hvm_mem_pinned_cacheattr_range from header to C file
>   (used only there)
> 
> Note that the code (before and after this change) implies the GFN
> ranges passed to the hypercall to be inclusive, which is in contrast
> to the sole current user in qemu (all variants). It is not clear to me
> at which layer (qemu, libxc, hypervisor) this would best be fixed.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Tim Deegan <tim@xxxxxxx>

Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -551,6 +551,15 @@ bool_t mtrr_pat_not_equal(struct vcpu *v
>      return 0;
>  }
> 
> +struct hvm_mem_pinned_cacheattr_range {
> +    struct list_head list;
> +    uint64_t start, end;
> +    uint32_t type;
> +    struct rcu_head rcu;
> +};
> +
> +static DEFINE_RCU_READ_LOCK(pinned_cacheattr_rcu_lock);
> +
>  void hvm_init_cacheattr_region_list(
>      struct domain *d)
>  {
> @@ -573,30 +582,47 @@ void hvm_destroy_cacheattr_region_list(
>      }
>  }
> 
> -int32_t hvm_get_mem_pinned_cacheattr(
> +int hvm_get_mem_pinned_cacheattr(
>      struct domain *d,
>      uint64_t guest_fn,
> +    unsigned int order,
>      uint32_t *type)
>  {
>      struct hvm_mem_pinned_cacheattr_range *range;
> +    int rc = 0;
> 
> -    *type = 0;
> +    *type = ~0;
> 
>      if ( !is_hvm_domain(d) )
>          return 0;
> 
> +    rcu_read_lock(&pinned_cacheattr_rcu_lock);
>      list_for_each_entry_rcu ( range,
> 
> &d->arch.hvm_domain.pinned_cacheattr_ranges,
>                                list )
>      {
> -        if ( (guest_fn >= range->start) && (guest_fn <= range->end) )
> +        if ( (guest_fn >= range->start) &&
> +             (guest_fn + (1UL << order) - 1 <= range->end) )
>          {
>              *type = range->type;
> -            return 1;
> +            rc = 1;
> +            break;
> +        }
> +        if ( (guest_fn <= range->end) &&
> +             (range->start <= guest_fn + (1UL << order) - 1) )
> +        {
> +            rc = -1;
> +            break;
>          }
>      }
> +    rcu_read_unlock(&pinned_cacheattr_rcu_lock);
> 
> -    return 0;
> +    return rc;
> +}
> +
> +static void free_pinned_cacheattr_entry(struct rcu_head *rcu)
> +{
> +    xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range,
> rcu));
>  }
> 
>  int32_t hvm_set_mem_pinned_cacheattr(
> @@ -606,6 +632,28 @@ int32_t hvm_set_mem_pinned_cacheattr(
>      uint32_t  type)
>  {
>      struct hvm_mem_pinned_cacheattr_range *range;
> +    int rc = 1;
> +
> +    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
> +        return 0;
> +
> +    if ( type == XEN_DOMCTL_DELETE_MEM_CACHEATTR )
> +    {
> +        /* Remove the requested range. */
> +        rcu_read_lock(&pinned_cacheattr_rcu_lock);
> +        list_for_each_entry_rcu ( range,
> +
> &d->arch.hvm_domain.pinned_cacheattr_ranges,
> +                                  list )
> +            if ( range->start == gfn_start && range->end == gfn_end )
> +            {
> +                rcu_read_unlock(&pinned_cacheattr_rcu_lock);
> +                list_del_rcu(&range->list);
> +                call_rcu(&range->rcu, free_pinned_cacheattr_entry);
> +                return 0;
> +            }
> +        rcu_read_unlock(&pinned_cacheattr_rcu_lock);
> +        return -ENOENT;
> +    }
> 
>      if ( !((type == PAT_TYPE_UNCACHABLE) ||
>             (type == PAT_TYPE_WRCOMB) ||
> @@ -616,6 +664,27 @@ int32_t hvm_set_mem_pinned_cacheattr(
>           !is_hvm_domain(d) )
>          return -EINVAL;
> 
> +    rcu_read_lock(&pinned_cacheattr_rcu_lock);
> +    list_for_each_entry_rcu ( range,
> +
> &d->arch.hvm_domain.pinned_cacheattr_ranges,
> +                              list )
> +    {
> +        if ( range->start == gfn_start && range->end == gfn_end )
> +        {
> +            range->type = type;
> +            rc = 0;
> +            break;
> +        }
> +        if ( range->start <= gfn_end && gfn_start <= range->end )
> +        {
> +            rc = -EBUSY;
> +            break;
> +        }
> +    }
> +    rcu_read_unlock(&pinned_cacheattr_rcu_lock);
> +    if ( rc <= 0 )
> +        return rc;
> +
>      range = xzalloc(struct hvm_mem_pinned_cacheattr_range);
>      if ( range == NULL )
>          return -ENOMEM;
> @@ -732,8 +801,14 @@ int epte_get_entry_emt(struct domain *d,
>      if ( !mfn_valid(mfn_x(mfn)) )
>          return MTRR_TYPE_UNCACHABLE;
> 
> -    if ( hvm_get_mem_pinned_cacheattr(d, gfn, &type) )
> -        return type;
> +    switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) )
> +    {
> +    case 1:
> +        *ipat = 1;
> +        return type != PAT_TYPE_UC_MINUS ? type :
> PAT_TYPE_UNCACHABLE;
> +    case -1:
> +        return -1;
> +    }
> 
>      if ( !iommu_enabled ||
>           (rangeset_is_empty(d->iomem_caps) &&
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -601,7 +601,7 @@ _sh_propagate(struct vcpu *v,
>           * 3) if disables snoop control, compute the PAT index with
>           *    gMTRR and gPAT.
>           */
> -        if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), &type) )
> +        if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), 0,
> &type) )
>              sflags |= pat_type_2_pte_flags(type);
>          else if ( d->arch.hvm_domain.is_in_uc_mode )
>              sflags |= pat_type_2_pte_flags(PAT_TYPE_UNCACHABLE);
> --- a/xen/include/asm-x86/hvm/cacheattr.h
> +++ b/xen/include/asm-x86/hvm/cacheattr.h
> @@ -1,12 +1,6 @@
>  #ifndef __HVM_CACHEATTR_H__
>  #define __HVM_CACHEATTR_H__
> 
> -struct hvm_mem_pinned_cacheattr_range {
> -    struct list_head list;
> -    uint64_t start, end;
> -    uint32_t type;
> -};
> -
>  void hvm_init_cacheattr_region_list(
>      struct domain *d);
>  void hvm_destroy_cacheattr_region_list(
> @@ -15,11 +9,13 @@ void hvm_destroy_cacheattr_region_list(
>  /*
>   * To see guest_fn is in the pinned range or not,
>   * if yes, return 1, and set type to value in this range
> - * if no,  return 0, and set type to 0
> + * if no,  return 0, setting type to ~0
> + * if ambiguous, return -1, setting type to ~0 (possible only for order > 0)
>   */
> -int32_t hvm_get_mem_pinned_cacheattr(
> +int hvm_get_mem_pinned_cacheattr(
>      struct domain *d,
>      uint64_t guest_fn,
> +    unsigned int order,
>      uint32_t *type);
> 
> 
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -555,6 +555,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_iopor
>  #define XEN_DOMCTL_MEM_CACHEATTR_WP  5
>  #define XEN_DOMCTL_MEM_CACHEATTR_WB  6
>  #define XEN_DOMCTL_MEM_CACHEATTR_UCM 7
> +#define XEN_DOMCTL_DELETE_MEM_CACHEATTR (~(uint32_t)0)
>  struct xen_domctl_pin_mem_cacheattr {
>      uint64_aligned_t start, end;
>      uint32_t type; /* XEN_DOMCTL_MEM_CACHEATTR_* */
> 


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