* [PATCH] nested SVM: adjust guest handling of structure mappings
@ 2013-11-11 8:33 Jan Beulich
2013-11-11 11:07 ` [PATCH v2] " Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2013-11-11 8:33 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 3717 bytes --]
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>
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -360,7 +360,6 @@ 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);
@@ -866,40 +865,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); ; )
+ {
+ 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 +970,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 +979,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);
[-- Attachment #2: nSVM-map-errors.patch --]
[-- Type: text/plain, Size: 3770 bytes --]
nested SVM: adjust guest handling of structure mappings
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>
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -360,7 +360,6 @@ 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);
@@ -866,40 +865,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); ; )
+ {
+ 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 +970,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 +979,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);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2] nested SVM: adjust guest handling of structure mappings 2013-11-11 8:33 [PATCH] nested SVM: adjust guest handling of structure mappings Jan Beulich @ 2013-11-11 11:07 ` Jan Beulich 2013-11-11 11:25 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2013-11-11 11:07 UTC (permalink / raw) To: xen-devel; +Cc: Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 4588 bytes --] 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); + } 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); ; ) + { + 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); [-- Attachment #2: nSVM-map-errors.patch --] [-- Type: text/plain, Size: 4643 bytes --] nested SVM: adjust guest handling of structure mappings 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); + } 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); ; ) + { + 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); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] nested SVM: adjust guest handling of structure mappings 2013-11-11 11:07 ` [PATCH v2] " Jan Beulich @ 2013-11-11 11:25 ` Andrew Cooper 2013-11-11 12:29 ` Jan Beulich 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Andrew Cooper @ 2013-11-11 11:25 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit [-- Attachment #1.1: Type: text/plain, Size: 5052 bytes --] 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 [-- Attachment #1.2: Type: text/html, Size: 5889 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] nested SVM: adjust guest handling of structure mappings 2013-11-11 11:25 ` Andrew Cooper @ 2013-11-11 12:29 ` Jan Beulich 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2013-11-11 12:29 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit >>> On 11.11.13 at 12:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 11/11/13 11:07, Jan Beulich wrote: >> @@ -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? It seemed less intrusive (to the guest) to simply disallow (direct) access to these ports in that case. >> - 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. Indeed, thanks for spotting. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 11:25 ` Andrew Cooper 2013-11-11 12:29 ` Jan Beulich @ 2013-11-11 12:53 ` Jan Beulich 2013-11-11 13:16 ` Andrew Cooper ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Jan Beulich @ 2013-11-11 12:53 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 4403 bytes --] 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> --- v3: Add error check to mapping operation in nsvm_vmcb_guest_intercepts_ioio() (pointed out by Andrew Cooper). --- 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); + } 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); ; ) + { + enabled = io_bitmap && 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); [-- Attachment #2: nSVM-map-errors.patch --] [-- Type: text/plain, Size: 4458 bytes --] nested SVM: adjust guest handling of structure mappings 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> --- v3: Add error check to mapping operation in nsvm_vmcb_guest_intercepts_ioio() (pointed out by Andrew Cooper). --- 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); + } 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); ; ) + { + enabled = io_bitmap && 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); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich @ 2013-11-11 13:16 ` Andrew Cooper 2013-11-11 13:35 ` Jan Beulich 2013-11-11 13:22 ` Egger, Christoph 2013-11-12 3:57 ` Suravee Suthikulpanit 2 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2013-11-11 13:16 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit On 11/11/13 12:53, 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> > --- > v3: Add error check to mapping operation in > nsvm_vmcb_guest_intercepts_ioio() (pointed out by Andrew Cooper). > > --- 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); > + } > > 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); ; ) > + { > + enabled = io_bitmap && 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; > + } > } Ok - this safe now, but I don't understand the reasoning for introducing this loop? The ioio exit value gives us a single port, and the size of access on that specific port. The switch statement tells us exactly which gfn the relevant bit refers to, surely a single hvm_map_guest_frame_ro() is sufficient? ~Andrew > - > - 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); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 13:16 ` Andrew Cooper @ 2013-11-11 13:35 ` Jan Beulich 2013-11-11 14:40 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2013-11-11 13:35 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit >>> On 11.11.13 at 14:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 11/11/13 12:53, Jan Beulich wrote: >> + for ( io_bitmap = hvm_map_guest_frame_ro(gfn, 0); ; ) >> + { >> + enabled = io_bitmap && 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; >> + } >> } > > Ok - this safe now, but I don't understand the reasoning for introducing > this loop? > > The ioio exit value gives us a single port, and the size of access on > that specific port. > > The switch statement tells us exactly which gfn the relevant bit refers > to, surely a single hvm_map_guest_frame_ro() is sufficient? When the operation spans multiple ports (INW, INL, etc), multiple bits need to be looked at. And when the access is misaligned and crosses a 32k (port number) boundary, more than one page needs looking at. Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 13:35 ` Jan Beulich @ 2013-11-11 14:40 ` Andrew Cooper 0 siblings, 0 replies; 10+ messages in thread From: Andrew Cooper @ 2013-11-11 14:40 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Boris Ostrovsky, Christoph Egger, suravee.suthikulpanit On 11/11/13 13:35, Jan Beulich wrote: >>>> On 11.11.13 at 14:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 11/11/13 12:53, Jan Beulich wrote: >>> + for ( io_bitmap = hvm_map_guest_frame_ro(gfn, 0); ; ) >>> + { >>> + enabled = io_bitmap && 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; >>> + } >>> } >> Ok - this safe now, but I don't understand the reasoning for introducing >> this loop? >> >> The ioio exit value gives us a single port, and the size of access on >> that specific port. >> >> The switch statement tells us exactly which gfn the relevant bit refers >> to, surely a single hvm_map_guest_frame_ro() is sufficient? > When the operation spans multiple ports (INW, INL, etc), multiple > bits need to be looked at. And when the access is misaligned and > crosses a 32k (port number) boundary, more than one page needs > looking at. > > Jan > Ah of course. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich 2013-11-11 13:16 ` Andrew Cooper @ 2013-11-11 13:22 ` Egger, Christoph 2013-11-12 3:57 ` Suravee Suthikulpanit 2 siblings, 0 replies; 10+ messages in thread From: Egger, Christoph @ 2013-11-11 13:22 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Andrew Cooper, Boris Ostrovsky, suravee.suthikulpanit On 11.11.13 13:53, 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> > --- > v3: Add error check to mapping operation in > nsvm_vmcb_guest_intercepts_ioio() (pointed out by Andrew Cooper). Reviewed-by: Christoph Egger <chegger@amazon.de> <offtopic to the context of this patch> A comment to the MSR algorithm: The performance of the MSR bitmap merging can be greatly improved. By default Xen does intercept MSRs with a few exceptions. Cache those few exceptions and for the nested bitmap only merge the bits from the guest bitmap with those few ones. All other bits are 1 in any case. That shaves off a huge memory footprint from each VMRUN emulation. </offtopic> > > --- 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); > + } > > 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); ; ) > + { > + enabled = io_bitmap && 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); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] nested SVM: adjust guest handling of structure mappings 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich 2013-11-11 13:16 ` Andrew Cooper 2013-11-11 13:22 ` Egger, Christoph @ 2013-11-12 3:57 ` Suravee Suthikulpanit 2 siblings, 0 replies; 10+ messages in thread From: Suravee Suthikulpanit @ 2013-11-12 3:57 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Boris Ostrovsky, Christoph Egger I am on travel until Dec 6th. So, I don't have machine that I can test this on. But it looks ok. Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Suravee On 11/11/2013 07:53 PM, 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> > --- > v3: Add error check to mapping operation in > nsvm_vmcb_guest_intercepts_ioio() (pointed out by Andrew Cooper). > > --- 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); > + } > > 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); ; ) > + { > + enabled = io_bitmap && 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); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-12 3:57 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-11 8:33 [PATCH] nested SVM: adjust guest handling of structure mappings Jan Beulich 2013-11-11 11:07 ` [PATCH v2] " Jan Beulich 2013-11-11 11:25 ` Andrew Cooper 2013-11-11 12:29 ` Jan Beulich 2013-11-11 12:53 ` [PATCH v3] " Jan Beulich 2013-11-11 13:16 ` Andrew Cooper 2013-11-11 13:35 ` Jan Beulich 2013-11-11 14:40 ` Andrew Cooper 2013-11-11 13:22 ` Egger, Christoph 2013-11-12 3:57 ` Suravee Suthikulpanit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).