Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types

[George Dunlap <george.dunlap@eu.citrix.com>]

On 22/11/13 01:00, Mukesh Rathor wrote:
> For pvh guests, epte_get_entry_emt() is incorrectly returning WB for
> all mem types because of the following check:
>      if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> Skip the check for pvh guests.
>
> Also note, MTRR ranges are not maintained for pvh, and a solution is
> being contrived using PAT.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>   xen/arch/x86/hvm/mtrr.c |    7 ++++++-
>   1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 4ff1e55..5427e1c 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -693,7 +693,8 @@ uint8_t epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
>            ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
>           return MTRR_TYPE_WRBACK;
>   
> -    if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> +    if ( !is_pvh_vcpu(v) &&
> +         !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
>           return MTRR_TYPE_WRBACK;
>   
>       if ( !mfn_valid(mfn_x(mfn)) )
> @@ -717,6 +718,10 @@ uint8_t epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
>           return MTRR_TYPE_WRBACK;
>       }
>   
> +    /* MTRR ranges are not maintained for pvh. */
> +    if ( is_pvh_vcpu(v) )
> +        return MTRR_TYPE_WRBACK;
> +
>       gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT));
>       hmtrr_mtype = get_mtrr_type(&mtrr_state, (mfn_x(mfn) << PAGE_SHIFT));
>       return ((gmtrr_mtype <= hmtrr_mtype) ? gmtrr_mtype : hmtrr_mtype);

So this will bypass the host mtrr settings, and always return WRBACK, 
even if in mtrr_state it was set to something lower. Presumably that 
"min(host,guest)" was there for a reason.  Are you sure it's safe to 
just ignore it?

This is why I suggested the following instead:

gmtrr_mtype = is_hvm_domain(v) ?
         get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT)) :
         MTRR_TYPE_WRBACK;

  -George