Re: [PATCH v2 1/2] x86/mem_event: Deliver gla fault EPT violation information

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/08/2014 00:03, Tamas Lengyel wrote:
> On Fri, Aug 8, 2014 at 12:58 AM, Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
>
>> On 07/08/2014 22:53, Tamas Lengyel wrote:
>>> On Thu, Aug 7, 2014 at 11:39 PM, Boris Ostrovsky <
>> boris.ostrovsky@oracle.com
>>>> wrote:
>>>> On 08/07/2014 03:47 PM, Tamas K Lengyel wrote:
>>>>
>>>>> On Intel EPT the exit qualification generated by a violation also
>>>>> includes a bit (EPT_GLA_FAULT) which describes the following
>> information:
>>>>> Set if the access causing the EPT violation is to a guest-physical
>>>>> address that is the translation of a linear address. Clear if the
>> access
>>>>> causing the EPT violation is to a paging-structure entry as part of a
>> page
>>>>> walk or the update of an accessed or dirty bit.
>>>>>
>>>>> For more information see Table 27-7 in the Intel SDM.
>>>>>
>>>>> This patch extends the mem_event system to deliver this extra
>>>>> information, which could be useful for determining the cause of a
>> violation.
>>>>> v2: Split gla_fault into fault_in_gpt and fault_gla to be more
>> compatible
>>>>> with the AMD implementation.
>>>>>
>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>>>>> ---
>>>>>   xen/arch/x86/hvm/hvm.c         |  8 ++++++--
>>>>>   xen/arch/x86/hvm/svm/svm.c     |  2 +-
>>>>>   xen/arch/x86/hvm/vmx/vmx.c     | 23 ++++++++++++++++++++++-
>>>>>   xen/arch/x86/mm/p2m.c          |  5 ++++-
>>>>>   xen/include/asm-x86/hvm/hvm.h  |  5 ++++-
>>>>>   xen/include/asm-x86/p2m.h      |  3 ++-
>>>>>   xen/include/public/mem_event.h |  4 +++-
>>>>>   7 files changed, 42 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>> index e834406..d7b5e2b 100644
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -2725,6 +2725,8 @@ void hvm_inject_page_fault(int errcode, unsigned
>>>>> long cr2)
>>>>>   int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>>                                 bool_t gla_valid,
>>>>>                                 unsigned long gla,
>>>>> +                              bool_t fault_in_gpt,
>>>>> +                              bool_t fault_gla,
>>>>>                                 bool_t access_r,
>>>>>                                 bool_t access_w,
>>>>>                                 bool_t access_x)
>>>>> @@ -2832,8 +2834,10 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>>             if ( violation )
>>>>>           {
>>>>> -            if ( p2m_mem_access_check(gpa, gla_valid, gla, access_r,
>>>>> -                                        access_w, access_x, &req_ptr)
>> )
>>>>> +            if ( p2m_mem_access_check(gpa, gla_valid, gla,
>>>>> +                                        fault_in_gpt, fault_gla,
>>>>> +                                        access_r, access_w, access_x,
>>>>> +                                        &req_ptr) )
>>>>>               {
>>>>>                   fall_through = 1;
>>>>>               } else {
>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>>> index 76616ac..9e35e7a 100644
>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>> @@ -1403,7 +1403,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>>>>>       p2m_access_t p2ma;
>>>>>       struct p2m_domain *p2m = NULL;
>>>>>   -    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul,
>>>>> +    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul, 0, 0,
>>>>>
>>>> Why not pass the actual bits that the HW provides?
>>>>
>>> The actual bits could be passed but it makes no difference at this point
>>> since the AMD side isn't setup to work with mem_event. When it is
>>> integrated, those bits could and should be passed accordingly.
>>>
>>> Tamas
>> There is a lot more than mem_event which might want these bits from AMD.
>>
>> If the bits are easily available at this point, you should fill them in.
>>
>> ~Andrew
>>
> I checked and there are no typedefs for these bits in the headers. Also,
> the EXITINFO1 passed here is truncated to 32-bits and that would need to be
> fixed.. so there are more then one issue that would have to be addressed
> for this. I think it would justify a separate patch of its own when it is
> actually needed.
>
> Tamas
>

That seems fair enough (although the final call is up to the AMD
maintainers).

~Andrew