Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
[...]
> > @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
> >      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
> >  }
> >  
> > -int32_t hvm_set_mem_pinned_cacheattr(
> > -    struct domain *d,
> > -    uint64_t gfn_start,
> > -    uint64_t gfn_end,
> > -    uint32_t  type)
> > +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
> > +                                 uint64_t gfn_end, 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 ( !is_hvm_domain(d) )
> > +        return -EOPNOTSUPP;
> 
> You introduce an asymmetry between set and get here, both in terms of
> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
> this?
> 
> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
> 

I don't think we can have ASSERT() in the set function because it might
be called by untrusted entity. On the other hand, the get function can
only be used by hypervisor so the ASSERT should be fine.

Wei.

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