Re: [Xen-devel] [PATCH v2 1/6] x86/mm/pat: Change PAT to support non-default PAT MSR
On Wed, 2016-03-23 at 09:43 +0100, Borislav Petkov wrote: > On Tue, Mar 22, 2016 at 12:35:19PM -0600, Toshi Kani wrote: > > Right. Will change to "Add support of non-default PAT MSR setting at > > handoff". > > Please remove this "handoff" notion from the text. Every hw register is > being handed off to the OS once the kernel takes over so there's no need > to make it special here. Will do. > > I'd like to make it clear that this function does not set PAT MSR, > > unlike what pat_init() does. When CPU supports PAT, it keeps PAT MSR > > in whatever the setting at handoff, and initializes PAT table to match > > with this setting. > > > > I am open to a better name, but I am afraid that setup_pat() can be > > confusing as if it sets PAT MSR. > > So call it init_cache_modes() and rename the current > pat_init_cache_modes() to __init_cache_modes() to denote it is a lower > level helper of the init_cache_modes() one. The init_cache_modes() one > deals with the higher level figuring out of whether PAT is enabled and > if not, preparing the attr bits for emulation. In the end, it calls > __init_cache_modes(). All nice and easy. Good idea! Will do. Thanks, -Toshi ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/6] x86/mm/pat: Change PAT to support non-default PAT MSR
On Tue, Mar 22, 2016 at 12:35:19PM -0600, Toshi Kani wrote: > Right. Will change to "Add support of non-default PAT MSR setting at > handoff". Please remove this "handoff" notion from the text. Every hw register is being handed off to the OS once the kernel takes over so there's no need to make it special here. > I'd like to make it clear that this function does not set PAT MSR, unlike > what pat_init() does. When CPU supports PAT, it keeps PAT MSR in whatever > the setting at handoff, and initializes PAT table to match with this > setting. > > I am open to a better name, but I am afraid that setup_pat() can be > confusing as if it sets PAT MSR. So call it init_cache_modes() and rename the current pat_init_cache_modes() to __init_cache_modes() to denote it is a lower level helper of the init_cache_modes() one. The init_cache_modes() one deals with the higher level figuring out of whether PAT is enabled and if not, preparing the attr bits for emulation. In the end, it calls __init_cache_modes(). All nice and easy. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/6] x86/mm/pat: Change PAT to support non-default PAT MSR
On Tue, 22 Mar 2016, Borislav Petkov wrote: > > +static void pat_keep_handoff_state(void) > > Static function, no need for "pat_" prefix. Also, no need for the > kernel-doc comment. I have to disagree. kernel-doc comments are not limited to global functions. I realy prefer kernel-doc style for static functions over randomly formatted ones for consistency reasons. Thanks, tglx ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/6] x86/mm/pat: Change PAT to support non-default PAT MSR
On Tue, 2016-03-22 at 17:57 +0100, Borislav Petkov wrote:
> $Subject is misleading - there's no non-default PAT MSR - the setting is
> non-default.
Right. Will change to "Add support of non-default PAT MSR setting at
handoff".
> On Wed, Mar 16, 2016 at 06:44:57PM -0600, Toshi Kani wrote:
> > In preparation to fix a regression caused by 'commit 9cd25aac1f44
> > ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to
> > support a case that PAT MSR is initialized with a non-default
> > value.
> >
> > When pat_init() is called in PAT disable state, it initializes
>
> is called and PAT is disabled
Will do.
> > PAT table with the BIOS default value. Xen, however, sets PAT MSR
> > with a non-default value to enable WC. This causes inconsistency
> > between PAT table and PAT MSR when PAT is set to disable on Xen.
> >
> > Change pat_init() to handle the PAT disable cases properly. Add
> > pat_keep_handoff_state() to handle two cases when PAT is set to
> > disable.
> > 1. CPU supports PAT: Set PAT table to be consistent with PAT MSR.
> > 2. CPU does not support PAT: Set PAT table to be consistent with
> > PWT and PCD bits in a PTE.
> >
:
> > +/**
> > + * pat_keep_handoff_state - Set PAT table to the handoff state
> > + *
> > + * This function keeps PAT in the BIOS handoff state. When CPU
> > supports
> > + * PAT, it sets PAT table to be consistent with PAT MSR. When CPU does
> > not
> > + * support PAT, it emulates PAT by setting PAT table consistent with
> > PWT
> > + * and PCD bits in a PTE.
> > + *
> > + * The PAT table is global to all CPUs, which is initialized once at
> > + * boot-time. Any subsequent calls to this function have no effect.
> > + */
> > +static void pat_keep_handoff_state(void)
>
> Static function, no need for "pat_" prefix. Also, no need for the
> kernel-doc comment.
>
> Also, no need for all that handoff nomenclature etc, just call it
> setup_pat(). Because it does exactly that - it sets up the PAT bits
> unconditionally, regardless of enabled or not.
I'd like to make it clear that this function does not set PAT MSR, unlike
what pat_init() does. When CPU supports PAT, it keeps PAT MSR in whatever
the setting at handoff, and initializes PAT table to match with this
setting.
I am open to a better name, but I am afraid that setup_pat() can be
confusing as if it sets PAT MSR.
> > {
> > - u64 pat;
> > - struct cpuinfo_x86 *c = _cpu_data;
> > + u64 pat = 0;
> > + static int set_handoff_done;
>
> s/set_handoff_done/pat_setup_done/
I will match it with a func name once we decided.
Thanks,
-Toshi
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
