Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap <george.dunlap@citrix.com>]

On 07/24/2018 01:02 PM, Jan Beulich wrote:
>>>> On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
>> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -221,12 +221,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long 
>> gla,
>>>>          {
>>>>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>>>              req->u.mem_access.gla = gla;
>>>> -
>>>> -            if ( npfec.kind == npfec_kind_with_gla )
>>>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>>>> -            else if ( npfec.kind == npfec_kind_in_gpt )
>>>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>>>>          }
>>>> +
>>>> +        if ( npfec.kind == npfec_kind_with_gla )
>>>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>>>> +        else if ( npfec.kind == npfec_kind_in_gpt )
>>>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>>>
>>> Without explanation in the commit message and without comment
>>> this change is a no-go imo: I consider it at least questionable to
>>> have npfec_kind_with_gla without .gla_valid set to true. Perhaps
>>> it's just a naming issue, but that would then still require half a
>>> sentence to explain.
>>
>> The naming here is confusing, but it seems to be based on the AMD manual
>> naming convention (IIRC from my skim through the manual last week).
>> "With gla" in this context means, "The nested fault happend while trying
>> to translate the final guest linear address of the access", as opposed
>> to "The nested fault happend while trying to translate one of the page
>> tables, before the guest linear address for the virtual address could be
>> calculated."  It's a perfectly valid setting on AMD box, in spite of the
>> fact that AMD doesn't report the virt -> gla translation.
>>
>> I'd be in favor of renaming the variable, but that shouldn't be
>> Alexandru's job.
>>
>> What about adding a comment like this:
>>
>> "Naming is confusing here; 'with_gla' simply means the fault happened as
>> the result of a translating the final gla, as opposed to translating one
>> of the pagetables."
> 
> Yes, that would clarify thnigs enough, I think.
> 
>>>> +        {
>>>> +            xfree(d->arch.monitor.msr_bitmap);
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +        radix_tree_init(p2m->mem_access_settings);
>>>> +    }
>>>
>>> What's the SVM connection here? Please don't forget that p2m-pt.c
>>> also serves the shadow case. Perhaps struct p2m_domain should
>>> contain a boolean indicator whether this auxiliary data structure is
>>> needed?
>>
>> It's basically just "hap_enabled()" isn't it?
> 
> Only if we can't make it there when EPT is active.

It can make it here when VMX is active and shadow is enabled, but it
shouldn't be able to get here when EPT is active.  We could add an
ASSERT() to that effect; it should be safe in production, as the only
side effect should be that we do a small pointless allocation.

 -George

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