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

[Huang, Kai]

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.