Re: APIC MSRs query

["Jan Beulich" <JBeulich@novell.com>]

>>> On 17.05.11 at 15:59, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2011-05-17 at 14:43 +0100, Jan Beulich wrote:
>> >>> On 17.05.11 at 15:25, Andrew Cooper <andrew.cooper3@citrix.com> 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:

Surely the Xen code got cloned from the Linux one.

> 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).

This is what I assumed, and I'm sure it's just for simplicity that 0 gets
passed here. I don't think it's correct, the more that the actual base
address doesn't matter while in x2apic mode (but would matter when
switching back to legacy mode e.g. for kexec).

Jan