On 06/27/2019 10:44 AM, Nadav Amit wrote:
On Jun 26, 2019, at 6:07 PM, Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx> wrote: On 6/25/19 5:58 AM, Nadav Amit wrote:The wrmsr that is used in x2apic ICR programming does not behave as a memory barrier. There is a hidden assumption that it is. Add an explicit memory barrier for this reason. Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx> --- lib/x86/apic.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/x86/apic.c b/lib/x86/apic.c index bc2706e..1514730 100644 --- a/lib/x86/apic.c +++ b/lib/x86/apic.c @@ -2,6 +2,7 @@ #include "apic.h" #include "msr.h" #include "processor.h" +#include "asm/barrier.h" void *g_apic = (void *)0xfee00000; void *g_ioapic = (void *)0xfec00000; @@ -71,6 +72,7 @@ static void x2apic_write(unsigned reg, u32 val) static void x2apic_icr_write(u32 val, u32 dest) { + mb(); asm volatile ("wrmsr" : : "a"(val), "d"(dest), "c"(APIC_BASE_MSR + APIC_ICR/16)); }Regarding non-serializing forms, the SDM mentions, "X2APIC MSRs (MSR indices 802H to 83FH)" (APIC_BASE_MSR + APIC_ICR/16) is a different value. So I am wondering why we need a barrier here.What am I missing here? APIC_BASE_MSR = 0x800 APIC_ICR = 0x300 So 0x800 + (0x300 / 16) = 0x830 , and 0x802 <= 0x830 <= 0x83f Hence, writes to APIC_ICR do not seem to be serializing. No?
Sorry, my bad ! I missed the "to" in the quote from the SDM and thought only two MSRs were non-serializing.
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
