From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] nested SVM: adjust guest handling of structure mappings Date: Mon, 11 Nov 2013 11:25:18 +0000 Message-ID: <5280BE9E.1060209@citrix.com> References: <5280A4640200007800101A29@nat28.tlf.novell.com> <5280C8830200007800101BCD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1401726662458664633==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VfpcS-0006Qq-Bw for xen-devel@lists.xenproject.org; Mon, 11 Nov 2013 11:25:24 +0000 In-Reply-To: <5280C8830200007800101BCD@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Boris Ostrovsky , Christoph Egger , suravee.suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org --===============1401726662458664633== Content-Type: multipart/alternative; boundary="------------020606010805060406040702" --------------020606010805060406040702 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 11/11/13 11:07, Jan Beulich wrote: > For one, nestedsvm_vmcb_map() error checking must not consist of using > assertions: Global (permanent) mappings can fail, and hence failure > needs to be dealt with properly. And non-global (transient) mappings > can't fail anyway. > > And then the I/O port access bitmap handling was broken: It checked > only to first of the accessed ports rather than each of them. > > Signed-off-by: Jan Beulich > --- > v2: Refine the change to nsvm_vmrun_permissionmap() - > hvm_map_guest_frame_*() can return NULL for reasons other than > map_domain_page_global() doing so (pointed out by Andrew Cooper). > That leaves the function broken though (it ought to handle the > shared/paged out cases in an appropriate way). > > --- a/xen/arch/x86/hvm/svm/nestedsvm.c > +++ b/xen/arch/x86/hvm/svm/nestedsvm.c > @@ -342,7 +342,7 @@ static int nsvm_vmrun_permissionmap(stru > unsigned int i; > enum hvm_copy_result ret; > unsigned long *ns_viomap; > - bool_t ioport_80, ioport_ed; > + bool_t ioport_80 = 1, ioport_ed = 1; > > ns_msrpm_ptr = (unsigned long *)svm->ns_cached_msrpm; > > @@ -360,10 +360,12 @@ static int nsvm_vmrun_permissionmap(stru > svm->ns_iomap_pa = ns_vmcb->_iopm_base_pa; > > ns_viomap = hvm_map_guest_frame_ro(svm->ns_iomap_pa >> PAGE_SHIFT, 0); > - ASSERT(ns_viomap != NULL); > - ioport_80 = test_bit(0x80, ns_viomap); > - ioport_ed = test_bit(0xed, ns_viomap); > - hvm_unmap_guest_frame(ns_viomap, 0); > + if ( ns_viomap ) > + { > + ioport_80 = test_bit(0x80, ns_viomap); > + ioport_ed = test_bit(0xed, ns_viomap); > + hvm_unmap_guest_frame(ns_viomap, 0); > + } Should we not bail on an error here, similar to the failure of hvm_copy_from_guest just out of context above? > > svm->ns_iomap = nestedhvm_vcpu_iomap_get(ioport_80, ioport_ed); > > @@ -866,40 +868,45 @@ nsvm_vmcb_guest_intercepts_msr(unsigned > static int > nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, uint64_t exitinfo1) > { > - unsigned long iopm_gfn = iopm_pa >> PAGE_SHIFT; > - unsigned long *io_bitmap = NULL; > + unsigned long gfn = iopm_pa >> PAGE_SHIFT; > + unsigned long *io_bitmap; > ioio_info_t ioinfo; > uint16_t port; > + unsigned int size; > bool_t enabled; > - unsigned long gfn = 0; /* gcc ... */ > > ioinfo.bytes = exitinfo1; > port = ioinfo.fields.port; > + size = ioinfo.fields.sz32 ? 4 : ioinfo.fields.sz16 ? 2 : 1; > > - switch (port) { > - case 0 ... 32767: /* first 4KB page */ > - gfn = iopm_gfn; > + switch ( port ) > + { > + case 0 ... 8 * PAGE_SIZE - 1: /* first 4KB page */ > break; > - case 32768 ... 65535: /* second 4KB page */ > - port -= 32768; > - gfn = iopm_gfn + 1; > + case 8 * PAGE_SIZE ... 2 * 8 * PAGE_SIZE - 1: /* second 4KB page */ > + port -= 8 * PAGE_SIZE; > + ++gfn; > break; > default: > BUG(); > break; > } > > - io_bitmap = hvm_map_guest_frame_ro(gfn, 0); > - if (io_bitmap == NULL) { > - gdprintk(XENLOG_ERR, > - "IOIO intercept: mapping of permission map failed\n"); > - return NESTEDHVM_VMEXIT_ERROR; > + for ( io_bitmap = hvm_map_guest_frame_ro(gfn, 0); ; ) This needs further checking for NULL. ~Andrew > + { > + enabled = test_bit(port, io_bitmap); > + if ( !enabled || !--size ) > + break; > + if ( unlikely(++port == 8 * PAGE_SIZE) ) > + { > + hvm_unmap_guest_frame(io_bitmap, 0); > + io_bitmap = hvm_map_guest_frame_ro(++gfn, 0); > + port -= 8 * PAGE_SIZE; > + } > } > - > - enabled = test_bit(port, io_bitmap); > hvm_unmap_guest_frame(io_bitmap, 0); > > - if (!enabled) > + if ( !enabled ) > return NESTEDHVM_VMEXIT_HOST; > > return NESTEDHVM_VMEXIT_INJECT; > @@ -966,8 +973,8 @@ nsvm_vmcb_guest_intercepts_exitcode(stru > switch (exitcode) { > case VMEXIT_MSR: > ASSERT(regs != NULL); > - nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr); > - ASSERT(nv->nv_vvmcx != NULL); > + if ( !nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr) ) > + break; > ns_vmcb = nv->nv_vvmcx; > vmexits = nsvm_vmcb_guest_intercepts_msr(svm->ns_cached_msrpm, > regs->ecx, ns_vmcb->exitinfo1 != 0); > @@ -975,8 +982,8 @@ nsvm_vmcb_guest_intercepts_exitcode(stru > return 0; > break; > case VMEXIT_IOIO: > - nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr); > - ASSERT(nv->nv_vvmcx != NULL); > + if ( !nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr) ) > + break; > ns_vmcb = nv->nv_vvmcx; > vmexits = nsvm_vmcb_guest_intercepts_ioio(ns_vmcb->_iopm_base_pa, > ns_vmcb->exitinfo1); > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020606010805060406040702 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 11/11/13 11:07, Jan Beulich wrote:
For one, nestedsvm_vmcb_map() error checking must not consist of using
assertions: Global (permanent) mappings can fail, and hence failure
needs to be dealt with properly. And non-global (transient) mappings
can't fail anyway.

And then the I/O port access bitmap handling was broken: It checked
only to first of the accessed ports rather than each of them.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Refine the change to nsvm_vmrun_permissionmap() -
    hvm_map_guest_frame_*() can return NULL for reasons other than
    map_domain_page_global() doing so (pointed out by Andrew Cooper).
    That leaves the function broken though (it ought to handle the
    shared/paged out cases in an appropriate way).

--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -342,7 +342,7 @@ static int nsvm_vmrun_permissionmap(stru
     unsigned int i;
     enum hvm_copy_result ret;
     unsigned long *ns_viomap;
-    bool_t ioport_80, ioport_ed;
+    bool_t ioport_80 = 1, ioport_ed = 1;
 
     ns_msrpm_ptr = (unsigned long *)svm->ns_cached_msrpm;
 
@@ -360,10 +360,12 @@ static int nsvm_vmrun_permissionmap(stru
     svm->ns_iomap_pa = ns_vmcb->_iopm_base_pa;
 
     ns_viomap = hvm_map_guest_frame_ro(svm->ns_iomap_pa >> PAGE_SHIFT, 0);
-    ASSERT(ns_viomap != NULL);
-    ioport_80 = test_bit(0x80, ns_viomap);
-    ioport_ed = test_bit(0xed, ns_viomap);
-    hvm_unmap_guest_frame(ns_viomap, 0);
+    if ( ns_viomap )
+    {
+        ioport_80 = test_bit(0x80, ns_viomap);
+        ioport_ed = test_bit(0xed, ns_viomap);
+        hvm_unmap_guest_frame(ns_viomap, 0);
+    }

Should we not bail on an error here, similar to the failure of hvm_copy_from_guest just out of context above?

 
     svm->ns_iomap = nestedhvm_vcpu_iomap_get(ioport_80, ioport_ed);
 
@@ -866,40 +868,45 @@ nsvm_vmcb_guest_intercepts_msr(unsigned 
 static int
 nsvm_vmcb_guest_intercepts_ioio(paddr_t iopm_pa, uint64_t exitinfo1)
 {
-    unsigned long iopm_gfn = iopm_pa >> PAGE_SHIFT;
-    unsigned long *io_bitmap = NULL;
+    unsigned long gfn = iopm_pa >> PAGE_SHIFT;
+    unsigned long *io_bitmap;
     ioio_info_t ioinfo;
     uint16_t port;
+    unsigned int size;
     bool_t enabled;
-    unsigned long gfn = 0; /* gcc ... */
 
     ioinfo.bytes = exitinfo1;
     port = ioinfo.fields.port;
+    size = ioinfo.fields.sz32 ? 4 : ioinfo.fields.sz16 ? 2 : 1;
 
-    switch (port) {
-    case 0 ... 32767: /* first 4KB page */
-        gfn = iopm_gfn;
+    switch ( port )
+    {
+    case 0 ... 8 * PAGE_SIZE - 1: /* first 4KB page */
         break;
-    case 32768 ... 65535: /* second 4KB page */
-        port -= 32768;
-        gfn = iopm_gfn + 1;
+    case 8 * PAGE_SIZE ... 2 * 8 * PAGE_SIZE - 1: /* second 4KB page */
+        port -= 8 * PAGE_SIZE;
+        ++gfn;
         break;
     default:
         BUG();
         break;
     }
 
-    io_bitmap = hvm_map_guest_frame_ro(gfn, 0);
-    if (io_bitmap == NULL) {
-        gdprintk(XENLOG_ERR,
-            "IOIO intercept: mapping of permission map failed\n");
-        return NESTEDHVM_VMEXIT_ERROR;
+    for ( io_bitmap = hvm_map_guest_frame_ro(gfn, 0); ; )

This needs further checking for NULL.

~Andrew

+    {
+        enabled = test_bit(port, io_bitmap);
+        if ( !enabled || !--size )
+            break;
+        if ( unlikely(++port == 8 * PAGE_SIZE) )
+        {
+            hvm_unmap_guest_frame(io_bitmap, 0);
+            io_bitmap = hvm_map_guest_frame_ro(++gfn, 0);
+            port -= 8 * PAGE_SIZE;
+        }
     }
-
-    enabled = test_bit(port, io_bitmap);
     hvm_unmap_guest_frame(io_bitmap, 0);
 
-    if (!enabled)
+    if ( !enabled )
         return NESTEDHVM_VMEXIT_HOST;
 
     return NESTEDHVM_VMEXIT_INJECT;
@@ -966,8 +973,8 @@ nsvm_vmcb_guest_intercepts_exitcode(stru
     switch (exitcode) {
     case VMEXIT_MSR:
         ASSERT(regs != NULL);
-        nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr);
-        ASSERT(nv->nv_vvmcx != NULL);
+        if ( !nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr) )
+            break;
         ns_vmcb = nv->nv_vvmcx;
         vmexits = nsvm_vmcb_guest_intercepts_msr(svm->ns_cached_msrpm,
             regs->ecx, ns_vmcb->exitinfo1 != 0);
@@ -975,8 +982,8 @@ nsvm_vmcb_guest_intercepts_exitcode(stru
             return 0;
         break;
     case VMEXIT_IOIO:
-        nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr);
-        ASSERT(nv->nv_vvmcx != NULL);
+        if ( !nestedsvm_vmcb_map(v, nv->nv_vvmcxaddr) )
+            break;
         ns_vmcb = nv->nv_vvmcx;
         vmexits = nsvm_vmcb_guest_intercepts_ioio(ns_vmcb->_iopm_base_pa,
             ns_vmcb->exitinfo1);





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

--------------020606010805060406040702-- --===============1401726662458664633== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1401726662458664633==--