* [PATCH] pvh: change epte_get_entry_emt() for pvh mem types
@ 2013-11-22 1:00 Mukesh Rathor
2013-11-22 10:52 ` George Dunlap
0 siblings, 1 reply; 5+ messages in thread
From: Mukesh Rathor @ 2013-11-22 1:00 UTC (permalink / raw)
To: Xen-devel@lists.xensource.com
Cc: Jan Beulich, George Dunlap, eddie.dong, Keir Fraser,
Nakajima, Jun, Roger Pau Monne
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types
2013-11-22 1:00 [PATCH] pvh: change epte_get_entry_emt() for pvh mem types Mukesh Rathor
@ 2013-11-22 10:52 ` George Dunlap
2013-11-22 11:03 ` George Dunlap
2013-11-22 12:05 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: George Dunlap @ 2013-11-22 10:52 UTC (permalink / raw)
To: Mukesh Rathor, Xen-devel@lists.xensource.com
Cc: Keir Fraser, eddie.dong, Nakajima, Jun, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types
2013-11-22 10:52 ` George Dunlap
@ 2013-11-22 11:03 ` George Dunlap
2013-11-22 12:05 ` Jan Beulich
1 sibling, 0 replies; 5+ messages in thread
From: George Dunlap @ 2013-11-22 11:03 UTC (permalink / raw)
To: Mukesh Rathor, Xen-devel@lists.xensource.com
Cc: Keir Fraser, eddie.dong, Nakajima, Jun, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types
2013-11-22 10:52 ` George Dunlap
2013-11-22 11:03 ` George Dunlap
@ 2013-11-22 12:05 ` Jan Beulich
2013-11-22 20:40 ` Mukesh Rathor
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2013-11-22 12:05 UTC (permalink / raw)
To: George Dunlap, Xen-devel@lists.xensource.com, Mukesh Rathor
Cc: Keir Fraser, eddie.dong, Jun Nakajima, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pvh: change epte_get_entry_emt() for pvh mem types
2013-11-22 12:05 ` Jan Beulich
@ 2013-11-22 20:40 ` Mukesh Rathor
0 siblings, 0 replies; 5+ messages in thread
From: Mukesh Rathor @ 2013-11-22 20:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel@lists.xensource.com, George Dunlap, eddie.dong,
Keir Fraser, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-22 20:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-22 1:00 [PATCH] pvh: change epte_get_entry_emt() for pvh mem types Mukesh Rathor
2013-11-22 10:52 ` George Dunlap
2013-11-22 11:03 ` George Dunlap
2013-11-22 12:05 ` Jan Beulich
2013-11-22 20:40 ` Mukesh Rathor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).