| Message ID | 20190625125836.9149-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Memory barrier before setting ICR | expand |
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@gmail.com> > --- > 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.
> On Jun 26, 2019, at 6:07 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> 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@gmail.com> >> --- >> 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?
On 06/27/2019 10:44 AM, Nadav Amit wrote: >> On Jun 26, 2019, at 6:07 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> 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@gmail.com> >>> --- >>> 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@oracle.com>
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)); }
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@gmail.com> --- lib/x86/apic.c | 2 ++ 1 file changed, 2 insertions(+)