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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/07/2014 07:03 PM, Tamas Lengyel wrote:

On Fri, Aug 8, 2014 at 12:58 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 07/08/2014 22:53, Tamas Lengyel wrote:
> On Thu, Aug 7, 2014 at 11:39 PM, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx
>> 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@xxxxxxxxxxxx>
>>> ---
>>>   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.

I think these are small enough fixes that can be made part of this series.

Otherwise we are going to add new interface that we know at the time of commit is not used properly (the fact that the called code does not use the information is not important IMO, as long as there are no negative consequences).

(I am also curious why we are truncating exitinfo1 to 32 bits and yet compiler doesn't warn.)

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel