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

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

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."

[snip]
>> +        {
>> +            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?

 -George

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