Re: [Xen-devel] [PATCH v2] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.18 at 10:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 9/5/18 9:56 AM, Jan Beulich wrote:
>>>>> On 04.09.18 at 22:58, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 9/4/18 11:40 PM, Tamas K Lengyel wrote:
>>>> On Mon, Sep 3, 2018 at 10:59 PM Adrian Pop <apop@xxxxxxxxxxxxxxx> wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -4533,8 +4533,7 @@ static int do_altp2m_op(
>>>>>          return -EOPNOTSUPP;
>>>>>      }
>>>>>
>>>>> -    d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>>> -        rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>>>>> +    d = rcu_lock_domain_by_any_id(a.domain);
>>>>
>>>> Does rcu_lock_domain_by_any_id work if its from the current domain? If
>>>> not, doesn't that change this function's accessibility to be from
>>>> exclusively usable only by the outside agent?
>>> The code says it should be safe:
>>>
>>>  633 struct domain *rcu_lock_domain_by_any_id(domid_t dom)
>>>  634 {
>>>  635     if ( dom == DOMID_SELF )
>>>  636         return rcu_lock_current_domain();
>>>  637     return rcu_lock_domain_by_id(dom);
>>>  638 }
>>>
>>> as long as dom == DOMID_SELF. I think the old behaviour assumed that
>>> HVMOP_altp2m_vcpu_enable_notify alone would only ever be used from the
>>> current domain, and this change expands its usability (Adrian should
>>> correct me if I'm wrong here).
>> 
>> But a guest exposed interface can't be changed like this: If a.domain
>> was ignored for this sub-op before, it needs to continue to be ignored.
> 
> Also, technically speaking a.domain is not currently ignored - it's just
> checked against DOMID_SELF:
> 
> 4596     case HVMOP_altp2m_vcpu_enable_notify:
> 4597     {
> 4598         struct vcpu *curr = current;
> 4599         p2m_type_t p2mt;
> 4600
> 4601         if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> 4602              a.u.enable_notify.vcpu_id != curr->vcpu_id )
> 4603         {
> 4604             rc = -EINVAL;
> 4605             break;
> 4606         }

Ah, yes, I think that's fine then.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel