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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.14 at 13:42, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> @@ -1409,6 +1409,11 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>          .insn_fetch = !!(pfec & PFEC_insn_fetch)
>      };
>  
> +    if ( pfec & NPT_FAULT_WITH_GLA )
> +        npfec.extra_fault_info |= extra_fault_info_with_gla;
> +    if ( pfec & NPT_FAULT_IN_GPT )
> +        npfec.extra_fault_info |= extra_fault_info_in_gpt;
> +

See below - these mask like operations aren't valid.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2379,6 +2379,10 @@ static void ept_handle_violation(unsigned long 
> qualification, paddr_t gpa)
>      {
>          __vmread(GUEST_LINEAR_ADDRESS, &gla);
>          npfec.gla_valid = 1;
> +        if( qualification & EPT_GLA_FAULT )
> +            npfec.extra_fault_info |= extra_fault_info_with_gla;
> +        else
> +            npfec.extra_fault_info |= extra_fault_info_in_gpt;

Same here.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1405,6 +1405,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long 
> gla,
>          req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
>          req->gla_valid = check.gla_valid;
>          req->gla = gla;
> +        if ( check.extra_fault_info & extra_fault_info_with_gla )
> +            req->fault_with_gla = 1;
> +        if ( check.extra_fault_info & extra_fault_info_in_gpt )
> +            req->fault_in_gpt = 1;

And again - these masking operations aren't valid.

> --- a/xen/include/asm-x86/hvm/svm/svm.h
> +++ b/xen/include/asm-x86/hvm/svm/svm.h
> @@ -105,4 +105,10 @@ extern u32 svm_feature_flags;
>  extern void svm_host_osvw_reset(void);
>  extern void svm_host_osvw_init(void);
>  
> +/* EXITINFO1 fields on NPT faults */
> +#define _NPT_FAULT_WITH_GLA     32
> +#define NPT_FAULT_WITH_GLA      (1UL<<_NPT_FAULT_WITH_GLA)
> +#define _NPT_FAULT_IN_GPT       33
> +#define NPT_FAULT_IN_GPT        (1UL<<_NPT_FAULT_IN_GPT)

To be able to associate them, they should also include PFEC_; to
indicate they're NPT-specific this could then be NPT_PFEC_* or
PFEC_NPT_*

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -552,6 +552,16 @@ void audit_domains(void);
>  #endif
>  
>  /*
> + * Extra fault info types which are used to further describe
> + * the source of a mem access violation.
> + */
> +typedef enum {
> +    extra_fault_info_unavailable    = 0u,
> +    extra_fault_info_in_gpt         = ( 1u << 0 ),
> +    extra_fault_info_with_gla       = ( 1u << 1 ),
> +} extra_fault_info_t;

This gives the impression of this still being a flag mask, whereas this
really is a enumeration (thus properly expressing that the states
cannot be combined):

+enum npfec_kind {
+    npfec_kind_unknown, /* must be first */
+    npfec_kind_in_gpt,
+    npfec_kind_gla,
+} extra_fault_info_t;

Jan


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