mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
@ 2010-09-28 13:19 Sander Eikelenboom
  0 siblings, 0 replies; 14+ messages in thread
From: Sander Eikelenboom @ 2010-09-28 13:19 UTC (permalink / raw)
  To: mingo, Jeremy Fitzhardinge, Stefano Stabellini
  Cc: linux-kernel, xen-devel, x86

>* stefano.stabellini@xxxxxxxxxxxxx <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>
>> From: Stephen Tweedie <sct@xxxxxxxxxx>
>> 
>> Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
>> allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
>> querying the hypervisor itself.)
>> 
>> [ Impact: add basic MTRR support ]
>> 
>> Signed-off-by: Stephen Tweedie <sct@xxxxxxxxxx>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>> ---
>> arch/x86/kernel/cpu/mtrr/Makefile | 2 +-
>> arch/x86/kernel/cpu/mtrr/main.c | 3 +
>> arch/x86/kernel/cpu/mtrr/mtrr.h | 7 ++
>> arch/x86/kernel/cpu/mtrr/xen.c | 110 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 121 insertions(+), 1 deletions(-)
> create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c
>
>Still NAK, for the very same reasons as we NAK-ed it the previous time: 
>/proc/mtrr is a problematic and complicated legacy interface that should 
>die. Any modern X server will do the right thing via PAT.
>
>Also, please get the Ack of at least one x86 maintainer for x86 patches.
>
>Thanks,
>
>Ingo
>--
>

I can't find the MTRR API to be officially deprecated with any schedule for removal (at least i couldn't find it in feature-removal-schedule.txt).
KVM has had patches for MTRR as well, if it's so deprecated .. why has it ? .. legacy support for a not officially deprecated API perhaps ? To support CPU's that support MTRR but not PAT ?

If you think the MTRR interface should die, and should die fast, why hasn't it been added to feature-removal-schedule.txt for a complete removal in a not so distant future, since this is the only way to force all users to switch to PAT.

Perhaps it would be fair to ask for a working PAT implementation besides MTRR, but since it is not officially deprecated completely objecting a MTRR one just for Xen seems a bit unfair to me.

--
Sander


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 18:58                   ` Jeremy Fitzhardinge
@ 2010-09-28 19:05                     ` H. Peter Anvin
  0 siblings, 0 replies; 14+ messages in thread
From: H. Peter Anvin @ 2010-09-28 19:05 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

On 09/28/2010 11:58 AM, Jeremy Fitzhardinge wrote:
>  On 09/28/2010 11:46 AM, H. Peter Anvin wrote:
>> Well, we're specifically talking about a virtual machine which has
>> direct access to hardware, so it is concerned about the real physical
>> memory properties of real physical pages.  If we can assume that
>> BIOS/Xen will always set up MTRR correctly then there shouldn't be any
>> need for the kernel to modify the MTRR itself.  How true is that in
>> general?  I don't know, but if we could rely on BIOS then there'd never
>> be a need to touch MTRR, would there?
>> Well, in the past MTRRs were abused for device properties mainly by the
>> X server, but other than that, no, not really.  The other thing we do is
>> the MTRR cleanup (which doesn't involve /proc/mtrr) to deal with
>> brokenness in the BIOS setup, but that really belongs in the hypervisor
>> in your case since it fundamentally affects how memory is handled.
> 
> Yeah, the hypervisor should definitely deal with that.  I have no
> problem in principle with leaving MTRRs entirely to Xen, but I was just
> concerned about possible repercussions.  Certainly when I first did this
> work, I was using Fedora 8 whose X server did depend on /proc/mtrr for
> good performance.
> 

Yeah, that should all be fixed now.

	-hpa

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 18:46                 ` H. Peter Anvin
@ 2010-09-28 18:58                   ` Jeremy Fitzhardinge
  2010-09-28 19:05                     ` H. Peter Anvin
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Fitzhardinge @ 2010-09-28 18:58 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

 On 09/28/2010 11:46 AM, H. Peter Anvin wrote:
> Well, we're specifically talking about a virtual machine which has
> direct access to hardware, so it is concerned about the real physical
> memory properties of real physical pages.  If we can assume that
> BIOS/Xen will always set up MTRR correctly then there shouldn't be any
> need for the kernel to modify the MTRR itself.  How true is that in
> general?  I don't know, but if we could rely on BIOS then there'd never
> be a need to touch MTRR, would there?
> Well, in the past MTRRs were abused for device properties mainly by the
> X server, but other than that, no, not really.  The other thing we do is
> the MTRR cleanup (which doesn't involve /proc/mtrr) to deal with
> brokenness in the BIOS setup, but that really belongs in the hypervisor
> in your case since it fundamentally affects how memory is handled.

Yeah, the hypervisor should definitely deal with that.  I have no
problem in principle with leaving MTRRs entirely to Xen, but I was just
concerned about possible repercussions.  Certainly when I first did this
work, I was using Fedora 8 whose X server did depend on /proc/mtrr for
good performance.

    J

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 18:24               ` Jeremy Fitzhardinge
@ 2010-09-28 18:46                 ` H. Peter Anvin
  2010-09-28 18:58                   ` Jeremy Fitzhardinge
  0 siblings, 1 reply; 14+ messages in thread
From: H. Peter Anvin @ 2010-09-28 18:46 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

On 09/28/2010 11:24 AM, Jeremy Fitzhardinge wrote:
>>>
>> No, and we really can't do it for a couple of reasons:
>>
>> a) Pre-PAT hardware;
>> b) MTRRs and PAT interact on hardware;
>> c) MTRRs, but not PAT, interact with SMM.
> 
> What about pre-PAT software (ie, X servers which still use /proc/mtrr)?
> 

Fortunately going away... we have talked in the past about doing
"virtual MTRRs" in terms of PAT to deal with this kind of legacy
software, but the demand for it seems to be low enough to not be worth
bothering with.

>> However, since a virtual machine like Xen doesn't have these issues, it
>> doesn't apply
> 
> Well, we're specifically talking about a virtual machine which has
> direct access to hardware, so it is concerned about the real physical
> memory properties of real physical pages.  If we can assume that
> BIOS/Xen will always set up MTRR correctly then there shouldn't be any
> need for the kernel to modify the MTRR itself.  How true is that in
> general?  I don't know, but if we could rely on BIOS then there'd never
> be a need to touch MTRR, would there?

Well, in the past MTRRs were abused for device properties mainly by the
X server, but other than that, no, not really.  The other thing we do is
the MTRR cleanup (which doesn't involve /proc/mtrr) to deal with
brokenness in the BIOS setup, but that really belongs in the hypervisor
in your case since it fundamentally affects how memory is handled.

	-hpa

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 18:19             ` H. Peter Anvin
@ 2010-09-28 18:24               ` Jeremy Fitzhardinge
  2010-09-28 18:46                 ` H. Peter Anvin
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Fitzhardinge @ 2010-09-28 18:24 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

 On 09/28/2010 11:19 AM, H. Peter Anvin wrote:
> On 09/28/2010 11:13 AM, Jeremy Fitzhardinge wrote:
>>  On 09/28/2010 10:56 AM, H. Peter Anvin wrote:
>>> On 09/28/2010 10:13 AM, Jeremy Fitzhardinge wrote:
>>>> Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.
>>>>
>>>> The alternative would be to use the wrmsr hooks to emulate the Intel
>>>> MTRR registers by mapping them to hypercalls, but that seems needlessly
>>>> complex.
>>>>
>>> Indeed.  Relying on pure PAT is the Right Thing[TM].
>> Is there a plan to formally deprecate /proc/mtrr and the kernel
>> infrastructure behind it?
>>
> No, and we really can't do it for a couple of reasons:
>
> a) Pre-PAT hardware;
> b) MTRRs and PAT interact on hardware;
> c) MTRRs, but not PAT, interact with SMM.

What about pre-PAT software (ie, X servers which still use /proc/mtrr)?

> However, since a virtual machine like Xen doesn't have these issues, it
> doesn't apply

Well, we're specifically talking about a virtual machine which has
direct access to hardware, so it is concerned about the real physical
memory properties of real physical pages.  If we can assume that
BIOS/Xen will always set up MTRR correctly then there shouldn't be any
need for the kernel to modify the MTRR itself.  How true is that in
general?  I don't know, but if we could rely on BIOS then there'd never
be a need to touch MTRR, would there?

    J

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 18:13           ` Jeremy Fitzhardinge
@ 2010-09-28 18:19             ` H. Peter Anvin
  2010-09-28 18:24               ` Jeremy Fitzhardinge
  0 siblings, 1 reply; 14+ messages in thread
From: H. Peter Anvin @ 2010-09-28 18:19 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

On 09/28/2010 11:13 AM, Jeremy Fitzhardinge wrote:
>  On 09/28/2010 10:56 AM, H. Peter Anvin wrote:
>> On 09/28/2010 10:13 AM, Jeremy Fitzhardinge wrote:
>>> Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.
>>>
>>> The alternative would be to use the wrmsr hooks to emulate the Intel
>>> MTRR registers by mapping them to hypercalls, but that seems needlessly
>>> complex.
>>>
>> Indeed.  Relying on pure PAT is the Right Thing[TM].
> 
> Is there a plan to formally deprecate /proc/mtrr and the kernel
> infrastructure behind it?
> 

No, and we really can't do it for a couple of reasons:

a) Pre-PAT hardware;
b) MTRRs and PAT interact on hardware;
c) MTRRs, but not PAT, interact with SMM.

However, since a virtual machine like Xen doesn't have these issues, it
doesn't apply.

	-hpa

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 17:56         ` H. Peter Anvin
@ 2010-09-28 18:13           ` Jeremy Fitzhardinge
  2010-09-28 18:19             ` H. Peter Anvin
  0 siblings, 1 reply; 14+ messages in thread
From: Jeremy Fitzhardinge @ 2010-09-28 18:13 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

 On 09/28/2010 10:56 AM, H. Peter Anvin wrote:
> On 09/28/2010 10:13 AM, Jeremy Fitzhardinge wrote:
>> Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.
>>
>> The alternative would be to use the wrmsr hooks to emulate the Intel
>> MTRR registers by mapping them to hypercalls, but that seems needlessly
>> complex.
>>
> Indeed.  Relying on pure PAT is the Right Thing[TM].

Is there a plan to formally deprecate /proc/mtrr and the kernel
infrastructure behind it?

    J

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 17:13       ` Jeremy Fitzhardinge
  2010-09-28 17:19         ` Stefano Stabellini
@ 2010-09-28 17:56         ` H. Peter Anvin
  2010-09-28 18:13           ` Jeremy Fitzhardinge
  1 sibling, 1 reply; 14+ messages in thread
From: H. Peter Anvin @ 2010-09-28 17:56 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

On 09/28/2010 10:13 AM, Jeremy Fitzhardinge wrote:
> 
> Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.
> 
> The alternative would be to use the wrmsr hooks to emulate the Intel
> MTRR registers by mapping them to hypercalls, but that seems needlessly
> complex.
> 

Indeed.  Relying on pure PAT is the Right Thing[TM].

	-hpa


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 17:13       ` Jeremy Fitzhardinge
@ 2010-09-28 17:19         ` Stefano Stabellini
  2010-09-28 17:56         ` H. Peter Anvin
  1 sibling, 0 replies; 14+ messages in thread
From: Stefano Stabellini @ 2010-09-28 17:19 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Stefano Stabellini, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
	linux-kernel, xen-devel, Jeremy Fitzhardinge, sct

On Tue, 28 Sep 2010, Jeremy Fitzhardinge wrote:
>  On 09/28/2010 07:00 AM, Stefano Stabellini wrote:
> > On Tue, 28 Sep 2010, Ingo Molnar wrote:
> >> * stefano.stabellini@eu.citrix.com <stefano.stabellini@eu.citrix.com> wrote:
> >>
> >>> From: Stephen Tweedie <sct@redhat.com>
> >>>
> >>> Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
> >>> allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
> >>> querying the hypervisor itself.)
> >>>
> >>> [ Impact: add basic MTRR support ]
> >>>
> >>> Signed-off-by: Stephen Tweedie <sct@redhat.com>
> >>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> ---
> >>>  arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
> >>>  arch/x86/kernel/cpu/mtrr/main.c   |    3 +
> >>>  arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
> >>>  arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
> >>>  4 files changed, 121 insertions(+), 1 deletions(-)
> >>>  create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c
> >> Still NAK, for the very same reasons as we NAK-ed it the previous time: 
> >> /proc/mtrr is a problematic and complicated legacy interface that should 
> >> die. Any modern X server will do the right thing via PAT.
> >>
> > Sorry I should have read the original thread more carefully: I didn't
> > realize this patch had been NAK-ed.
> >
> > However it is not a problem because we can easily disable MTRRs from Xen
> > and with no cpu_has_mtrr the kernel would still boot fine on Xen.
> > Also I think we do have PAT support nowadays but I'll let Jeremy comment
> > on that.
> 
> Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.
> 
> The alternative would be to use the wrmsr hooks to emulate the Intel
> MTRR registers by mapping them to hypercalls, but that seems needlessly
> complex.
 
Yeah, I have already tested a prototype patch to xen to mask the MTRR
cpu features and everything seems to run fine on my testbox.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 12:39   ` Ingo Molnar
  2010-09-28 14:00     ` Stefano Stabellini
@ 2010-09-28 17:14     ` Jeremy Fitzhardinge
  1 sibling, 0 replies; 14+ messages in thread
From: Jeremy Fitzhardinge @ 2010-09-28 17:14 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: stefano.stabellini, Thomas Gleixner, H. Peter Anvin,
	linux-kernel, xen-devel, Jeremy Fitzhardinge, Stephen Tweedie

 On 09/28/2010 05:39 AM, Ingo Molnar wrote:
> * stefano.stabellini@eu.citrix.com <stefano.stabellini@eu.citrix.com> wrote:
>
>> From: Stephen Tweedie <sct@redhat.com>
>>
>> Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
>> allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
>> querying the hypervisor itself.)
>>
>> [ Impact: add basic MTRR support ]
>>
>> Signed-off-by: Stephen Tweedie <sct@redhat.com>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> ---
>>  arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
>>  arch/x86/kernel/cpu/mtrr/main.c   |    3 +
>>  arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
>>  arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 121 insertions(+), 1 deletions(-)
>>  create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c
> Still NAK, for the very same reasons as we NAK-ed it the previous time: 
> /proc/mtrr is a problematic and complicated legacy interface that should 
> die.

Is your objection because we're adding a Xen mtrr implementation at all,
or because we're (slightly) extending the mtrr interface to do it?

Thanks,
    J

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 14:00     ` Stefano Stabellini
@ 2010-09-28 17:13       ` Jeremy Fitzhardinge
  2010-09-28 17:19         ` Stefano Stabellini
  2010-09-28 17:56         ` H. Peter Anvin
  0 siblings, 2 replies; 14+ messages in thread
From: Jeremy Fitzhardinge @ 2010-09-28 17:13 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Ingo Molnar, Thomas Gleixner, H. Peter Anvin, linux-kernel,
	xen-devel, Jeremy Fitzhardinge, sct

 On 09/28/2010 07:00 AM, Stefano Stabellini wrote:
> On Tue, 28 Sep 2010, Ingo Molnar wrote:
>> * stefano.stabellini@eu.citrix.com <stefano.stabellini@eu.citrix.com> wrote:
>>
>>> From: Stephen Tweedie <sct@redhat.com>
>>>
>>> Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
>>> allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
>>> querying the hypervisor itself.)
>>>
>>> [ Impact: add basic MTRR support ]
>>>
>>> Signed-off-by: Stephen Tweedie <sct@redhat.com>
>>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> ---
>>>  arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
>>>  arch/x86/kernel/cpu/mtrr/main.c   |    3 +
>>>  arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
>>>  arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 121 insertions(+), 1 deletions(-)
>>>  create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c
>> Still NAK, for the very same reasons as we NAK-ed it the previous time: 
>> /proc/mtrr is a problematic and complicated legacy interface that should 
>> die. Any modern X server will do the right thing via PAT.
>>
> Sorry I should have read the original thread more carefully: I didn't
> realize this patch had been NAK-ed.
>
> However it is not a problem because we can easily disable MTRRs from Xen
> and with no cpu_has_mtrr the kernel would still boot fine on Xen.
> Also I think we do have PAT support nowadays but I'll let Jeremy comment
> on that.

Yes, we could just mask out the MTRR CPU feature and rely entirely on PAT.

The alternative would be to use the wrmsr hooks to emulate the Intel
MTRR registers by mapping them to hypercalls, but that seems needlessly
complex.

    J

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 12:39   ` Ingo Molnar
@ 2010-09-28 14:00     ` Stefano Stabellini
  2010-09-28 17:13       ` Jeremy Fitzhardinge
  2010-09-28 17:14     ` Jeremy Fitzhardinge
  1 sibling, 1 reply; 14+ messages in thread
From: Stefano Stabellini @ 2010-09-28 14:00 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Stefano Stabellini, Thomas Gleixner, H. Peter Anvin,
	linux-kernel, xen-devel, Jeremy Fitzhardinge, sct

On Tue, 28 Sep 2010, Ingo Molnar wrote:
> 
> * stefano.stabellini@eu.citrix.com <stefano.stabellini@eu.citrix.com> wrote:
> 
> > From: Stephen Tweedie <sct@redhat.com>
> > 
> > Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
> > allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
> > querying the hypervisor itself.)
> > 
> > [ Impact: add basic MTRR support ]
> > 
> > Signed-off-by: Stephen Tweedie <sct@redhat.com>
> > Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
> >  arch/x86/kernel/cpu/mtrr/main.c   |    3 +
> >  arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
> >  arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 121 insertions(+), 1 deletions(-)
> >  create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c
> 
> Still NAK, for the very same reasons as we NAK-ed it the previous time: 
> /proc/mtrr is a problematic and complicated legacy interface that should 
> die. Any modern X server will do the right thing via PAT.
> 

Sorry I should have read the original thread more carefully: I didn't
realize this patch had been NAK-ed.

However it is not a problem because we can easily disable MTRRs from Xen
and with no cpu_has_mtrr the kernel would still boot fine on Xen.
Also I think we do have PAT support nowadays but I'll let Jeremy comment
on that.


> Also, please get the Ack of at least one x86 maintainer for x86 patches.
> 
 
I'll repost the series without the last two patches, so there won't be
any x86 changes at all :)

Many thanks for your quick feedback,

Stefano

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 12:16 ` [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr stefano.stabellini
@ 2010-09-28 12:39   ` Ingo Molnar
  2010-09-28 14:00     ` Stefano Stabellini
  2010-09-28 17:14     ` Jeremy Fitzhardinge
  0 siblings, 2 replies; 14+ messages in thread
From: Ingo Molnar @ 2010-09-28 12:39 UTC (permalink / raw)
  To: stefano.stabellini, Thomas Gleixner, H. Peter Anvin
  Cc: linux-kernel, xen-devel, Jeremy Fitzhardinge, Stephen Tweedie,
	H. Peter Anvin


* stefano.stabellini@eu.citrix.com <stefano.stabellini@eu.citrix.com> wrote:

> From: Stephen Tweedie <sct@redhat.com>
> 
> Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
> allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
> querying the hypervisor itself.)
> 
> [ Impact: add basic MTRR support ]
> 
> Signed-off-by: Stephen Tweedie <sct@redhat.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
>  arch/x86/kernel/cpu/mtrr/main.c   |    3 +
>  arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
>  arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 121 insertions(+), 1 deletions(-)
>  create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c

Still NAK, for the very same reasons as we NAK-ed it the previous time: 
/proc/mtrr is a problematic and complicated legacy interface that should 
die. Any modern X server will do the right thing via PAT.

Also, please get the Ack of at least one x86 maintainer for x86 patches.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr
  2010-09-28 12:16 [PATCH 00/12] xen: initial domain support Stefano Stabellini
@ 2010-09-28 12:16 ` stefano.stabellini
  2010-09-28 12:39   ` Ingo Molnar
  0 siblings, 1 reply; 14+ messages in thread
From: stefano.stabellini @ 2010-09-28 12:16 UTC (permalink / raw)
  To: linux-kernel
  Cc: xen-devel, Jeremy Fitzhardinge, Stefano Stabellini,
	Stephen Tweedie, Jeremy Fitzhardinge, Stefano Stabellini

From: Stephen Tweedie <sct@redhat.com>

Add a Xen mtrr type, and reorganise mtrr initialisation slightly to
allow the mtrr driver to set up num_var_ranges (Xen needs to do this by
querying the hypervisor itself.)

[ Impact: add basic MTRR support ]

Signed-off-by: Stephen Tweedie <sct@redhat.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/x86/kernel/cpu/mtrr/Makefile |    2 +-
 arch/x86/kernel/cpu/mtrr/main.c   |    3 +
 arch/x86/kernel/cpu/mtrr/mtrr.h   |    7 ++
 arch/x86/kernel/cpu/mtrr/xen.c    |  110 +++++++++++++++++++++++++++++++++++++
 4 files changed, 121 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/mtrr/xen.c

diff --git a/arch/x86/kernel/cpu/mtrr/Makefile b/arch/x86/kernel/cpu/mtrr/Makefile
index ad9e5ed..e955771 100644
--- a/arch/x86/kernel/cpu/mtrr/Makefile
+++ b/arch/x86/kernel/cpu/mtrr/Makefile
@@ -1,3 +1,3 @@
 obj-y		:= main.o if.o generic.o cleanup.o
 obj-$(CONFIG_X86_32) += amd.o cyrix.o centaur.o
-
+obj-$(CONFIG_XEN) += xen.o
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 91f8f62..fcfa520 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -727,6 +727,9 @@ void __init mtrr_bp_init(void)
 		}
 	}
 
+	/* Let Xen code override the above if it wants */
+	xen_init_mtrr();
+
 	if (mtrr_if) {
 		num_var_ranges = mtrr_if->num_var_ranges();
 		init_table();
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index add8abe..8f0693e 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -74,6 +74,13 @@ void mtrr_wrmsr(unsigned, unsigned, unsigned);
 int amd_init_mtrr(void);
 int cyrix_init_mtrr(void);
 int centaur_init_mtrr(void);
+#ifdef CONFIG_XEN
+void xen_init_mtrr(void);
+#else
+static inline void xen_init_mtrr(void)
+{
+}
+#endif
 
 extern int changed_by_mtrr_cleanup;
 extern int mtrr_cleanup(unsigned address_bits);
diff --git a/arch/x86/kernel/cpu/mtrr/xen.c b/arch/x86/kernel/cpu/mtrr/xen.c
new file mode 100644
index 0000000..fec3b0a
--- /dev/null
+++ b/arch/x86/kernel/cpu/mtrr/xen.c
@@ -0,0 +1,110 @@
+#include <linux/init.h>
+#include <linux/mm.h>
+
+#include <asm/pat.h>
+#include <asm/mtrr.h>
+
+#include "mtrr.h"
+
+#include <xen/xen.h>
+#include <xen/interface/platform.h>
+#include <asm/xen/hypervisor.h>
+#include <asm/xen/hypercall.h>
+
+static void xen_set_mtrr(unsigned int reg, unsigned long base,
+			 unsigned long size, mtrr_type type)
+{
+	struct xen_platform_op op;
+	int error;
+
+	/* mtrr_ops->set() is called once per CPU,
+	 * but Xen's ops apply to all CPUs.
+	 */
+	if (smp_processor_id())
+		return;
+
+	if (size == 0) {
+		op.cmd = XENPF_del_memtype;
+		op.u.del_memtype.handle = 0;
+		op.u.del_memtype.reg    = reg;
+	} else {
+		op.cmd = XENPF_add_memtype;
+		op.u.add_memtype.mfn     = base;
+		op.u.add_memtype.nr_mfns = size;
+		op.u.add_memtype.type    = type;
+	}
+
+	error = HYPERVISOR_dom0_op(&op);
+	BUG_ON(error != 0);
+}
+
+static void xen_get_mtrr(unsigned int reg, unsigned long *base,
+			 unsigned long *size, mtrr_type *type)
+{
+	struct xen_platform_op op;
+
+	op.cmd = XENPF_read_memtype;
+	op.u.read_memtype.reg = reg;
+	if (HYPERVISOR_dom0_op(&op) != 0) {
+		*base = 0;
+		*size = 0;
+		*type = 0;
+		return;
+	}
+
+	*size = op.u.read_memtype.nr_mfns;
+	*base = op.u.read_memtype.mfn;
+	*type = op.u.read_memtype.type;
+}
+
+static int __init xen_num_var_ranges(void)
+{
+	int ranges;
+	struct xen_platform_op op;
+
+	op.cmd = XENPF_read_memtype;
+
+	for (ranges = 0; ; ranges++) {
+		op.u.read_memtype.reg = ranges;
+		if (HYPERVISOR_dom0_op(&op) != 0)
+			break;
+	}
+	return ranges;
+}
+
+/*
+ * DOM0 TODO: Need to fill in the remaining mtrr methods to have full
+ * working userland mtrr support.
+ */
+static struct mtrr_ops xen_mtrr_ops = {
+	.vendor            = X86_VENDOR_UNKNOWN,
+	.get_free_region   = generic_get_free_region,
+	.set               = xen_set_mtrr,
+	.get               = xen_get_mtrr,
+	.have_wrcomb       = positive_have_wrcomb,
+	.validate_add_page = generic_validate_add_page,
+	.use_intel_if	   = 0,
+	.num_var_ranges	   = xen_num_var_ranges,
+};
+
+void __init xen_init_mtrr(void)
+{
+	/* 
+	 * Check that we're running under Xen, and privileged enough
+	 * to play with MTRRs.
+	 */
+	if (!xen_initial_domain())
+		return;
+
+	/* 
+	 * Check that the CPU has an MTRR implementation we can
+	 * support.
+	 */
+	if (cpu_has_mtrr ||
+	    cpu_has_k6_mtrr ||
+	    cpu_has_cyrix_arr ||
+	    cpu_has_centaur_mcr) {
+		mtrr_if = &xen_mtrr_ops;
+		pat_init();
+	}
+}
-- 
1.5.6.5


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2010-09-28 19:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-28 13:19 [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr Sander Eikelenboom
  -- strict thread matches above, loose matches on Subject: below --
2010-09-28 12:16 [PATCH 00/12] xen: initial domain support Stefano Stabellini
2010-09-28 12:16 ` [PATCH 12/12] xen/mtrr: Add mtrr_if support for Xen mtrr stefano.stabellini
2010-09-28 12:39   ` Ingo Molnar
2010-09-28 14:00     ` Stefano Stabellini
2010-09-28 17:13       ` Jeremy Fitzhardinge
2010-09-28 17:19         ` Stefano Stabellini
2010-09-28 17:56         ` H. Peter Anvin
2010-09-28 18:13           ` Jeremy Fitzhardinge
2010-09-28 18:19             ` H. Peter Anvin
2010-09-28 18:24               ` Jeremy Fitzhardinge
2010-09-28 18:46                 ` H. Peter Anvin
2010-09-28 18:58                   ` Jeremy Fitzhardinge
2010-09-28 19:05                     ` H. Peter Anvin
2010-09-28 17:14     ` Jeremy Fitzhardinge

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®