* [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 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 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: 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).