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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.03.16 at 13:12, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/03/16 12:10, Wei Liu wrote:
>> 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.
> 
> The hypercall handler should sanitise the untrusted caller before we get
> into this function.

I don't think this would be a good idea: Once we extend this to
also allow such ranges for PVH, keeping the change in policy to
the source file / function actually carrying this out seems quite
a bit more logical.

Jan


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