Re: [Xen-devel] APIC MSRs query

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/05/11 14:59, Ian Campbell wrote:
> On Tue, 2011-05-17 at 14:43 +0100, Jan Beulich wrote:
> > > > > On 17.05.11 at 15:25, Andrew Cooper<andrew.cooper3@xxxxxxxxxx>  wrote:
> > > Hello,
> > >
> > > I am currently cleaning up the APIC code for the sake of
> > > shutdown/reboot/crashdump and have a query about the (modified for
> > > brevity) snippet of code:
> > >
> > > uint64_t msr_content;
> > > rdmsrl(MSR_IA32_APICBASE, msr_content);
> > > msr_content |= MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD;
> > > msr_content = (uint32_t)msr_content;
> > > wrmsrl(MSR_IA32_APICBASE, msr_content);
> > >
> > > which is added into apic.c in changeset b622e411eef8, and has propagated
> > > elsewhere in the codebase during subsequent cleanups etc.
> > >
> > > The MP spec and x2apic spec states that bits [35:12] of
> > > MSR_IA32_APICBASE is the base APIC MMIO address.  Is there reason why
> > > the code (almost always) clears the top 4 bits, or is it just an
> > > overlooked mistake?
> > I think this is a benign mistake. Benign because I don't think there is
> > a meaningful (to Xen at least) number of systems that would not
> > have their LAPIC at the default address (which fits in 32 bits).
> That "msr_content = (uint32_t)msr_content;" seems to be pretty
> deliberate, what else would it be trying to do?
>
> FWIW enable_x2apic in Linux seems to have a similar construct which
> throws away the top half of the MSR:
>
> void enable_x2apic(void)
> {
>         int msr, msr2;
>
>         rdmsr(MSR_IA32_APICBASE, msr, msr2);
>         if (!(msr&  X2APIC_ENABLE)) {
>                 printk("Enabling x2apic\n");
>                 wrmsr(MSR_IA32_APICBASE, msr | X2APIC_ENABLE, 0);
>         }
> }
>
> (FWIW the original Xen code in 17545:9fd00ff95068 looked a lot like this
> too, b622e411eef8 just switched to wrmsrl and preserved the clearing
> behaviour).
>
> Perhaps there is some errata? Google didn't find one, but ...
>
> Ian.
I couldn't find any errata which is why I asked here.  Bits [63:36] are 
reserved so should be WriteAsZero - it is possible that whoever put it 
into Linux just wanted to zero the top bits and either missed the top 4 
bits or decided that they would never be set.
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel