For multi-byte operations all affected ports'' bits in the bitmap need
to be checked, not just the first port''s one.
Reported-by: Matthew Daley <mattd@bugfuzz.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
u32 ctrl;
- u8 *bitmap;
nvcpu->nv_vmexit_pending = 0;
nvmx->intr.intr_info = 0;
@@ -2220,15 +2219,22 @@ int nvmx_n2_vmexit_handler(struct cpu_us
if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP )
{
unsigned long qual;
- u16 port;
+ u16 port, size;
__vmread(EXIT_QUALIFICATION, &qual);
- port = qual >> 16;
- bitmap = nvmx->iobitmap[port >> 15];
- if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port
& 0x7)) )
- nvcpu->nv_vmexit_pending = 1;
+ for ( port = qual >> 16, size = (qual & 7) + 1; ; )
+ {
+ const u8 *bitmap = nvmx->iobitmap[port >> 15];
+
+ if ( bitmap[(port & 0x7fff) >> 3] & (1 <<
(port & 7)) )
+ nvcpu->nv_vmexit_pending = 1;
+ if ( !--size )
+ break;
+ if ( !++port )
+ nvcpu->nv_vmexit_pending = 1;
+ } while ( !nvcpu->nv_vmexit_pending );
if ( !nvcpu->nv_vmexit_pending )
- gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port);
+ printk(XENLOG_G_WARNING "L0 PIO %04x\n", port);
}
else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING )
nvcpu->nv_vmexit_pending = 1;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On 03/12/13 13:44, Jan Beulich wrote:> For multi-byte operations all affected ports'' bits in the bitmap need > to be checked, not just the first port''s one. > > Reported-by: Matthew Daley <mattd@bugfuzz.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us > struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); > struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > u32 ctrl; > - u8 *bitmap; > > nvcpu->nv_vmexit_pending = 0; > nvmx->intr.intr_info = 0; > @@ -2220,15 +2219,22 @@ int nvmx_n2_vmexit_handler(struct cpu_us > if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) > { > unsigned long qual; > - u16 port; > + u16 port, size; > > __vmread(EXIT_QUALIFICATION, &qual); > - port = qual >> 16; > - bitmap = nvmx->iobitmap[port >> 15]; > - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) > - nvcpu->nv_vmexit_pending = 1; > + for ( port = qual >> 16, size = (qual & 7) + 1; ; ) > + { > + const u8 *bitmap = nvmx->iobitmap[port >> 15]; > + > + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) > + nvcpu->nv_vmexit_pending = 1; > + if ( !--size ) > + break; > + if ( !++port ) > + nvcpu->nv_vmexit_pending = 1; > + } while ( !nvcpu->nv_vmexit_pending );You have a rather odd looking "for () { } while ()" loop, which appears to be a while loop with no body and a constant loop condition. Is this intended? ~Andrew> if ( !nvcpu->nv_vmexit_pending ) > - gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port); > + printk(XENLOG_G_WARNING "L0 PIO %04x\n", port); > } > else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING ) > nvcpu->nv_vmexit_pending = 1; > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 03.12.13 at 14:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 03/12/13 13:44, Jan Beulich wrote: >> For multi-byte operations all affected ports'' bits in the bitmap need >> to be checked, not just the first port''s one. >> >> Reported-by: Matthew Daley <mattd@bugfuzz.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); >> struct nestedvmx *nvmx = &vcpu_2_nvmx(v); >> u32 ctrl; >> - u8 *bitmap; >> >> nvcpu->nv_vmexit_pending = 0; >> nvmx->intr.intr_info = 0; >> @@ -2220,15 +2219,22 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >> { >> unsigned long qual; >> - u16 port; >> + u16 port, size; >> >> __vmread(EXIT_QUALIFICATION, &qual); >> - port = qual >> 16; >> - bitmap = nvmx->iobitmap[port >> 15]; >> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >> - nvcpu->nv_vmexit_pending = 1; >> + for ( port = qual >> 16, size = (qual & 7) + 1; ; ) >> + { >> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >> + >> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >> + nvcpu->nv_vmexit_pending = 1; >> + if ( !--size ) >> + break; >> + if ( !++port ) >> + nvcpu->nv_vmexit_pending = 1; >> + } while ( !nvcpu->nv_vmexit_pending ); > > You have a rather odd looking "for () { } while ()" loop, which appears > to be a while loop with no body and a constant loop condition. Is this > intended?Oops, no, of course not. Jan
For multi-byte operations all affected ports'' bits in the bitmap need
to be checked, not just the first port''s one.
Reported-by: Matthew Daley <mattd@bugfuzz.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fix loop construct.
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
u32 ctrl;
- u8 *bitmap;
nvcpu->nv_vmexit_pending = 0;
nvmx->intr.intr_info = 0;
@@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us
if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP )
{
unsigned long qual;
- u16 port;
+ u16 port, size;
__vmread(EXIT_QUALIFICATION, &qual);
port = qual >> 16;
- bitmap = nvmx->iobitmap[port >> 15];
- if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port
& 0x7)) )
- nvcpu->nv_vmexit_pending = 1;
+ size = (qual & 7) + 1;
+ do {
+ const u8 *bitmap = nvmx->iobitmap[port >> 15];
+
+ if ( bitmap[(port & 0x7fff) >> 3] & (1 <<
(port & 7)) )
+ nvcpu->nv_vmexit_pending = 1;
+ if ( !--size )
+ break;
+ if ( !++port )
+ nvcpu->nv_vmexit_pending = 1;
+ } while ( !nvcpu->nv_vmexit_pending );
if ( !nvcpu->nv_vmexit_pending )
- gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port);
+ printk(XENLOG_G_WARNING "L0 PIO %04x\n", port);
}
else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING )
nvcpu->nv_vmexit_pending = 1;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Andrew Cooper
2013-Dec-03 14:30 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
On 03/12/13 14:06, Jan Beulich wrote:> For multi-byte operations all affected ports'' bits in the bitmap need > to be checked, not just the first port''s one. > > Reported-by: Matthew Daley <mattd@bugfuzz.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Fix loop construct. > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us > struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); > struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > u32 ctrl; > - u8 *bitmap; > > nvcpu->nv_vmexit_pending = 0; > nvmx->intr.intr_info = 0; > @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us > if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) > { > unsigned long qual; > - u16 port; > + u16 port, size; > > __vmread(EXIT_QUALIFICATION, &qual); > port = qual >> 16; > - bitmap = nvmx->iobitmap[port >> 15]; > - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) > - nvcpu->nv_vmexit_pending = 1; > + size = (qual & 7) + 1;This should be (qual & 3) for the correct size (Bit 3 is the direction). Is it worth also verifying that ((qual & 3) != 2)? ~Andrew> + do { > + const u8 *bitmap = nvmx->iobitmap[port >> 15]; > + > + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) > + nvcpu->nv_vmexit_pending = 1; > + if ( !--size ) > + break; > + if ( !++port ) > + nvcpu->nv_vmexit_pending = 1; > + } while ( !nvcpu->nv_vmexit_pending ); > if ( !nvcpu->nv_vmexit_pending ) > - gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port); > + printk(XENLOG_G_WARNING "L0 PIO %04x\n", port); > } > else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING ) > nvcpu->nv_vmexit_pending = 1; > > >
>>> On 03.12.13 at 15:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >> { >> unsigned long qual; >> - u16 port; >> + u16 port, size; >> >> __vmread(EXIT_QUALIFICATION, &qual); >> port = qual >> 16; >> - bitmap = nvmx->iobitmap[port >> 15]; >> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >> - nvcpu->nv_vmexit_pending = 1; >> + size = (qual & 7) + 1; > > This should be (qual & 3) for the correct size (Bit 3 is the > direction).Right - bit 3 is the direction. Bits 0..2 are the size. Hence the mask ought to be 7.> Is it worth also verifying that ((qual & 3) != 2)?I don''t think so - there''s no harm to our code here if an undefined value was there. Jan
Andrew Cooper
2013-Dec-03 15:58 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
On 03/12/13 15:55, Jan Beulich wrote:>>>> On 03.12.13 at 15:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >>> { >>> unsigned long qual; >>> - u16 port; >>> + u16 port, size; >>> >>> __vmread(EXIT_QUALIFICATION, &qual); >>> port = qual >> 16; >>> - bitmap = nvmx->iobitmap[port >> 15]; >>> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >>> - nvcpu->nv_vmexit_pending = 1; >>> + size = (qual & 7) + 1; >> This should be (qual & 3) for the correct size (Bit 3 is the >> direction). > Right - bit 3 is the direction. Bits 0..2 are the size. Hence the > mask ought to be 7.D''oh - I cant count, even with the manual for reference. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> >> Is it worth also verifying that ((qual & 3) != 2)? > I don''t think so - there''s no harm to our code here if an undefined > value was there. > > Jan >
Zhang, Yang Z
2013-Dec-04 01:51 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
Jan Beulich wrote on 2013-12-03:> For multi-byte operations all affected ports'' bits in the bitmap need to be > checked, not just the first port''s one. > > Reported-by: Matthew Daley <mattd@bugfuzz.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Fix loop construct. > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us > struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); > struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > u32 ctrl; > - u8 *bitmap; > > nvcpu->nv_vmexit_pending = 0; > nvmx->intr.intr_info = 0; > @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us > if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) > { > unsigned long qual; > - u16 port; > + u16 port, size; > > __vmread(EXIT_QUALIFICATION, &qual); > port = qual >> 16; > - bitmap = nvmx->iobitmap[port >> 15]; > - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) > - nvcpu->nv_vmexit_pending = 1; > + size = (qual & 7) + 1; > + do { > + const u8 *bitmap = nvmx->iobitmap[port >> 15]; > + > + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) > + nvcpu->nv_vmexit_pending = 1; > + if ( !--size ) > + break; > + if ( !++port ) > + nvcpu->nv_vmexit_pending = 1;If port overflow, will it cause vmexit or maybe other fault like GP or just be ignored? Also, you need to check the DF bit to know the string direction before updating the port. Best regards, Yang
Andrew Cooper
2013-Dec-04 02:08 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
On 04/12/2013 01:51, Zhang, Yang Z wrote:> Jan Beulich wrote on 2013-12-03: >> For multi-byte operations all affected ports'' bits in the bitmap need to be >> checked, not just the first port''s one. >> >> Reported-by: Matthew Daley <mattd@bugfuzz.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Fix loop construct. >> >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); >> struct nestedvmx *nvmx = &vcpu_2_nvmx(v); >> u32 ctrl; >> - u8 *bitmap; >> >> nvcpu->nv_vmexit_pending = 0; >> nvmx->intr.intr_info = 0; >> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >> { >> unsigned long qual; >> - u16 port; >> + u16 port, size; >> >> __vmread(EXIT_QUALIFICATION, &qual); >> port = qual >> 16; >> - bitmap = nvmx->iobitmap[port >> 15]; >> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >> - nvcpu->nv_vmexit_pending = 1; >> + size = (qual & 7) + 1; >> + do { >> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >> + >> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >> + nvcpu->nv_vmexit_pending = 1; >> + if ( !--size ) >> + break; >> + if ( !++port ) >> + nvcpu->nv_vmexit_pending = 1; > If port overflow, will it cause vmexit or maybe other fault like GP or just be ignored? Also, you need to check the DF bit to know the string direction before updating the port. > > Best regards, > Yang > >What does the direction flag have to do with the port(s) used as the target of an ins/outs instruction? I was under the impression that it solely acted as an increment/decrement on si/di. ~Andrew
Zhang, Yang Z
2013-Dec-04 02:16 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
Andrew Cooper wrote on 2013-12-04:> On 04/12/2013 01:51, Zhang, Yang Z wrote: >> Jan Beulich wrote on 2013-12-03: >>> For multi-byte operations all affected ports'' bits in the bitmap >>> need to be checked, not just the first port''s one. >>> >>> Reported-by: Matthew Daley <mattd@bugfuzz.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> v2: Fix loop construct. >>> >>> --- a/xen/arch/x86/hvm/vmx/vvmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >>> @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>> struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); >>> struct nestedvmx *nvmx = &vcpu_2_nvmx(v); >>> u32 ctrl; >>> - u8 *bitmap; >>> >>> nvcpu->nv_vmexit_pending = 0; >>> nvmx->intr.intr_info = 0; >>> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >>> { >>> unsigned long qual; >>> - u16 port; >>> + u16 port, size; >>> >>> __vmread(EXIT_QUALIFICATION, &qual); >>> port = qual >> 16; >>> - bitmap = nvmx->iobitmap[port >> 15]; >>> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >>> - nvcpu->nv_vmexit_pending = 1; >>> + size = (qual & 7) + 1; >>> + do { >>> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >>> + >>> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >>> + nvcpu->nv_vmexit_pending = 1; >>> + if ( !--size ) >>> + break; >>> + if ( !++port ) >>> + nvcpu->nv_vmexit_pending = 1; >> If port overflow, will it cause vmexit or maybe other fault like GP >> or just be > ignored? Also, you need to check the DF bit to know the string > direction before updating the port. >> >> Best regards, >> Yang >> >> > > What does the direction flag have to do with the port(s) used as the > target of an ins/outs instruction? I was under the impression that it > solely acted as an increment/decrement on si/di. >Uh.. It seems DF only impact the memory location, ioport is not affected, See what SDM said. After the byte, word, or doubleword is transfer from the I/O port to the memory location, the DI/EDI/RDI register is incremented or decremented automatically according to the setting of the DF flag in the EFLAGS register. (If the DF flag is 0, the (E)DI register is incremented; if the DF flag is 1, the (E)DI register is decremented.) The (E)DI register is incremented or decremented by 1 for byte operations, by 2 for word operations, or by 4 for doubleword operations.> ~AndrewBest regards, Yang
>>> On 04.12.13 at 02:51, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > Jan Beulich wrote on 2013-12-03: >> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >> { >> unsigned long qual; >> - u16 port; >> + u16 port, size; >> >> __vmread(EXIT_QUALIFICATION, &qual); >> port = qual >> 16; >> - bitmap = nvmx->iobitmap[port >> 15]; >> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >> - nvcpu->nv_vmexit_pending = 1; >> + size = (qual & 7) + 1; >> + do { >> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >> + >> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >> + nvcpu->nv_vmexit_pending = 1; >> + if ( !--size ) >> + break; >> + if ( !++port ) >> + nvcpu->nv_vmexit_pending = 1; > > If port overflow, will it cause vmexit or maybe other fault like GP or just > be ignored?The documentation is explicit here: It causes a VM exit. And hence the emulation is doing so too.> Also, you need to check the DF bit to know the string direction > before updating the port.I think Andrew already sufficiently clarified that part. Jan
Zhang, Yang Z
2013-Dec-04 08:07 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
Jan Beulich wrote on 2013-12-04:>>>> On 04.12.13 at 02:51, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: >> Jan Beulich wrote on 2013-12-03: >>> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >>> { >>> unsigned long qual; >>> - u16 port; >>> + u16 port, size; >>> >>> __vmread(EXIT_QUALIFICATION, &qual); >>> port = qual >> 16; >>> - bitmap = nvmx->iobitmap[port >> 15]; >>> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >>> - nvcpu->nv_vmexit_pending = 1; >>> + size = (qual & 7) + 1; >>> + do { >>> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >>> + >>> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >>> + nvcpu->nv_vmexit_pending = 1; >>> + if ( !--size ) >>> + break; >>> + if ( !++port ) >>> + nvcpu->nv_vmexit_pending = 1; >> >> If port overflow, will it cause vmexit or maybe other fault like GP >> or just be ignored? > > The documentation is explicit here: It causes a VM exit. And hence the > emulation is doing so too.Ok. BTW, which chapter tells this? I didn''t find the corresponding chapter in Intel SDM. :(> >> Also, you need to check the DF bit to know the string direction >> before updating the port. > > I think Andrew already sufficiently clarified that part. > > JanBest regards, Yang
Acked-by Eddie Dong <eddie.dong@intel.com>
-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com]
Sent: Tuesday, December 03, 2013 10:07 PM
To: xen-devel
Cc: Matthew Daley; Andrew Cooper; Dong, Eddie; Nakajima, Jun
Subject: [PATCH v2] nested VMX: fix I/O port exit emulation
For multi-byte operations all affected ports'' bits in the bitmap need
to be checked, not just the first port''s one.
Reported-by: Matthew Daley <mattd@bugfuzz.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fix loop construct.
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
u32 ctrl;
- u8 *bitmap;
nvcpu->nv_vmexit_pending = 0;
nvmx->intr.intr_info = 0;
@@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us
if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP )
{
unsigned long qual;
- u16 port;
+ u16 port, size;
__vmread(EXIT_QUALIFICATION, &qual);
port = qual >> 16;
- bitmap = nvmx->iobitmap[port >> 15];
- if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port
& 0x7)) )
- nvcpu->nv_vmexit_pending = 1;
+ size = (qual & 7) + 1;
+ do {
+ const u8 *bitmap = nvmx->iobitmap[port >> 15];
+
+ if ( bitmap[(port & 0x7fff) >> 3] & (1 <<
(port & 7)) )
+ nvcpu->nv_vmexit_pending = 1;
+ if ( !--size )
+ break;
+ if ( !++port )
+ nvcpu->nv_vmexit_pending = 1;
+ } while ( !nvcpu->nv_vmexit_pending );
if ( !nvcpu->nv_vmexit_pending )
- gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port);
+ printk(XENLOG_G_WARNING "L0 PIO %04x\n", port);
}
else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING )
nvcpu->nv_vmexit_pending = 1;
>>> On 04.12.13 at 09:07, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > Jan Beulich wrote on 2013-12-04: >>>>> On 04.12.13 at 02:51, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: >>> Jan Beulich wrote on 2013-12-03: >>>> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>>> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >>>> { >>>> unsigned long qual; >>>> - u16 port; >>>> + u16 port, size; >>>> >>>> __vmread(EXIT_QUALIFICATION, &qual); >>>> port = qual >> 16; >>>> - bitmap = nvmx->iobitmap[port >> 15]; >>>> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >>>> - nvcpu->nv_vmexit_pending = 1; >>>> + size = (qual & 7) + 1; >>>> + do { >>>> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >>>> + >>>> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >>>> + nvcpu->nv_vmexit_pending = 1; >>>> + if ( !--size ) >>>> + break; >>>> + if ( !++port ) >>>> + nvcpu->nv_vmexit_pending = 1; >>> >>> If port overflow, will it cause vmexit or maybe other fault like GP >>> or just be ignored? >> >> The documentation is explicit here: It causes a VM exit. And hence the >> emulation is doing so too. > > Ok. BTW, which chapter tells this? I didn't find the corresponding chapter > in Intel SDM. :("25.1.3 Instructions That Cause VM Exits Conditionally" says "● IN, INS/INSB/INSW/INSD, OUT, OUTS/OUTSB/OUTSW/OUTSD. The behavior of each of these instructions is determined by the settings of the “unconditional I/O exiting” and “use I/O bitmaps” VM-execution controls: — If both controls are 0, the instruction executes normally. — If the “unconditional I/O exiting” VM-execution control is 1 and the “use I/O bitmaps” VM-execution control is 0, the instruction causes a VM exit. — If the “use I/O bitmaps” VM-execution control is 1, the instruction causes a VM exit if it attempts to access an I/O port corresponding to a bit set to 1 in the appropriate I/O bitmap (see Section 24.6.4). If an I/O operation “wraps around” the 16-bit I/O-port space (accesses ports FFFFH and 0000H), the I/O instruction causes a VM exit (the “unconditional I/O exiting” VM-execution control is ignored if the “use I/O bitmaps” VM-execution control is 1)." Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Egger, Christoph
2013-Dec-04 09:51 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
On 03.12.13 15:06, Jan Beulich wrote:> For multi-byte operations all affected ports'' bits in the bitmap need > to be checked, not just the first port''s one. > > Reported-by: Matthew Daley <mattd@bugfuzz.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Fix loop construct. > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us > struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); > struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > u32 ctrl; > - u8 *bitmap; > > nvcpu->nv_vmexit_pending = 0; > nvmx->intr.intr_info = 0; > @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us > if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) > { > unsigned long qual; > - u16 port; > + u16 port, size; > > __vmread(EXIT_QUALIFICATION, &qual); > port = qual >> 16; > - bitmap = nvmx->iobitmap[port >> 15]; > - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) > - nvcpu->nv_vmexit_pending = 1; > + size = (qual & 7) + 1; > + do { > + const u8 *bitmap = nvmx->iobitmap[port >> 15]; > + > + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) > + nvcpu->nv_vmexit_pending = 1; > + if ( !--size ) > + break; > + if ( !++port ) > + nvcpu->nv_vmexit_pending = 1; > + } while ( !nvcpu->nv_vmexit_pending ); > if ( !nvcpu->nv_vmexit_pending ) > - gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port); > + printk(XENLOG_G_WARNING "L0 PIO %04x\n", port); > } > else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING ) > nvcpu->nv_vmexit_pending = 1; >Can you use #define''s for the bit operations, please? That makes the code more readable and avoids copy & paste errors. Christoph
>>> On 04.12.13 at 10:51, "Egger, Christoph" <chegger@amazon.de> wrote: > On 03.12.13 15:06, Jan Beulich wrote: >> For multi-byte operations all affected ports'' bits in the bitmap need >> to be checked, not just the first port''s one. >> >> Reported-by: Matthew Daley <mattd@bugfuzz.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Fix loop construct. >> >> --- a/xen/arch/x86/hvm/vmx/vvmx.c >> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >> @@ -2134,7 +2134,6 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v); >> struct nestedvmx *nvmx = &vcpu_2_nvmx(v); >> u32 ctrl; >> - u8 *bitmap; >> >> nvcpu->nv_vmexit_pending = 0; >> nvmx->intr.intr_info = 0; >> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >> { >> unsigned long qual; >> - u16 port; >> + u16 port, size; >> >> __vmread(EXIT_QUALIFICATION, &qual); >> port = qual >> 16; >> - bitmap = nvmx->iobitmap[port >> 15]; >> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >> - nvcpu->nv_vmexit_pending = 1; >> + size = (qual & 7) + 1; >> + do { >> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >> + >> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >> + nvcpu->nv_vmexit_pending = 1; >> + if ( !--size ) >> + break; >> + if ( !++port ) >> + nvcpu->nv_vmexit_pending = 1; >> + } while ( !nvcpu->nv_vmexit_pending ); >> if ( !nvcpu->nv_vmexit_pending ) >> - gdprintk(XENLOG_WARNING, "L0 PIO %x.\n", port); >> + printk(XENLOG_G_WARNING "L0 PIO %04x\n", port); >> } >> else if ( ctrl & CPU_BASED_UNCOND_IO_EXITING ) >> nvcpu->nv_vmexit_pending = 1; >> > > Can you use #define''s for the bit operations, please? > That makes the code more readable and avoids copy & paste errors.That would be a separate cleanup patch, as there''s code in vmx.c also wanting to then use such #define-s. Jan
Zhang, Yang Z
2013-Dec-05 01:38 UTC
Re: [PATCH v2] nested VMX: fix I/O port exit emulation
Jan Beulich wrote on 2013-12-04:>>>> On 04.12.13 at 09:07, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: >> Jan Beulich wrote on 2013-12-04: >>>>>> On 04.12.13 at 02:51, "Zhang, Yang Z" <yang.z.zhang@intel.com> > wrote: >>>> Jan Beulich wrote on 2013-12-03: >>>>> @@ -2220,15 +2219,23 @@ int nvmx_n2_vmexit_handler(struct cpu_us >>>>> if ( ctrl & CPU_BASED_ACTIVATE_IO_BITMAP ) >>>>> { >>>>> unsigned long qual; >>>>> - u16 port; >>>>> + u16 port, size; >>>>> >>>>> __vmread(EXIT_QUALIFICATION, &qual); >>>>> port = qual >> 16; >>>>> - bitmap = nvmx->iobitmap[port >> 15]; >>>>> - if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 0x7)) ) >>>>> - nvcpu->nv_vmexit_pending = 1; >>>>> + size = (qual & 7) + 1; >>>>> + do { >>>>> + const u8 *bitmap = nvmx->iobitmap[port >> 15]; >>>>> + >>>>> + if ( bitmap[(port & 0x7fff) >> 3] & (1 << (port & 7)) ) >>>>> + nvcpu->nv_vmexit_pending = 1; >>>>> + if ( !--size ) >>>>> + break; >>>>> + if ( !++port ) >>>>> + nvcpu->nv_vmexit_pending = 1; >>>> >>>> If port overflow, will it cause vmexit or maybe other fault like >>>> GP or just be ignored? >>> >>> The documentation is explicit here: It causes a VM exit. And hence >>> the emulation is doing so too. >> >> Ok. BTW, which chapter tells this? I didn't find the corresponding >> chapter in Intel SDM. :( > > "25.1.3 Instructions That Cause VM Exits Conditionally" says > > "● IN, INS/INSB/INSW/INSD, OUT, OUTS/OUTSB/OUTSW/OUTSD. The behavior of > each of these instructions is determined by the settings of the > “unconditional I/O exiting” and “use I/O bitmaps” VM-execution controls: > — If both controls are 0, the instruction executes normally. — If the > “unconditional I/O exiting” VM-execution control is 1 and the “use I/O > bitmaps” VM-execution control is 0, the instruction causes a VM exit. — > If the “use I/O bitmaps” VM-execution control is 1, the instruction > causes a VM exit if it attempts to access an I/O port corresponding to a > bit set to 1 in the appropriate I/O bitmap (see Section 24.6.4). If an > I/O operation “wraps around” the 16-bit I/O-port space (accesses ports > FFFFH and 0000H), the I/O instruction causes a VM exit (the > “unconditional I/O exiting” VM-execution control is ignored if the “use > I/O bitmaps” VM-execution control is 1)." >Got it. Thanks.> JanBest regards, Yang _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel