[PATCH 1/2] x86/vmx: dump MSR load area

[PATCH 1/2] x86/vmx: dump MSR load area

[Matt Wilson]

From: Matt Wilson <msw@amazon.com>

... as it is very helpful to diagnose VM entry failures due to MSR
loading.

Signed-off-by: Matt Wilson <msw@amazon.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 1bd875a..a540d4e 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1740,6 +1740,20 @@ static void vmx_dump_sel2(char *name, uint32_t lim)
     printk("%s:            %08x %016"PRIx64"\n", name, limit, base);
 }
 
+static void vmx_dump_vcpu_msr_area(struct vcpu *v)
+{
+    const struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
+    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
+
+    printk("  msr_count = %d\n", v->arch.hvm_vmx.msr_count);
+    for ( i = 0; i < msr_count; i++ )
+    {
+        printk("  msr_area[%d].index=0x%08x .data=0x%016lx\n",
+               i, msr_area[i].index, msr_area[i].data);
+    }
+}
+
+
 void vmcs_dump_vcpu(struct vcpu *v)
 {
     struct cpu_user_regs *regs = &v->arch.user_regs;
@@ -1879,6 +1893,13 @@ void vmcs_dump_vcpu(struct vcpu *v)
          (SECONDARY_EXEC_ENABLE_VPID | SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) )
         printk("Virtual processor ID = 0x%04x VMfunc controls = %016lx\n",
                vmr16(VIRTUAL_PROCESSOR_ID), vmr(VM_FUNCTION_CONTROL));
+    printk("EXIT MSR load count = 0x%04x\n",
+           (uint32_t)vmr(VM_EXIT_MSR_LOAD_COUNT));
+    printk("EXIT MSR store count = 0x%04x\n",
+           (uint32_t)vmr(VM_EXIT_MSR_STORE_COUNT));
+    printk("ENTRY MSR load count = 0x%04x\n",
+           (uint32_t)vmr(VM_ENTRY_MSR_LOAD_COUNT));
+    vmx_dump_vcpu_msr_area(v);
 
     vmx_vmcs_exit(v);
 }
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] x86/vmx: conditionally disable LBR support due to TSX format quirk

[Matt Wilson]

From: Matt Wilson <msw@amazon.com>

Systems that support LBR formats that include TSX information but do
not support TSX require special handling when saving and restoring MSR
values. For example, see the Linux kernel quirks[1, 2] in the MSR
context switching code. As a wrmsr with certain values under these
conditions causes a #GP, VM entry will fail due to MSR loading (see
last bullet of SDM 26.4 "Loading MSRS").

This failure can be triggered on a Haswell-EP system with the
following test Linux kernel module:

In domU:
  $ cat > lbr.c << EOF

  static int __init
  lbr_init(void)
  {
          u64 val;

          rdmsrl(MSR_IA32_DEBUGCTLMSR, val);
          val |= DEBUGCTLMSR_LBR;
          wrmsrl(MSR_IA32_DEBUGCTLMSR, val);

          return 0;
  }

  static void __exit
  lbr_cleanup(void)
  {
  }

  module_init(lbr_init);
  module_exit(lbr_cleanup);

  MODULE_DESCRIPTION("lbr test");
  MODULE_LICENSE("GPL");
  EOF
  $ echo "obj-m += lbr.o" > Makefile
  $ make -C /lib/modules/`uname -r`/build M=`pwd` modules
  make: Entering directory `/home/ec2-user/linux'
    CC [M]  /home/ec2-user/lbr.o
    Building modules, stage 2.
    MODPOST 1 modules
    CC      /home/ec2-user/lbr.mod.o
    LD [M]  /home/ec2-user/lbr.ko
  make: Leaving directory `/home/ec2-user/linux'
  $ sudo insmod lbr.ko
  $ Timeout, server not responding.

In dom0:
  [...]
  (XEN) Failed vm entry (exit reason 0x80000022) caused by MSR entry 1 loading.
  [...]
  (XEN) EXIT MSR load count = 0x0001
  (XEN) EXIT MSR store count = 0x0023
  (XEN) ENTRY MSR load count = 0x0023
  (XEN)   msr_count = 35
  (XEN)   msr_area[0].index=0x000001dd .data=0x1fffffff811911db
  ...

This change dynamically disables LBR save/load on systems in the
problematic configuration.

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=71adae99ed18
[2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=19fc9ddd61e0

Signed-off-by: Matt Wilson <msw@amazon.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3d330b6..c51cefc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2576,8 +2576,22 @@ static const struct lbr_info *last_branch_msr_get(void)
         /* Haswell */
         case 60: case 63: case 69: case 70:
         /* Broadwell */
-        case 61: case 71: case 79: case 86:
+        case 61: case 71: case 79: case 86: {
+            u64 caps;
+            bool_t tsx_support = boot_cpu_has(X86_FEATURE_HLE) ||
+                                 boot_cpu_has(X86_FEATURE_RTM);
+
+            rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);
+            /*
+             * Unimplemented fixups are required if the processor
+             * supports an LBR format that includes TSX information,
+             * but not TSX. Disable LBR save/load on such platforms.
+             */
+            if ( !tsx_support && (caps & 4) )
+                return NULL;
+
             return nh_lbr;
+        }
         /* Skylake */
         case 78: case 94:
         /* future */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86/vmx: dump MSR load area

[Jan Beulich]

>>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1740,6 +1740,20 @@ static void vmx_dump_sel2(char *name, uint32_t lim)
>      printk("%s:            %08x %016"PRIx64"\n", name, limit, base);
>  }
>  
> +static void vmx_dump_vcpu_msr_area(struct vcpu *v)

const

> +{
> +    const struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
> +    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
> +
> +    printk("  msr_count = %d\n", v->arch.hvm_vmx.msr_count);

Please use the local variable you have just latched and %u.

> +    for ( i = 0; i < msr_count; i++ )
> +    {
> +        printk("  msr_area[%d].index=0x%08x .data=0x%016lx\n",

[%u], and I'm not sure the 0x prefixes are really all that helpful
here. I think it would also be beneficial to shorten the logged text
a little, as this may consume quite a bit of serial line bandwidth.
How about "   MSR[%08x]=%016lx\n"?

> @@ -1879,6 +1893,13 @@ void vmcs_dump_vcpu(struct vcpu *v)
>           (SECONDARY_EXEC_ENABLE_VPID | SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) 
> )
>          printk("Virtual processor ID = 0x%04x VMfunc controls = %016lx\n",
>                 vmr16(VIRTUAL_PROCESSOR_ID), vmr(VM_FUNCTION_CONTROL));
> +    printk("EXIT MSR load count = 0x%04x\n",
> +           (uint32_t)vmr(VM_EXIT_MSR_LOAD_COUNT));
> +    printk("EXIT MSR store count = 0x%04x\n",
> +           (uint32_t)vmr(VM_EXIT_MSR_STORE_COUNT));
> +    printk("ENTRY MSR load count = 0x%04x\n",
> +           (uint32_t)vmr(VM_ENTRY_MSR_LOAD_COUNT));

First - do you really need to make three log lines out of these? And
then, please use vmr32(), as the neighboring vmr16() suggests.
Plus finally - please log all four counts consistently either in hex
or in dec.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/vmx: conditionally disable LBR support due to TSX format quirk

[Jan Beulich]

>>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
> Systems that support LBR formats that include TSX information but do
> not support TSX require special handling when saving and restoring MSR
> values. For example, see the Linux kernel quirks[1, 2] in the MSR
> context switching code. As a wrmsr with certain values under these
> conditions causes a #GP, VM entry will fail due to MSR loading (see
> last bullet of SDM 26.4 "Loading MSRS").

I had recently noticed this as an area needing work too, so I'm glad
you (hopefully) eliminate this item from my todo list. However, I'm
sad to see that you really just disable LBR use in the problem case.
I'd prefer to take such a change only as an immediate workaround,
with the understanding that a proper one will be made available not
too much later.

Jun, Kevin - workarounds for hardware quirks like this one are really
what I would think should come out of Intel, and preferably not
after the first problem report on Xen, but soon after the quirk has
got identified in general (which according to the Linux patches must
have been at least 2 months ago).

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2576,8 +2576,22 @@ static const struct lbr_info *last_branch_msr_get(void)
>          /* Haswell */
>          case 60: case 63: case 69: case 70:
>          /* Broadwell */
> -        case 61: case 71: case 79: case 86:
> +        case 61: case 71: case 79: case 86: {
> +            u64 caps;
> +            bool_t tsx_support = boot_cpu_has(X86_FEATURE_HLE) ||
> +                                 boot_cpu_has(X86_FEATURE_RTM);
> +
> +            rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);

This is guarded by a X86_FEATURE_PDCM check in Linux - why
would we not need the same here? Also I think this RDMSR should
be performed once at boot, not every time we come here.

> +            /*
> +             * Unimplemented fixups are required if the processor
> +             * supports an LBR format that includes TSX information,
> +             * but not TSX. Disable LBR save/load on such platforms.
> +             */
> +            if ( !tsx_support && (caps & 4) )

As I understand it the low 6 bits of the MSR contain an enumeration
like value, so just checking bit 2 can't be right (and would wrongly
trigger on already defined format types 5 and 6 - even if those were
known to be impossible on the models currently covered, this would
be a latent trap for someone to fall into when adding further model
values).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86/vmx: dump MSR load area

[Matt Wilson]

On Wed, Aug 10, 2016 at 04:44:21AM -0600, Jan Beulich wrote:
> >>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -1740,6 +1740,20 @@ static void vmx_dump_sel2(char *name, uint32_t lim)
> >      printk("%s:            %08x %016"PRIx64"\n", name, limit, base);
> >  }
> >  
> > +static void vmx_dump_vcpu_msr_area(struct vcpu *v)
> 
> const

Ack.

> > +{
> > +    const struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
> > +    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
> > +
> > +    printk("  msr_count = %d\n", v->arch.hvm_vmx.msr_count);
> 
> Please use the local variable you have just latched and %u.

Ack.

> > +    for ( i = 0; i < msr_count; i++ )
> > +    {
> > +        printk("  msr_area[%d].index=0x%08x .data=0x%016lx\n",
> 
> [%u], and I'm not sure the 0x prefixes are really all that helpful
> here. I think it would also be beneficial to shorten the logged text
> a little, as this may consume quite a bit of serial line bandwidth.
> How about "   MSR[%08x]=%016lx\n"?

Everyone isn't running at 115,200 baud these days? ;-)

Ack.

> > @@ -1879,6 +1893,13 @@ void vmcs_dump_vcpu(struct vcpu *v)
> >           (SECONDARY_EXEC_ENABLE_VPID | SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) 
> > )
> >          printk("Virtual processor ID = 0x%04x VMfunc controls = %016lx\n",
> >                 vmr16(VIRTUAL_PROCESSOR_ID), vmr(VM_FUNCTION_CONTROL));
> > +    printk("EXIT MSR load count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_EXIT_MSR_LOAD_COUNT));
> > +    printk("EXIT MSR store count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_EXIT_MSR_STORE_COUNT));
> > +    printk("ENTRY MSR load count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_ENTRY_MSR_LOAD_COUNT));
> 
> First - do you really need to make three log lines out of these? And
> then, please use vmr32(), as the neighboring vmr16() suggests.
> Plus finally - please log all four counts consistently either in hex
> or in dec.

With one line, output might look something like:
(XEN) MSR load/store count ExitLoad=0x0001 ExitStore=0x0023 EntryLoad=0x0023

Spaces around = are inconsistent in the existing output and it seems
that no space is more popular. Does this format seem better to you?

I see three counts here - are you talking about the msr_count above?
For msr_count I was thinking that this is internal Xen state, whereas
the other values are VMCS fields where everything else is dumped in
hex. I think printing msr_count is redundant (one could just count the
lines of output), so I'll just remove it.

--msw

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/vmx: conditionally disable LBR support due to TSX format quirk

[Matt Wilson]

On Wed, Aug 10, 2016 at 06:34:10AM -0600, Jan Beulich wrote:
> >>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
> > Systems that support LBR formats that include TSX information but do
> > not support TSX require special handling when saving and restoring MSR
> > values. For example, see the Linux kernel quirks[1, 2] in the MSR
> > context switching code. As a wrmsr with certain values under these
> > conditions causes a #GP, VM entry will fail due to MSR loading (see
> > last bullet of SDM 26.4 "Loading MSRS").
> 
> I had recently noticed this as an area needing work too, so I'm glad
> you (hopefully) eliminate this item from my todo list. However, I'm
> sad to see that you really just disable LBR use in the problem case.
> I'd prefer to take such a change only as an immediate workaround,
> with the understanding that a proper one will be made available not
> too much later.

I agree that this is a short term workaround. I spent a while trying
to fix up the values but I wasn't successful.

> Jun, Kevin - workarounds for hardware quirks like this one are really
> what I would think should come out of Intel, and preferably not
> after the first problem report on Xen, but soon after the quirk has
> got identified in general (which according to the Linux patches must
> have been at least 2 months ago).

+1

> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2576,8 +2576,22 @@ static const struct lbr_info *last_branch_msr_get(void)
> >          /* Haswell */
> >          case 60: case 63: case 69: case 70:
> >          /* Broadwell */
> > -        case 61: case 71: case 79: case 86:
> > +        case 61: case 71: case 79: case 86: {
> > +            u64 caps;
> > +            bool_t tsx_support = boot_cpu_has(X86_FEATURE_HLE) ||
> > +                                 boot_cpu_has(X86_FEATURE_RTM);
> > +
> > +            rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);
> 
> This is guarded by a X86_FEATURE_PDCM check in Linux - why
> would we not need the same here?

You're right, it should be. It also seems to be missing from
core2_vpmu_init().

> Also I think this RDMSR should be performed once at boot, not every
> time we come here.

I thought you might say that. It didn't seem obviously right to put
this in boot_cpu_data -- is that what you suggest?

> > +            /*
> > +             * Unimplemented fixups are required if the processor
> > +             * supports an LBR format that includes TSX information,
> > +             * but not TSX. Disable LBR save/load on such platforms.
> > +             */
> > +            if ( !tsx_support && (caps & 4) )
> 
> As I understand it the low 6 bits of the MSR contain an enumeration
> like value, so just checking bit 2 can't be right (and would wrongly
> trigger on already defined format types 5 and 6 - even if those were
> known to be impossible on the models currently covered, this would
> be a latent trap for someone to fall into when adding further model
> values).

Fair point.

--msw

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86/vmx: dump MSR load area

[Jan Beulich]

>>> On 10.08.16 at 16:25, <msw@amzn.com> wrote:
> On Wed, Aug 10, 2016 at 04:44:21AM -0600, Jan Beulich wrote:
>> >>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
>> > @@ -1879,6 +1893,13 @@ void vmcs_dump_vcpu(struct vcpu *v)
>> >           (SECONDARY_EXEC_ENABLE_VPID | SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) 
> 
>> > )
>> >          printk("Virtual processor ID = 0x%04x VMfunc controls = %016lx\n",
>> >                 vmr16(VIRTUAL_PROCESSOR_ID), vmr(VM_FUNCTION_CONTROL));
>> > +    printk("EXIT MSR load count = 0x%04x\n",
>> > +           (uint32_t)vmr(VM_EXIT_MSR_LOAD_COUNT));
>> > +    printk("EXIT MSR store count = 0x%04x\n",
>> > +           (uint32_t)vmr(VM_EXIT_MSR_STORE_COUNT));
>> > +    printk("ENTRY MSR load count = 0x%04x\n",
>> > +           (uint32_t)vmr(VM_ENTRY_MSR_LOAD_COUNT));
>> 
>> First - do you really need to make three log lines out of these? And
>> then, please use vmr32(), as the neighboring vmr16() suggests.
>> Plus finally - please log all four counts consistently either in hex
>> or in dec.
> 
> With one line, output might look something like:
> (XEN) MSR load/store count ExitLoad=0x0001 ExitStore=0x0023 EntryLoad=0x0023
> 
> Spaces around = are inconsistent in the existing output and it seems
> that no space is more popular. Does this format seem better to you?

Yes.

> I see three counts here - are you talking about the msr_count above?

Yes.

> For msr_count I was thinking that this is internal Xen state, whereas
> the other values are VMCS fields where everything else is dumped in
> hex. I think printing msr_count is redundant (one could just count the
> lines of output), so I'll just remove it.

That's fine as an option of course.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/vmx: conditionally disable LBR support due to TSX format quirk

[Jan Beulich]

>>> On 10.08.16 at 17:47, <msw@amzn.com> wrote:
> On Wed, Aug 10, 2016 at 06:34:10AM -0600, Jan Beulich wrote:
>> >>> On 10.08.16 at 08:59, <msw@amzn.com> wrote:
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -2576,8 +2576,22 @@ static const struct lbr_info *last_branch_msr_get(void)
>> >          /* Haswell */
>> >          case 60: case 63: case 69: case 70:
>> >          /* Broadwell */
>> > -        case 61: case 71: case 79: case 86:
>> > +        case 61: case 71: case 79: case 86: {
>> > +            u64 caps;
>> > +            bool_t tsx_support = boot_cpu_has(X86_FEATURE_HLE) ||
>> > +                                 boot_cpu_has(X86_FEATURE_RTM);
>> > +
>> > +            rdmsrl(MSR_IA32_PERF_CAPABILITIES, caps);
>> 
>> This is guarded by a X86_FEATURE_PDCM check in Linux - why
>> would we not need the same here?
> 
> You're right, it should be. It also seems to be missing from
> core2_vpmu_init().

Feel free to take the liberty to fix it there at once (but please
briefly mention this as an independent change in the description).

>> Also I think this RDMSR should be performed once at boot, not every
>> time we come here.
> 
> I thought you might say that. It didn't seem obviously right to put
> this in boot_cpu_data -- is that what you suggest?

Why boot_cpu_data? Just an ordinary (possibly per-CPU) static
in vmx.c.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel