Re: PATCH: MTRR save and restore.

Re: PATCH: MTRR save and restore.

[Pavel Machek]

Hi!

[I added l-k to the cc list].

> > > We could add it to suspend scripts, but wouldn't mtrrs fit into the
> > > driver model idea? Would you say the same thing about implementing S3
> > > support?
> > 
> > I think going through the driver model is the right thing to
> > do.
> 
> It's useless bloat.
> 
> > Userspace should not need to know about mtrrs,
> 
> It's a swsuspend helper daemon, not some random application. There
> *should* be a helper daemon, if not, the design is flawed.

There's no helper daemon, nor I plan to make one. Notice that mtrr
stuff is shared between S3 (== suspend to ram) and swsusp. Both S3 and
swsusp can be used to do some pretty important stuff (machine
overheats or battery critically low -> suspend somewhere), so I do not
think userland daemon is good idea.

It can be dependend on CONFIG_PM; if you still think that's too much
bloat, it could be dependend on CONFIG_SLEEP which could be only
compiled when S3 or swsusp is selected (but I feel that would be
overdesign).

								Pavel
-- 
When do you have heart between your knees?

Re: PATCH: MTRR save and restore.

[Patrick Mochel]

I'd like to see the rest of this thread ;)

> > > > We could add it to suspend scripts, but wouldn't mtrrs fit into the
> > > > driver model idea? Would you say the same thing about implementing S3
> > > > support?
> > > 
> > > I think going through the driver model is the right thing to
> > > do.
> > 
> > It's useless bloat.

What exactly is useless bloat? Based on the level of indentation, I'd 
assume that Richard said that...care to elaborate? 

> > > Userspace should not need to know about mtrrs,
> > 
> > It's a swsuspend helper daemon, not some random application. There
> > *should* be a helper daemon, if not, the design is flawed.

Care to elaborate on that one, too? What does a userland daemon add to 
make it a sane design? 

> There's no helper daemon, nor I plan to make one. Notice that mtrr
> stuff is shared between S3 (== suspend to ram) and swsusp. Both S3 and
> swsusp can be used to do some pretty important stuff (machine
> overheats or battery critically low -> suspend somewhere), so I do not
> think userland daemon is good idea.
> 
> It can be dependend on CONFIG_PM; if you still think that's too much
> bloat, it could be dependend on CONFIG_SLEEP which could be only
> compiled when S3 or swsusp is selected (but I feel that would be
> overdesign).

Yes, that's too much. 

MTRRs are one interface to an x86 CPU. CPUs are already represented in the
device tree. The proper thing to do would be to have the CPU suspend/ 
resume methods save and restore the MTRRs. It still requires an #ifdef in 
the CPU code, but with a little work, could be massaged down a ways. 


	-pat

Re: PATCH: MTRR save and restore.

[Alan Cox]

On Maw, 2003-04-15 at 21:18, Patrick Mochel wrote:
> MTRRs are one interface to an x86 CPU. CPUs are already represented in the
> device tree. The proper thing to do would be to have the CPU suspend/ 
> resume methods save and restore the MTRRs. It still requires an #ifdef in 
> the CPU code, but with a little work, could be massaged down a ways. 

Also it has to be done kernel side for SMP because having mismatched
MTRRs between processors is unsafe, yet lots of BIOSen get it wrong.

Re: PATCH: MTRR save and restore.

[Pavel Machek]

Hi!

> I'd like to see the rest of this thread ;)
> 
> > > > > We could add it to suspend scripts, but wouldn't mtrrs fit into the
> > > > > driver model idea? Would you say the same thing about implementing S3
> > > > > support?
> > > > 
> > > > I think going through the driver model is the right thing to
> > > > do.
> > > 
> > > It's useless bloat.
> 
> What exactly is useless bloat? Based on the level of indentation, I'd 
> assume that Richard said that...care to elaborate? 

Richard claims that MTRR save/restore is "useless bloat" and that it
can be don in userspace. He wants userland daemon to do it. I believe
that's bad idea (for reasons like suspend when battery low).

[This is what I said]
> > There's no helper daemon, nor I plan to make one. Notice that mtrr
> > stuff is shared between S3 (== suspend to ram) and swsusp. Both S3 and
> > swsusp can be used to do some pretty important stuff (machine
> > overheats or battery critically low -> suspend somewhere), so I do not
> > think userland daemon is good idea.
> > 
> > It can be dependend on CONFIG_PM; if you still think that's too much
> > bloat, it could be dependend on CONFIG_SLEEP which could be only
> > compiled when S3 or swsusp is selected (but I feel that would be
> > overdesign).
> 
> Yes, that's too much. 
> 
> MTRRs are one interface to an x86 CPU. CPUs are already represented in the
> device tree. The proper thing to do would be to have the CPU suspend/ 
> resume methods save and restore the MTRRs. It still requires an #ifdef in 
> the CPU code, but with a little work, could be massaged down a ways. 

Yep, agreed, mtrrs need in-kernel save/restore support.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]