linux-kernel - Re: [PATCH v5 04/15] x86/mtrr: support setting MTRR state for software defined MTRRs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2039843c14d04a4e9928834c4bf40ea93592d34.camel@intel.com>
Date:   Mon, 3 Apr 2023 09:43:10 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Gross, Jurgen" <jgross@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "mikelley@...rosoft.com" <mikelley@...rosoft.com>
Subject: Re: [PATCH v5 04/15] x86/mtrr: support setting MTRR state for
 software defined MTRRs

On Mon, 2023-04-03 at 09:27 +0000, Huang, Kai wrote:
> > > >   /**
> > > >    * mtrr_type_lookup - look up memory type in MTRR
> > > >    *
> > > > diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c
> > > > b/arch/x86/kernel/cpu/mtrr/mtrr.c
> > > > index 1beb38f7a7a3..1c19d67ddab3 100644
> > > > --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> > > > +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> > > > @@ -666,6 +666,15 @@ void __init mtrr_bp_init(void)
> > > >   	const char *why = "(not available)";
> > > >   	unsigned int phys_addr;
> > > >   
> > > > +	if (!generic_mtrrs && mtrr_state.enabled) {
> > > > +		/* Software overwrite of MTRR state, only for generic
> > > > case. */
> > > 							      ^
> > > 							      !generic
> > > case?
> > 
> > No. This test just verifies that the (visible) MTRR feature is switched off,
> > as there are no ways to modify any MTRR registers in the overwrite case.
> > 
> > I can make this more obvious in a comment.
> 
> Should the comment say something like (because it applies to the code inside
> the
> check):
> 
> 
> 	If we have a static (synthetic) MTRR already established for special 
> 	VMs, we still need to calculate the physical address bits using
> generic 
> 	way, because the hardware to run those special VMs indeed has MTRR.
> 
> That explains why 'true' is passed to mtrr_calc_physbits().
> 
> ?
> 
> 

And yes I guess it would be better to point out we cannot modify any MTRR
registers in the overwrite case, but this is also clear to me.  So no opinion on
this point, but I do find the original comment is a little bit confusing.