thr3ads.net - Xen devel - [PATCH] xen: handle paged gfn in wrmsr_hypervisor

If this information is useful, please help other people find it:
Share via:

Olaf Hering

2013-May-02 16:24 UTC

[PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1367511750 -7200
# Node ID 19777301f89653c640de73c6319c9d386a28327f
# Parent  9df019eef776d129c2abb130d1458914fe1ecac4
xen: handle paged gfn in wrmsr_hypervisor_regs

If xenpaging is started very early for a guest the gfn for the hypercall
page may be paged-out already. This leads to a guest crash:

...
(XEN) HVM10: Allocated Xen hypercall page at 169ff000
(XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
(XEN) HVM10: Detected Xen v4.3
(XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83 ff
ff 10 7c
(XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action 1.
(XEN) HVM11: HVM Loader
...

Handle paged gfns in wrmsr_hypervisor_regs and update callers to deal with the
new return code -EAGAIN.

Also update the gdprintk to handle a page value of NULL to avoid printing a
bogus MFN value.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 9df019eef776 -r 19777301f896 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
 
 static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
-    int ret;
+    int ret, retry = 0;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int sync = 0;
@@ -1682,14 +1682,15 @@ static int svm_msr_write_intercept(unsig
         if ( wrmsr_viridian_regs(msr, msr_content) )
             break;
 
-        wrmsr_hypervisor_regs(msr, msr_content);
+        ret = wrmsr_hypervisor_regs(msr, msr_content);
+        retry = ret == -EAGAIN;
         break;
     }
 
     if ( sync )
         svm_vmload(vmcb);
 
-    return X86EMUL_OKAY;
+    return retry ? X86EMUL_RETRY : X86EMUL_OKAY;
 
  gpf:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff -r 9df019eef776 -r 19777301f896 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2020,6 +2020,7 @@ void vmx_vlapic_msr_changed(struct vcpu 
 
 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
+    int ret = 0;
     struct vcpu *v = current;
 
     HVM_DBG_LOG(DBG_LEVEL_1, "ecx=%#x, msr_value=%#"PRIx64, msr,
msr_content);
@@ -2088,7 +2089,9 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    ret = wrmsr_hypervisor_regs(msr, msr_content);
+                    if ( ret == -EAGAIN )
+                        return X86EMUL_RETRY;
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;
diff -r 9df019eef776 -r 19777301f896 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -634,6 +634,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
         unsigned long gmfn = val >> 12;
         unsigned int idx  = val & 0xfff;
         struct page_info *page;
+        p2m_type_t t;
 
         if ( idx > 0 )
         {
@@ -643,15 +644,21 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
             return 0;
         }
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
                 put_page(page);
+            if ( p2m_is_paging(t) )
+            {
+                p2m_mem_paging_populate(d, gmfn);
+                return -EAGAIN;
+            }
+
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
-                     gmfn, page_to_mfn(page), base + idx);
+                     gmfn, page ? page_to_mfn(page) : -1UL, base + idx);
             return 0;
         }

Jan Beulich

2013-May-03 07:12 UTC

head link

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 02.05.13 at 18:24, Olaf Hering <olaf@aepfle.de> wrote:
> @@ -1682,14 +1682,15 @@ static int svm_msr_write_intercept(unsig
>          if ( wrmsr_viridian_regs(msr, msr_content) )
>              break;
>  
> -        wrmsr_hypervisor_regs(msr, msr_content);
> +        ret = wrmsr_hypervisor_regs(msr, msr_content);
> +        retry = ret == -EAGAIN;
If you add error handling, don''t constrain this to a single error code
please. For the case here, the easiest would appear to be a switch
converting to X86EMUL_OKAY, X86EMUL_RETRY, or
X86EMUL_UNHANDLEABLE. If the function had ways to fail before,
it would have been a bug anyway to not check the return value.
> @@ -2088,7 +2089,9 @@ static int vmx_msr_write_intercept(unsig
>              case HNDL_unhandled:
>                  if ( (vmx_write_guest_msr(msr, msr_content) != 0)
&&
>                       !is_last_branch_msr(msr) )
> -                    wrmsr_hypervisor_regs(msr, msr_content);
> +                    ret = wrmsr_hypervisor_regs(msr, msr_content);
> +                    if ( ret == -EAGAIN )
> +                        return X86EMUL_RETRY;
Similarly here, obviously.

Jan

Olaf Hering

2013-May-03 12:57 UTC

head link

[PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1367585741 -7200
# Node ID 219e97e92394eb8aea443144ae93e48d9cce9d87
# Parent  9df019eef776d129c2abb130d1458914fe1ecac4
xen: handle paged gfn in wrmsr_hypervisor_regs

If xenpaging is started very early for a guest the gfn for the hypercall
page may be paged-out already. This leads to a guest crash:

...
(XEN) HVM10: Allocated Xen hypercall page at 169ff000
(XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
(XEN) HVM10: Detected Xen v4.3
(XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83 ff
ff 10 7c
(XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action 1.
(XEN) HVM11: HVM Loader
...

Handle paged gfns in wrmsr_hypervisor_regs and update callers to deal with the
new return code -EAGAIN.

Also update the gdprintk to handle a page value of NULL to avoid printing a
bogus MFN value.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 9df019eef776 -r 219e97e92394 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
 
 static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
-    int ret;
+    int ret, result = X86EMUL_OKAY;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int sync = 0;
@@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
         if ( wrmsr_viridian_regs(msr, msr_content) )
             break;
 
-        wrmsr_hypervisor_regs(msr, msr_content);
+        ret = wrmsr_hypervisor_regs(msr, msr_content);
+        switch ( ret )
+        {
+            case -EAGAIN:
+                result = X86EMUL_RETRY;
+                break;
+            case 0:
+                result = X86EMUL_UNHANDLEABLE;
+                break;
+            default:
+                break;
+        }
         break;
     }
 
     if ( sync )
         svm_vmload(vmcb);
 
-    return X86EMUL_OKAY;
+    return result;
 
  gpf:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff -r 9df019eef776 -r 219e97e92394 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2020,6 +2020,7 @@ void vmx_vlapic_msr_changed(struct vcpu 
 
 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
+    int ret = 0;
     struct vcpu *v = current;
 
     HVM_DBG_LOG(DBG_LEVEL_1, "ecx=%#x, msr_value=%#"PRIx64, msr,
msr_content);
@@ -2088,7 +2089,16 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    ret = wrmsr_hypervisor_regs(msr, msr_content);
+                    switch ( ret )
+                    {
+                        case -EAGAIN:
+                            return X86EMUL_RETRY;
+                        case 0:
+                            return X86EMUL_UNHANDLEABLE;
+                        default:
+                            break;
+                    }
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;
diff -r 9df019eef776 -r 219e97e92394 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -634,6 +634,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
         unsigned long gmfn = val >> 12;
         unsigned int idx  = val & 0xfff;
         struct page_info *page;
+        p2m_type_t t;
 
         if ( idx > 0 )
         {
@@ -643,15 +644,22 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
             return 0;
         }
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
                 put_page(page);
+
+            if ( p2m_is_paging(t) )
+            {
+                p2m_mem_paging_populate(d, gmfn);
+                return -EAGAIN;
+            }
+
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
-                     gmfn, page_to_mfn(page), base + idx);
+                     gmfn, page ? page_to_mfn(page) : -1UL, base + idx);
             return 0;
         }

Olaf Hering

2013-May-03 12:58 UTC

head link

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

On Fri, May 03, Jan Beulich wrote:
> >>> On 02.05.13 at 18:24, Olaf Hering <olaf@aepfle.de>
wrote:
> > @@ -1682,14 +1682,15 @@ static int svm_msr_write_intercept(unsig
> >          if ( wrmsr_viridian_regs(msr, msr_content) )
> >              break;
> >  
> > -        wrmsr_hypervisor_regs(msr, msr_content);
> > +        ret = wrmsr_hypervisor_regs(msr, msr_content);
> > +        retry = ret == -EAGAIN;
> 
> If you add error handling, don''t constrain this to a single error
code
> please. For the case here, the easiest would appear to be a switch
> converting to X86EMUL_OKAY, X86EMUL_RETRY, or
> X86EMUL_UNHANDLEABLE. If the function had ways to fail before,
> it would have been a bug anyway to not check the return value.
I just sent v2 of this patch.


Olaf

Olaf Hering

2013-May-03 13:40 UTC

head link

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

On Fri, May 03, Olaf Hering wrote:
> On Fri, May 03, Jan Beulich wrote:
> 
> > >>> On 02.05.13 at 18:24, Olaf Hering <olaf@aepfle.de>
wrote:
> > > @@ -1682,14 +1682,15 @@ static int svm_msr_write_intercept(unsig
> > >          if ( wrmsr_viridian_regs(msr, msr_content) )
> > >              break;
> > >  
> > > -        wrmsr_hypervisor_regs(msr, msr_content);
> > > +        ret = wrmsr_hypervisor_regs(msr, msr_content);
> > > +        retry = ret == -EAGAIN;
> > 
> > If you add error handling, don''t constrain this to a single
error code
> > please. For the case here, the easiest would appear to be a switch
> > converting to X86EMUL_OKAY, X86EMUL_RETRY, or
> > X86EMUL_UNHANDLEABLE. If the function had ways to fail before,
> > it would have been a bug anyway to not check the return value.
> 
> I just sent v2 of this patch.
My change v2 causes a boot failure, the return value 0 is not handled
correctly.
Did you really mean to translate ret == 0 to X86EMUL_UNHANDLEABLE?

Olaf

+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2020,6 +2020,7 @@ void vmx_vlapic_msr_changed(struct vcpu 

 static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
+    int ret = 0;
     struct vcpu *v = current;

     HVM_DBG_LOG(DBG_LEVEL_1, "ecx=%#x, msr_value=%#"PRIx64, msr,
msr_content);
@@ -2088,7 +2089,17 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    ret = wrmsr_hypervisor_regs(msr, msr_content);
+                    printk("%s(%u) msr %x mc %lx r %d\n", __func__,
__LINE__, msr, msr_content, ret);
+                    switch ( ret )
+                    {
+                        case -EAGAIN:
+                            return X86EMUL_RETRY;
+                        case 0:
+                            return X86EMUL_UNHANDLEABLE;
+                        default:
+                            break;
+                    }
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;

satriani login: (XEN) HVM2: HVM Loader
(XEN) vmx_msr_write_intercept(2093) msr 40000000 mc 80000 r 1
(XEN) HVM2: Detected Xen v4.3.26961-20130503
(XEN) HVM2: Xenbus rings @0xfeffc000, event channel 6
(XEN) HVM2: System requested SeaBIOS
(XEN) HVM2: CPU speed is 2926 MHz
(XEN) irq.c:270: Dom2 PCI link 0 changed 0 -> 5
(XEN) HVM2: PCI-ISA link 0 routed to IRQ5
(XEN) irq.c:270: Dom2 PCI link 1 changed 0 -> 10
(XEN) HVM2: PCI-ISA link 1 routed to IRQ10
(XEN) irq.c:270: Dom2 PCI link 2 changed 0 -> 11
(XEN) HVM2: PCI-ISA link 2 routed to IRQ11
(XEN) irq.c:270: Dom2 PCI link 3 changed 0 -> 5
(XEN) HVM2: PCI-ISA link 3 routed to IRQ5
(XEN) HVM2: pci dev 01:2 INTD->IRQ5
(XEN) HVM2: pci dev 01:3 INTA->IRQ10
(XEN) HVM2: pci dev 03:0 INTA->IRQ5
(XEN) HVM2: pci dev 02:0 bar 10 size lx: 02000000
(XEN) HVM2: pci dev 03:0 bar 14 size lx: 01000000
(XEN) HVM2: pci dev 02:0 bar 30 size lx: 00010000
(XEN) HVM2: pci dev 02:0 bar 14 size lx: 00001000
(XEN) HVM2: pci dev 03:0 bar 10 size lx: 00000100
(XEN) HVM2: pci dev 01:2 bar 20 size lx: 00000020
(XEN) HVM2: pci dev 01:1 bar 20 size lx: 00000010
(XEN) HVM2: Multiprocessor initialisation:
(XEN) HVM2:  - CPU0 ... 40-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM2:  - CPU1 ... 40-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM2:  - CPU2 ... 40-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM2:  - CPU3 ... 40-bit phys ... fixed MTRRs ... var MTRRs [2/8] ...
done.
(XEN) HVM2: Testing HVM environment:
(XEN) HVM2:  - REP INSB across page boundaries ... passed
(XEN) HVM2:  - GS base MSRs and SWAPGS ... passed
(XEN) HVM2: Passed 2 of 2 tests
(XEN) HVM2: Writing SMBIOS tables ...
(XEN) HVM2: Loading SeaBIOS ...
(XEN) HVM2: Creating MP tables ...
(XEN) HVM2: Loading ACPI ...
(XEN) HVM2: vm86 TSS at fc00a100
(XEN) HVM2: BIOS map:
(XEN) HVM2:  10000-100d3: Scratch space
(XEN) HVM2:  e0000-fffff: Main BIOS
(XEN) HVM2: E820 table:
(XEN) HVM2:  [00]: 00000000:00000000 - 00000000:000a0000: RAM
(XEN) HVM2:  HOLE: 00000000:000a0000 - 00000000:000e0000
(XEN) HVM2:  [01]: 00000000:000e0000 - 00000000:00100000: RESERVED
(XEN) HVM2:  [02]: 00000000:00100000 - 00000000:16a00000: RAM
(XEN) HVM2:  HOLE: 00000000:16a00000 - 00000000:fc000000
(XEN) HVM2:  [03]: 00000000:fc000000 - 00000001:00000000: RESERVED
(XEN) HVM2: Invoking SeaBIOS ...
(XEN) HVM2: SeaBIOS (version ?-20130503_132523-bax)
(XEN) HVM2:
(XEN) HVM2: Found Xen hypervisor signature at 40000000
(XEN) HVM2: xen: copy e820...
(XEN) HVM2: Ram Size=0x16a00000 (0x0000000000000000 high)
(XEN) HVM2: Relocating low data from 0x000e2490 to 0x000ef790 (size 2156)
(XEN) HVM2: Relocating init from 0x000e2cfc to 0x169e20f0 (size 56804)
(XEN) HVM2: CPU Mhz=2926
(XEN) HVM2: Found 7 PCI devices (max PCI bus is 00)
(XEN) HVM2: Allocated Xen hypercall page at 169ff000
(XEN) vmx_msr_write_intercept(2093) msr 40000000 mc 169ff000 r 1
(XEN) HVM2: Detected Xen v4.3.26961-20130503
(XEN) HVM2: Found 4 cpu(s) max supported 4 cpu(s)
(XEN) HVM2: xen: copy BIOS tables...
(XEN) HVM2: Copying SMBIOS entry point from 0x00010010 to 0x000fdb10
(XEN) HVM2: Copying MPTABLE from 0xfc0011c0/fc0011d0 to 0x000fd9f0
(XEN) HVM2: Copying PIR from 0x00010030 to 0x000fd970
(XEN) HVM2: Copying ACPI RSDP from 0x000100b0 to 0x000fd940
(XEN) HVM2: Scan for VGA option rom
(XEN) HVM2: Running option rom at c000:0003
(XEN) stdvga.c:147:d2 entering stdvga and caching modes
(XEN) HVM2: Turning on vga text mode console
(XEN) HVM2: SeaBIOS (version ?-20130503_132523-bax)
(XEN) HVM2:
(XEN) HVM2: UHCI init on dev 00:01.2 (io=c100)
(XEN) HVM2: Found 1 lpt ports
(XEN) HVM2: Found 1 serial ports
(XEN) HVM2: ATA controller 1 at 1f0/3f4/c120 (irq 14 dev 9)
(XEN) HVM2: ATA controller 2 at 170/374/c128 (irq 15 dev 9)
(XEN) HVM2: ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (10240 MiBytes)
(XEN) HVM2: Searching bootorder for: /pci@i0cf8/*@1,1/drive@0/disk@0
(XEN) HVM2: DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD]
(XEN) HVM2: Searching bootorder for: /pci@i0cf8/*@1,1/drive@1/disk@0
(XEN) HVM2: PS2 keyboard initialized
(XEN) HVM2: All threads complete.
(XEN) HVM2: Scan for option roms
(XEN) HVM2: Press F12 for boot menu.
(XEN) HVM2:
(XEN) HVM2: drive 0x000fd8f0: PCHS=16383/16/63 translation=lba LCHS=1024/255/63
s=20971520
(XEN) HVM2:
(XEN) HVM2: Space available for UMB: 000c9000-000ee800
(XEN) HVM2: Returned 61440 bytes of ZoneHigh
(XEN) HVM2: e820 map has 6 items:
(XEN) HVM2:   0: 0000000000000000 - 000000000009fc00 = 1 RAM
(XEN) HVM2:   1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED
(XEN) HVM2:   2: 00000000000f0000 - 0000000000100000 = 2 RESERVED
(XEN) HVM2:   3: 0000000000100000 - 00000000169ff000 = 1 RAM
(XEN) HVM2:   4: 00000000169ff000 - 0000000016a00000 = 2 RESERVED
(XEN) HVM2:   5: 00000000fc000000 - 0000000100000000 = 2 RESERVED
(XEN) HVM2: enter handle_19:
(XEN) HVM2:   NULL
(XEN) HVM2: Booting from DVD/CD...
(XEN) HVM2: Booting from 0000:7c00
(XEN) stdvga.c:151:d2 leaving stdvga
(XEN) stdvga.c:147:d2 entering stdvga and caching modes
(XEN) stdvga.c:151:d2 leaving stdvga
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
(XEN) vmx_msr_write_intercept(2093) msr 8b mc 0 r 0
...

Jan Beulich

2013-May-03 13:53 UTC

head link

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 15:40, Olaf Hering <olaf@aepfle.de> wrote:
> On Fri, May 03, Olaf Hering wrote:
> 
>> On Fri, May 03, Jan Beulich wrote:
>> 
>> > >>> On 02.05.13 at 18:24, Olaf Hering
<olaf@aepfle.de> wrote:
>> > > @@ -1682,14 +1682,15 @@ static int
svm_msr_write_intercept(unsig
>> > >          if ( wrmsr_viridian_regs(msr, msr_content) )
>> > >              break;
>> > >  
>> > > -        wrmsr_hypervisor_regs(msr, msr_content);
>> > > +        ret = wrmsr_hypervisor_regs(msr, msr_content);
>> > > +        retry = ret == -EAGAIN;
>> > 
>> > If you add error handling, don''t constrain this to a
single error code
>> > please. For the case here, the easiest would appear to be a switch
>> > converting to X86EMUL_OKAY, X86EMUL_RETRY, or
>> > X86EMUL_UNHANDLEABLE. If the function had ways to fail before,
>> > it would have been a bug anyway to not check the return value.
>> 
>> I just sent v2 of this patch.
> 
> My change v2 causes a boot failure, the return value 0 is not handled
> correctly.
> Did you really mean to translate ret == 0 to X86EMUL_UNHANDLEABLE?
Of course I didn''t - I merely enumerated the values that I''d
expect
to result, with no meaning implied from their ordering.

Jan

Jan Beulich

2013-May-03 13:58 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 14:57, Olaf Hering <olaf@aepfle.de> wrote:
> @@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
>          if ( wrmsr_viridian_regs(msr, msr_content) )
>              break;
>  
> -        wrmsr_hypervisor_regs(msr, msr_content);
> +        ret = wrmsr_hypervisor_regs(msr, msr_content);
> +        switch ( ret )
> +        {
> +            case -EAGAIN:
> +                result = X86EMUL_RETRY;
> +                break;
> +            case 0:
> +                result = X86EMUL_UNHANDLEABLE;
> +                break;
> +            default:
> +                break;
As you had already noticed the hard way - case 0 and default of
course need to be switched (0 -> okay, anything else ->
unhandleable).

Jan

Olaf Hering

2013-May-03 14:11 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

On Fri, May 03, Jan Beulich wrote:
> >>> On 03.05.13 at 14:57, Olaf Hering <olaf@aepfle.de>
wrote:
> > @@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
> >          if ( wrmsr_viridian_regs(msr, msr_content) )
> >              break;
> >  
> > -        wrmsr_hypervisor_regs(msr, msr_content);
> > +        ret = wrmsr_hypervisor_regs(msr, msr_content);
> > +        switch ( ret )
> > +        {
> > +            case -EAGAIN:
> > +                result = X86EMUL_RETRY;
> > +                break;
> > +            case 0:
> > +                result = X86EMUL_UNHANDLEABLE;
> > +                break;
> > +            default:
> > +                break;
> 
> As you had already noticed the hard way - case 0 and default of
> course need to be switched (0 -> okay, anything else ->
> unhandleable).
I dont follow.
ret == 1 looks like success to me, ret == 0 some sort of failure.


Olaf

Jan Beulich

2013-May-03 14:24 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 16:11, Olaf Hering <olaf@aepfle.de> wrote:
> On Fri, May 03, Jan Beulich wrote:
> 
>> >>> On 03.05.13 at 14:57, Olaf Hering <olaf@aepfle.de>
wrote:
>> > @@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
>> >          if ( wrmsr_viridian_regs(msr, msr_content) )
>> >              break;
>> >  
>> > -        wrmsr_hypervisor_regs(msr, msr_content);
>> > +        ret = wrmsr_hypervisor_regs(msr, msr_content);
>> > +        switch ( ret )
>> > +        {
>> > +            case -EAGAIN:
>> > +                result = X86EMUL_RETRY;
>> > +                break;
>> > +            case 0:
>> > +                result = X86EMUL_UNHANDLEABLE;
>> > +                break;
>> > +            default:
>> > +                break;
>> 
>> As you had already noticed the hard way - case 0 and default of
>> course need to be switched (0 -> okay, anything else ->
>> unhandleable).
> 
> I dont follow.
> ret == 1 looks like success to me, ret == 0 some sort of failure.
Let me check again... Positive values (or 1 in particular) mean
"handled", 0 means not handled, negative values are errors.

Jan

Keir Fraser

2013-May-03 14:26 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

On 03/05/2013 14:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 03.05.13 at 14:57, Olaf Hering <olaf@aepfle.de> wrote:
>> @@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
>>          if ( wrmsr_viridian_regs(msr, msr_content) )
>>              break;
>>  
>> -        wrmsr_hypervisor_regs(msr, msr_content);
>> +        ret = wrmsr_hypervisor_regs(msr, msr_content);
>> +        switch ( ret )
>> +        {
>> +            case -EAGAIN:
>> +                result = X86EMUL_RETRY;
>> +                break;
>> +            case 0:
>> +                result = X86EMUL_UNHANDLEABLE;
>> +                break;
>> +            default:
>> +                break;
> 
> As you had already noticed the hard way - case 0 and default of
> course need to be switched (0 -> okay, anything else ->
> unhandleable).
No!

Actually anything other than -EAGAIN should be handled here as
''okay''. In
fact the return codes from wrmsr_hypervisor_regs are going to be a bit of a
mess if we''re not careful.

I suggest the following return codes:
 0: not handled
 1: handled
 -EINVAL: error during handling
 -EAGAIN: retry

The HVM callers should then handle as follows:
 -EAGAIN: rc = X86EMUL_RETRY
 -EINVAL: goto gp_fault
 0: try other msr handlers (if any)
 1: we''re done, return X86EMUL_OKAY

Does that make sense?

 -- Keir
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Jan Beulich

2013-May-03 14:31 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 16:26, Keir Fraser <keir.xen@gmail.com> wrote:
> On 03/05/2013 14:58, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>>> On 03.05.13 at 14:57, Olaf Hering <olaf@aepfle.de>
wrote:
>>> @@ -1682,14 +1682,25 @@ static int svm_msr_write_intercept(unsig
>>>          if ( wrmsr_viridian_regs(msr, msr_content) )
>>>              break;
>>>  
>>> -        wrmsr_hypervisor_regs(msr, msr_content);
>>> +        ret = wrmsr_hypervisor_regs(msr, msr_content);
>>> +        switch ( ret )
>>> +        {
>>> +            case -EAGAIN:
>>> +                result = X86EMUL_RETRY;
>>> +                break;
>>> +            case 0:
>>> +                result = X86EMUL_UNHANDLEABLE;
>>> +                break;
>>> +            default:
>>> +                break;
>> 
>> As you had already noticed the hard way - case 0 and default of
>> course need to be switched (0 -> okay, anything else ->
>> unhandleable).
> 
> No!
> 
> Actually anything other than -EAGAIN should be handled here as
''okay''. In
> fact the return codes from wrmsr_hypervisor_regs are going to be a bit of a
> mess if we''re not careful.
> 
> I suggest the following return codes:
>  0: not handled
>  1: handled
>  -EINVAL: error during handling
>  -EAGAIN: retry
> 
> The HVM callers should then handle as follows:
>  -EAGAIN: rc = X86EMUL_RETRY
>  -EINVAL: goto gp_fault
>  0: try other msr handlers (if any)
>  1: we''re done, return X86EMUL_OKAY
> 
> Does that make sense?
Sure - you may have seen my later reply where I also notide
this mistake in my first response to Olaf.

The only thing I''d like to ensure is to not constrain the code to
specific error codes - any negative value other than -EAGAIN
should result in #GP (or whatever a suitable action is) imo.

Jan

Olaf Hering

2013-May-03 15:17 UTC

head link

[PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1367593457 -7200
# Node ID b8af60cf8282bfddb13cc10e4ffaf0c396a15104
# Parent  9df019eef776d129c2abb130d1458914fe1ecac4
xen: handle paged gfn in wrmsr_hypervisor_regs

If xenpaging is started very early for a guest the gfn for the hypercall
page may be paged-out already. This leads to a guest crash:

...
(XEN) HVM10: Allocated Xen hypercall page at 169ff000
(XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
(XEN) HVM10: Detected Xen v4.3
(XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83 ff
ff 10 7c
(XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action 1.
(XEN) HVM11: HVM Loader
...

Update return codes of wrmsr_hypervisor_regs, update callers to deal
with the new return codes:
 0: not handled
 1: handled
 -EINVAL: error during handling
 -EAGAIN: retry


Also update the gdprintk to handle a page value of NULL to avoid
printing a bogus MFN value. Update also computing of MSR value in
gdprintk, the idx was always zero.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
 
 static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
-    int ret;
+    int ret, result = X86EMUL_OKAY;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int sync = 0;
@@ -1682,14 +1682,24 @@ static int svm_msr_write_intercept(unsig
         if ( wrmsr_viridian_regs(msr, msr_content) )
             break;
 
-        wrmsr_hypervisor_regs(msr, msr_content);
+        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+        {
+            case -EAGAIN:
+                result = X86EMUL_RETRY;
+                break;
+            case 0:
+            case 1:
+                break;
+            default:
+	        goto gpf;
+        }
         break;
     }
 
     if ( sync )
         svm_vmload(vmcb);
 
-    return X86EMUL_OKAY;
+    return result;
 
  gpf:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2088,7 +2088,16 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+                    {
+                        case -EAGAIN:
+                            return X86EMUL_RETRY;
+                        case 0:
+                        case 1:
+                            break;
+                        default:
+                            goto gp_fault;
+                    }
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;
diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -634,25 +634,33 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
         unsigned long gmfn = val >> 12;
         unsigned int idx  = val & 0xfff;
         struct page_info *page;
+        p2m_type_t t;
 
         if ( idx > 0 )
         {
             gdprintk(XENLOG_WARNING,
                      "Out of range index %u to MSR %08x\n",
                      idx, 0x40000000);
-            return 0;
+            return -EINVAL;
         }
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
                 put_page(page);
+
+            if ( p2m_is_paging(t) )
+            {
+                p2m_mem_paging_populate(d, gmfn);
+                return -EAGAIN;
+            }
+
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
-                     gmfn, page_to_mfn(page), base + idx);
-            return 0;
+                     gmfn, page ? page_to_mfn(page) : -1UL, base);
+            return -EINVAL;
         }
 
         hypercall_page = __map_domain_page(page);
@@ -2490,7 +2498,7 @@ static int emulate_privileged_op(struct 
                 goto fail;
             break;
         default:
-            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
+            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
                 break;
 
             rc = vmce_wrmsr(regs->ecx, msr_content);

Olaf Hering

2013-May-03 15:19 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

On Fri, May 03, Jan Beulich wrote:
> >>> On 03.05.13 at 16:26, Keir Fraser <keir.xen@gmail.com>
wrote:
> > I suggest the following return codes:
> >  0: not handled
> >  1: handled
> >  -EINVAL: error during handling
> >  -EAGAIN: retry
> > 
> > The HVM callers should then handle as follows:
> >  -EAGAIN: rc = X86EMUL_RETRY
> >  -EINVAL: goto gp_fault
> >  0: try other msr handlers (if any)
> >  1: we''re done, return X86EMUL_OKAY
> > 
> > Does that make sense?
> 
> Sure - you may have seen my later reply where I also notide
> this mistake in my first response to Olaf.
I sent v3 of the patch. I had to guess which ''return 0'' is
supposed to
be an error.

Olaf

Jan Beulich

2013-May-03 15:30 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 17:17, Olaf Hering <olaf@aepfle.de> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
>  
>  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>  {
> -    int ret;
> +    int ret, result = X86EMUL_OKAY;
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>      int sync = 0;
> @@ -1682,14 +1682,24 @@ static int svm_msr_write_intercept(unsig
>          if ( wrmsr_viridian_regs(msr, msr_content) )
>              break;
>  
> -        wrmsr_hypervisor_regs(msr, msr_content);
> +        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
> +        {
> +            case -EAGAIN:
> +                result = X86EMUL_RETRY;
> +                break;
> +            case 0:
> +            case 1:
> +                break;
> +            default:
> +	        goto gpf;
> +        }
>          break;
>      }
>  
>      if ( sync )
>          svm_vmload(vmcb);
>  
> -    return X86EMUL_OKAY;
> +    return result;
>  
>   gpf:
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2088,7 +2088,16 @@ static int vmx_msr_write_intercept(unsig
>              case HNDL_unhandled:
>                  if ( (vmx_write_guest_msr(msr, msr_content) != 0)
&&
>                       !is_last_branch_msr(msr) )
> -                    wrmsr_hypervisor_regs(msr, msr_content);
> +                    switch ( wrmsr_hypervisor_regs(msr, msr_content) )
> +                    {
> +                        case -EAGAIN:
> +                            return X86EMUL_RETRY;
> +                        case 0:
> +                        case 1:
> +                            break;
> +                        default:
> +                            goto gp_fault;
> +                    }
>                  break;
>              case HNDL_exception_raised:
>                  return X86EMUL_EXCEPTION;
Apart from formatting things look okay up to here.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -634,25 +634,33 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
>          unsigned long gmfn = val >> 12;
>          unsigned int idx  = val & 0xfff;
>          struct page_info *page;
> +        p2m_type_t t;
>  
>          if ( idx > 0 )
>          {
>              gdprintk(XENLOG_WARNING,
>                       "Out of range index %u to MSR %08x\n",
>                       idx, 0x40000000);
> -            return 0;
> +            return -EINVAL;
But I''d stay away from converting to actual errors both here ...
>          }
>  
> -        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>  
>          if ( !page || !get_page_type(page, PGT_writable_page) )
>          {
>              if ( page )
>                  put_page(page);
> +
> +            if ( p2m_is_paging(t) )
> +            {
> +                p2m_mem_paging_populate(d, gmfn);
> +                return -EAGAIN;
> +            }
> +
>              gdprintk(XENLOG_WARNING,
>                       "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
> -                     gmfn, page_to_mfn(page), base + idx);
> -            return 0;
> +                     gmfn, page ? page_to_mfn(page) : -1UL, base);
> +            return -EINVAL;
... and here. If at all these ought to go into a separate patch
(which we''d likely postpone until after 4.3).

Jan
>          }
>  
>          hypercall_page = __map_domain_page(page);
> @@ -2490,7 +2498,7 @@ static int emulate_privileged_op(struct 
>                  goto fail;
>              break;
>          default:
> -            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
> +            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
>                  break;
>  
>              rc = vmce_wrmsr(regs->ecx, msr_content);

Olaf Hering

2013-May-03 15:43 UTC

head link

[PATCH v4] xen: handle paged gfn in wrmsr_hypervisor_regs

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1367595698 -7200
# Node ID 4b7f3f754c015e0c2012d4b8ef4faf89ba18a3bf
# Parent  9df019eef776d129c2abb130d1458914fe1ecac4
xen: handle paged gfn in wrmsr_hypervisor_regs

If xenpaging is started very early for a guest the gfn for the hypercall
page may be paged-out already. This leads to a guest crash:

...
(XEN) HVM10: Allocated Xen hypercall page at 169ff000
(XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
(XEN) HVM10: Detected Xen v4.3
(XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83 ff
ff 10 7c
(XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action 1.
(XEN) HVM11: HVM Loader
...

Update return codes of wrmsr_hypervisor_regs, update callers to deal
with the new return codes:
 0: not handled
 1: handled
 -EINVAL: error during handling
 -EAGAIN: retry


Also update the gdprintk to handle a page value of NULL to avoid
printing a bogus MFN value. Update also computing of MSR value in
gdprintk, the idx was always zero.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 9df019eef776 -r 4b7f3f754c01 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
 
 static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
-    int ret;
+    int ret, result = X86EMUL_OKAY;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int sync = 0;
@@ -1682,14 +1682,24 @@ static int svm_msr_write_intercept(unsig
         if ( wrmsr_viridian_regs(msr, msr_content) )
             break;
 
-        wrmsr_hypervisor_regs(msr, msr_content);
+        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+        {
+            case -EAGAIN:
+                result = X86EMUL_RETRY;
+                break;
+            case 0:
+            case 1:
+                break;
+            default:
+                goto gpf;
+        }
         break;
     }
 
     if ( sync )
         svm_vmload(vmcb);
 
-    return X86EMUL_OKAY;
+    return result;
 
  gpf:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff -r 9df019eef776 -r 4b7f3f754c01 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2088,7 +2088,16 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+                    {
+                        case -EAGAIN:
+                            return X86EMUL_RETRY;
+                        case 0:
+                        case 1:
+                            break;
+                        default:
+                            goto gp_fault;
+                    }
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;
diff -r 9df019eef776 -r 4b7f3f754c01 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -634,6 +634,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
         unsigned long gmfn = val >> 12;
         unsigned int idx  = val & 0xfff;
         struct page_info *page;
+        p2m_type_t t;
 
         if ( idx > 0 )
         {
@@ -643,15 +644,22 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
             return 0;
         }
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
                 put_page(page);
+
+            if ( p2m_is_paging(t) )
+            {
+                p2m_mem_paging_populate(d, gmfn);
+                return -EAGAIN;
+            }
+
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
-                     gmfn, page_to_mfn(page), base + idx);
+                     gmfn, page ? page_to_mfn(page) : -1UL, base);
             return 0;
         }
 
@@ -2490,7 +2498,7 @@ static int emulate_privileged_op(struct 
                 goto fail;
             break;
         default:
-            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
+            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
                 break;
 
             rc = vmce_wrmsr(regs->ecx, msr_content);

Olaf Hering

2013-May-03 15:48 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

On Fri, May 03, Jan Beulich wrote:

> Apart from formatting things look okay up to here.
Did you mean just the single tab in svm_msr_write_intercept?
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -634,25 +634,33 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
> >          unsigned long gmfn = val >> 12;
> >          unsigned int idx  = val & 0xfff;
> >          struct page_info *page;
> > +        p2m_type_t t;
> >  
> >          if ( idx > 0 )
> >          {
> >              gdprintk(XENLOG_WARNING,
> >                       "Out of range index %u to MSR %08x\n",
> >                       idx, 0x40000000);
> > -            return 0;
> > +            return -EINVAL;
> 
> But I''d stay away from converting to actual errors both here ...
I sent v4 with the -EINVAL removed.


Olaf

Jan Beulich

2013-May-03 15:53 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 17:48, Olaf Hering <olaf@aepfle.de> wrote:
> On Fri, May 03, Jan Beulich wrote:
>> Apart from formatting things look okay up to here.
> 
> Did you mean just the single tab in svm_msr_write_intercept?
That and the too deep indentation of the case labels within the
new switch blocks.

Jan

Keir Fraser

2013-May-03 15:55 UTC

head link

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

On 03/05/2013 16:19, "Olaf Hering" <olaf@aepfle.de> wrote:
> On Fri, May 03, Jan Beulich wrote:
> 
>>>>> On 03.05.13 at 16:26, Keir Fraser
<keir.xen@gmail.com> wrote:
>>> I suggest the following return codes:
>>>  0: not handled
>>>  1: handled
>>>  -EINVAL: error during handling
>>>  -EAGAIN: retry
>>> 
>>> The HVM callers should then handle as follows:
>>>  -EAGAIN: rc = X86EMUL_RETRY
>>>  -EINVAL: goto gp_fault
>>>  0: try other msr handlers (if any)
>>>  1: we''re done, return X86EMUL_OKAY
>>> 
>>> Does that make sense?
>> 
>> Sure - you may have seen my later reply where I also notide
>> this mistake in my first response to Olaf.
> 
> I sent v3 of the patch. I had to guess which ''return 0''
is supposed to
> be an error.
You guessed correctly ;)
> Olaf

Keir Fraser

2013-May-03 15:55 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

On 03/05/2013 16:17, "Olaf Hering" <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1367593457 -7200
> # Node ID b8af60cf8282bfddb13cc10e4ffaf0c396a15104
> # Parent  9df019eef776d129c2abb130d1458914fe1ecac4
> xen: handle paged gfn in wrmsr_hypervisor_regs
Acked-by: Keir Fraser <keir@xen.org>
> If xenpaging is started very early for a guest the gfn for the hypercall
> page may be paged-out already. This leads to a guest crash:
> 
> ...
> (XEN) HVM10: Allocated Xen hypercall page at 169ff000
> (XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
> (XEN) HVM10: Detected Xen v4.3
> (XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83
ff
> ff 10 7c
> (XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action
1.
> (XEN) HVM11: HVM Loader
> ...
> 
> Update return codes of wrmsr_hypervisor_regs, update callers to deal
> with the new return codes:
>  0: not handled
>  1: handled
>  -EINVAL: error during handling
>  -EAGAIN: retry
> 
> 
> Also update the gdprintk to handle a page value of NULL to avoid
> printing a bogus MFN value. Update also computing of MSR value in
> gdprintk, the idx was always zero.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
>  
>  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>  {
> -    int ret;
> +    int ret, result = X86EMUL_OKAY;
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>      int sync = 0;
> @@ -1682,14 +1682,24 @@ static int svm_msr_write_intercept(unsig
>          if ( wrmsr_viridian_regs(msr, msr_content) )
>              break;
>  
> -        wrmsr_hypervisor_regs(msr, msr_content);
> +        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
> +        {
> +            case -EAGAIN:
> +                result = X86EMUL_RETRY;
> +                break;
> +            case 0:
> +            case 1:
> +                break;
> +            default:
> +         goto gpf;
> +        }
>          break;
>      }
>  
>      if ( sync )
>          svm_vmload(vmcb);
>  
> -    return X86EMUL_OKAY;
> +    return result;
>  
>   gpf:
>      hvm_inject_hw_exception(TRAP_gp_fault, 0);
> diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2088,7 +2088,16 @@ static int vmx_msr_write_intercept(unsig
>              case HNDL_unhandled:
>                  if ( (vmx_write_guest_msr(msr, msr_content) != 0)
&&
>                       !is_last_branch_msr(msr) )
> -                    wrmsr_hypervisor_regs(msr, msr_content);
> +                    switch ( wrmsr_hypervisor_regs(msr, msr_content) )
> +                    {
> +                        case -EAGAIN:
> +                            return X86EMUL_RETRY;
> +                        case 0:
> +                        case 1:
> +                            break;
> +                        default:
> +                            goto gp_fault;
> +                    }
>                  break;
>              case HNDL_exception_raised:
>                  return X86EMUL_EXCEPTION;
> diff -r 9df019eef776 -r b8af60cf8282 xen/arch/x86/traps.c
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -634,25 +634,33 @@ int wrmsr_hypervisor_regs(uint32_t idx,
>          unsigned long gmfn = val >> 12;
>          unsigned int idx  = val & 0xfff;
>          struct page_info *page;
> +        p2m_type_t t;
>  
>          if ( idx > 0 )
>          {
>              gdprintk(XENLOG_WARNING,
>                       "Out of range index %u to MSR %08x\n",
>                       idx, 0x40000000);
> -            return 0;
> +            return -EINVAL;
>          }
>  
> -        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>  
>          if ( !page || !get_page_type(page, PGT_writable_page) )
>          {
>              if ( page )
>                  put_page(page);
> +
> +            if ( p2m_is_paging(t) )
> +            {
> +                p2m_mem_paging_populate(d, gmfn);
> +                return -EAGAIN;
> +            }
> +
>              gdprintk(XENLOG_WARNING,
>                       "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
> -                     gmfn, page_to_mfn(page), base + idx);
> -            return 0;
> +                     gmfn, page ? page_to_mfn(page) : -1UL, base);
> +            return -EINVAL;
>          }
>  
>          hypercall_page = __map_domain_page(page);
> @@ -2490,7 +2498,7 @@ static int emulate_privileged_op(struct
>                  goto fail;
>              break;
>          default:
> -            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
> +            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
>                  break;
>  
>              rc = vmce_wrmsr(regs->ecx, msr_content);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Keir Fraser

2013-May-03 15:58 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

On 03/05/2013 16:30, "Jan Beulich" <JBeulich@suse.com> wrote:
>>          if ( idx > 0 )
>>          {
>>              gdprintk(XENLOG_WARNING,
>>                       "Out of range index %u to MSR %08x\n",
>>                       idx, 0x40000000);
>> -            return 0;
>> +            return -EINVAL;
> 
> But I''d stay away from converting to actual errors both here ...
> 
>>          }
>>  
>> -        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
>> +        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>>  
>>          if ( !page || !get_page_type(page, PGT_writable_page) )
>>          {
>>              if ( page )
>>                  put_page(page);
>> +
>> +            if ( p2m_is_paging(t) )
>> +            {
>> +                p2m_mem_paging_populate(d, gmfn);
>> +                return -EAGAIN;
>> +            }
>> +
>>              gdprintk(XENLOG_WARNING,
>>                       "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
>> -                     gmfn, page_to_mfn(page), base + idx);
>> -            return 0;
>> +                     gmfn, page ? page_to_mfn(page) : -1UL, base);
>> +            return -EINVAL;
> 
> ... and here. If at all these ought to go into a separate patch
> (which we''d likely postpone until after 4.3).
But they are errors?

Could agree with postponing to post-4.3 though.

 -- Keir

Jan Beulich

2013-May-03 16:03 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 17:58, Keir Fraser <keir.xen@gmail.com> wrote:
> On 03/05/2013 16:30, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>          if ( idx > 0 )
>>>          {
>>>              gdprintk(XENLOG_WARNING,
>>>                       "Out of range index %u to MSR
%08x\n",
>>>                       idx, 0x40000000);
>>> -            return 0;
>>> +            return -EINVAL;
>> 
>> But I''d stay away from converting to actual errors both here
...
>> 
>>>          }
>>>  
>>> -        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
>>> +        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>>>  
>>>          if ( !page || !get_page_type(page, PGT_writable_page) )
>>>          {
>>>              if ( page )
>>>                  put_page(page);
>>> +
>>> +            if ( p2m_is_paging(t) )
>>> +            {
>>> +                p2m_mem_paging_populate(d, gmfn);
>>> +                return -EAGAIN;
>>> +            }
>>> +
>>>              gdprintk(XENLOG_WARNING,
>>>                       "Bad GMFN %lx (MFN %lx) to MSR
%08x\n",
>>> -                     gmfn, page_to_mfn(page), base + idx);
>>> -            return 0;
>>> +                     gmfn, page ? page_to_mfn(page) : -1UL, base);
>>> +            return -EINVAL;
>> 
>> ... and here. If at all these ought to go into a separate patch
>> (which we''d likely postpone until after 4.3).
> 
> But they are errors?
Right. But they weren''t handled as such so far, and doing the
conversion isn''t the purpose of the patch.
> Could agree with postponing to post-4.3 though.
Hence the request to split off that part of the patch. I''d be happy
to apply that after the trees got branched.

Jan

Keir Fraser

2013-May-03 16:06 UTC

head link

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

On 03/05/2013 17:03, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> ... and here. If at all these ought to go into a separate patch
>>> (which we''d likely postpone until after 4.3).
>> 
>> But they are errors?
> 
> Right. But they weren''t handled as such so far, and doing the
> conversion isn''t the purpose of the patch.
Okay, I wasn''t sure how dismissive your "if at all" was. :)
>> Could agree with postponing to post-4.3 though.
> 
> Hence the request to split off that part of the patch. I''d be
happy
> to apply that after the trees got branched.
Fine.

 -- Keir

Olaf Hering

2013-May-03 16:29 UTC

head link

[PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1367598534 -7200
# Node ID e04344e05ce4ab608df4570d7c081770cc40f3cf
# Parent  9df019eef776d129c2abb130d1458914fe1ecac4
xen: handle paged gfn in wrmsr_hypervisor_regs

If xenpaging is started very early for a guest the gfn for the hypercall
page may be paged-out already. This leads to a guest crash:

...
(XEN) HVM10: Allocated Xen hypercall page at 169ff000
(XEN) traps.c:654:d10 Bad GMFN 169ff (MFN 3e900000000) to MSR 40000000
(XEN) HVM10: Detected Xen v4.3
(XEN) io.c:201:d10 MMIO emulation failed @ 0008:c2c2c2c2: 18 7c 55 6d 03 83 ff
ff 10 7c
(XEN) hvm.c:1253:d10 Triple fault on VCPU0 - invoking HVM shutdown action 1.
(XEN) HVM11: HVM Loader
...

Update return codes of wrmsr_hypervisor_regs, update callers to deal
with the new return codes:
 0: not handled
 1: handled
 -EAGAIN: retry

Currently wrmsr_hypervisor_regs will not return the following error, it
will be added in a separate patch:
 -EINVAL: error during handling


Also update the gdprintk to handle a page value of NULL to avoid
printing a bogus MFN value. Update also computing of MSR value in
gdprintk, the idx was always zero.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Keir Fraser <keir@xen.org>

diff -r 9df019eef776 -r e04344e05ce4 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1569,7 +1569,7 @@ static int svm_msr_read_intercept(unsign
 
 static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 {
-    int ret;
+    int ret, result = X86EMUL_OKAY;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     int sync = 0;
@@ -1682,14 +1682,24 @@ static int svm_msr_write_intercept(unsig
         if ( wrmsr_viridian_regs(msr, msr_content) )
             break;
 
-        wrmsr_hypervisor_regs(msr, msr_content);
+        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+        {
+        case -EAGAIN:
+            result = X86EMUL_RETRY;
+            break;
+        case 0:
+        case 1:
+            break;
+        default:
+            goto gpf;
+        }
         break;
     }
 
     if ( sync )
         svm_vmload(vmcb);
 
-    return X86EMUL_OKAY;
+    return result;
 
  gpf:
     hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff -r 9df019eef776 -r e04344e05ce4 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2088,7 +2088,16 @@ static int vmx_msr_write_intercept(unsig
             case HNDL_unhandled:
                 if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
                      !is_last_branch_msr(msr) )
-                    wrmsr_hypervisor_regs(msr, msr_content);
+                    switch ( wrmsr_hypervisor_regs(msr, msr_content) )
+                    {
+                    case -EAGAIN:
+                        return X86EMUL_RETRY;
+                    case 0:
+                    case 1:
+                        break;
+                    default:
+                        goto gp_fault;
+                    }
                 break;
             case HNDL_exception_raised:
                 return X86EMUL_EXCEPTION;
diff -r 9df019eef776 -r e04344e05ce4 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -634,6 +634,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
         unsigned long gmfn = val >> 12;
         unsigned int idx  = val & 0xfff;
         struct page_info *page;
+        p2m_type_t t;
 
         if ( idx > 0 )
         {
@@ -643,15 +644,22 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
             return 0;
         }
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
                 put_page(page);
+
+            if ( p2m_is_paging(t) )
+            {
+                p2m_mem_paging_populate(d, gmfn);
+                return -EAGAIN;
+            }
+
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
-                     gmfn, page_to_mfn(page), base + idx);
+                     gmfn, page ? page_to_mfn(page) : -1UL, base);
             return 0;
         }
 
@@ -2490,7 +2498,7 @@ static int emulate_privileged_op(struct 
                 goto fail;
             break;
         default:
-            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) )
+            if ( wrmsr_hypervisor_regs(regs->ecx, msr_content) == 1 )
                 break;
 
             rc = vmce_wrmsr(regs->ecx, msr_content);

Jan Beulich

2013-May-08 09:27 UTC

head link

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 03.05.13 at 18:29, Olaf Hering <olaf@aepfle.de> wrote:
> xen: handle paged gfn in wrmsr_hypervisor_regs
Does this need any backporting?

Jan

Olaf Hering

2013-May-08 10:51 UTC

head link

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

On Wed, May 08, Jan Beulich wrote:
> >>> On 03.05.13 at 18:29, Olaf Hering <olaf@aepfle.de>
wrote:
> > xen: handle paged gfn in wrmsr_hypervisor_regs
> 
> Does this need any backporting?
4.2 has the same issue, if I read the code correctly.

Olaf

Jan Beulich

2013-May-08 11:41 UTC

head link

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

>>> On 08.05.13 at 12:51, Olaf Hering <olaf@aepfle.de> wrote:
> On Wed, May 08, Jan Beulich wrote:
> 
>> >>> On 03.05.13 at 18:29, Olaf Hering <olaf@aepfle.de>
wrote:
>> > xen: handle paged gfn in wrmsr_hypervisor_regs
>> 
>> Does this need any backporting?
> 
> 4.2 has the same issue, if I read the code correctly.
I understand that - the question is whether paging is usable
enough in 4.2 to warrant this being backported.

Jan

Olaf Hering

2013-May-08 11:45 UTC

head link

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

On Wed, May 08, Jan Beulich wrote:
> >>> On 08.05.13 at 12:51, Olaf Hering <olaf@aepfle.de>
wrote:
> > On Wed, May 08, Jan Beulich wrote:
> > 
> >> >>> On 03.05.13 at 18:29, Olaf Hering
<olaf@aepfle.de> wrote:
> >> > xen: handle paged gfn in wrmsr_hypervisor_regs
> >> 
> >> Does this need any backporting?
> > 
> > 4.2 has the same issue, if I read the code correctly.
> 
> I understand that - the question is whether paging is usable
> enough in 4.2 to warrant this being backported.
It is, but for some reason I did not run into this issue.

Olaf

Xen devel - May 2013 - [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

[PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

[PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

[PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

[PATCH v4] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v2] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v3] xen: handle paged gfn in wrmsr_hypervisor_regs

[PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs

Re: [PATCH v5] xen: handle paged gfn in wrmsr_hypervisor_regs