On 07/19/2018 09:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>> om> wrote:
>>>
>>> From: Isaila Alexandru <aisa...@bitdefender.com>
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete. Something like
>> this:
>>
>> ---
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT
>> p2m table, so we add a radix tree to store the rights. For
>> efficiency, remove entries which match the default permissions rather
>> than continuing to store them.
>>
>> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
>> an access, and removing / adding RW or NX flags as appropriate.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>> - Delete blak line
>>> - Add return if p2m_access_rwx = a
>>> - Delete the comment from p2m_pt_get_entry()
>>> - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c | 3 ++
>>> xen/arch/x86/mm/p2m-pt.c | 109
>>> ++++++++++++++++++++++++++++++++++-----
>>> xen/arch/x86/mm/p2m.c | 6 +++
>>> xen/arch/x86/monitor.c | 13 +++++
>>> xen/include/asm-x86/mem_access.h | 2 +-
>>> xen/include/asm-x86/p2m.h | 6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
>>> + {
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
>
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.
Right -- then really this check was always a VMX-ism, and should really
have been:
/* VMX can only tell the fault type if gla_valid is set */
if ( !(cpu_has_vmx && !npfec.gla_valid) ) {
...
But in any case, if cpu_has_vmx && !npfec.gla_valid, then npfec.kind
should be npfec_kind_unknown (==0); so is there any reason not to read
npfec.kind here unconditionally? i.e., like below?
if ( npfec.gla_valid )
{
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;
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
