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

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

[Mukesh Rathor]

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);
-- 
1.7.2.3

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

[George Dunlap]

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

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

[George Dunlap]

On 22/11/13 10:52, George Dunlap wrote:
> 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;

Also, while I suppose this is fine for now (since we're starting the 
code freeze), for the future:  is it actually better to "bake in" that 
MTRRs are completely disabled and always effectively return WRBACK 
(except for direct_mmio memory), or would it be better to initialize the 
virtual MTRRs to WRBACK (the way a BIOS would), and allow the guest to 
change them if it wants?

  -George

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

[Jan Beulich]

>>> On 22.11.13 at 11:52, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> 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;

Indeed, that's what we should use.

Jan

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

[Mukesh Rathor]

On Fri, 22 Nov 2013 12:05:03 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.11.13 at 11:52, George Dunlap <george.dunlap@eu.citrix.com>
> >>> wrote:
> > 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;
> 
> Indeed, that's what we should use.

Sorry, totally my bad. For some reason I had concluded that hmtrr_mtype
was being set to UC. V2 patch on the way...

thanks
Mukesh