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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5011 bytes --]

On 08/07/2014 07:03 PM, Tamas Lengyel wrote:
>
>
>
> On Fri, Aug 8, 2014 at 12:58 AM, Andrew Cooper 
> <andrew.cooper3@citrix.com <mailto: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 <mailto: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
>     <mailto: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.

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


[-- Attachment #1.2: Type: text/html, Size: 10669 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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