Re: [PATCH 05/32] x86/mtrr: Stop using 32-bit MSR interfaces

[Ingo Molnar <mingo@kernel.org>]

* Jürgen Groß <jgross@suse.com> wrote:

> On 29.06.26 13:33, Ingo Molnar wrote:
> > 
> > * Juergen Gross <jgross@suse.com> wrote:
> > 
> > >   amd_get_mtrr(unsigned int reg, unsigned long *base,
> > >   	     unsigned long *size, mtrr_type *type)
> > >   {
> > > -	unsigned long low, high;
> > > +	unsigned long val;
> > > +	struct msr msr;
> > 
> > So, 'struct msr' is defined in <asm/msr.h>, but:
> > 
> > >   static void
> > >   amd_set_mtrr(unsigned int reg, unsigned long base, unsigned long size, mtrr_type type)
> > >   {
> > > -	u32 regs[2];
> > > +	union {
> > > +		u32 regs[2];
> > > +		u64 val;
> > > +	} msr;
> > 
> > Why do we use a local anonymous union here with overlapping
> > nomenclature and a confusingly different but (AFAICS) functionally
> > equivalent definition to 'struct msr'?
> 
> This is done in order to avoid having to change the code too much.

That's OK, but I'd suggest a followup cleanup in a separate patch:

> alternative I could use struct msr by doing:
> 
>   rdmsrq(MSR_K6_UWCCR, msr.q);
>   regs[0] = msr.l;
>   regs[1] = msr.h;
> 
>   ...
> 
>   msr.l = regs[0];
>   msr.h = regs[1];
>   wrmsrq(MSR_K6_UWCCR, msr.q);

Or just move regs[] over to a struct msr variable use?

Thanks,

	Ingo