[kvm-unit-tests PATCH] x86: Memory barrier before setting ICR

[kvm-unit-tests PATCH] x86: Memory barrier before setting ICR

[Nadav Amit]

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));
 }
-- 
2.17.1

Re: [kvm-unit-tests PATCH] x86: Memory barrier before setting ICR

[Krish Sadhukhan]

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.

Re: [kvm-unit-tests PATCH] x86: Memory barrier before setting ICR

[Nadav Amit]

> 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?

Re: [kvm-unit-tests PATCH] x86: Memory barrier before setting ICR

[Krish Sadhukhan]

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>