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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.18 at 13:26, <george.dunlap@xxxxxxxxxx> wrote:
> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>> On 23.07.18 at 15:48, <aisaila@xxxxxxxxxxxxxxx> 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.

Jan



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