1: properly handle backward string instruction emulation 2: fix direct PCI port I/O emulation retry and error handling 3: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept 4: properly deal with hvm_copy_*_guest_phys() errors 5: cache emulated instruction for retry processing Signed-off-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich
2013-Sep-30 12:57 UTC
[PATCH 1/5] x86/HVM: properly handle backward string instruction emulation
Multiplying a signed 32-bit quantity with an unsigned 32-bit quantity produces an unsigned 32-bit result, yet for emulation of backward string instructions we need the result sign extended before getting added to the base address. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/intercept.c +++ b/xen/arch/x86/hvm/intercept.c @@ -48,7 +48,7 @@ static int hvm_mmio_access(struct vcpu * hvm_mmio_write_t write_handler) { unsigned long data; - int rc = X86EMUL_OKAY, i, sign = p->df ? -1 : 1; + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; if ( !p->data_is_ptr ) { @@ -68,13 +68,10 @@ static int hvm_mmio_access(struct vcpu * { int ret; - rc = read_handler(v, p->addr + (sign * i * p->size), p->size, - &data); + rc = read_handler(v, p->addr + step * i, p->size, &data); if ( rc != X86EMUL_OKAY ) break; - ret = hvm_copy_to_guest_phys(p->data + (sign * i * p->size), - &data, - p->size); + ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); if ( (ret == HVMCOPY_gfn_paged_out) || (ret == HVMCOPY_gfn_shared) ) { @@ -87,8 +84,7 @@ static int hvm_mmio_access(struct vcpu * { for ( i = 0; i < p->count; i++ ) { - switch ( hvm_copy_from_guest_phys(&data, - p->data + sign * i * p->size, + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, p->size) ) { case HVMCOPY_okay: @@ -109,8 +105,7 @@ static int hvm_mmio_access(struct vcpu * } if ( rc != X86EMUL_OKAY ) break; - rc = write_handler(v, p->addr + (sign * i * p->size), p->size, - data); + rc = write_handler(v, p->addr + step * i, p->size, data); if ( rc != X86EMUL_OKAY ) break; } @@ -142,7 +137,7 @@ int hvm_mmio_intercept(ioreq_t *p) static int process_portio_intercept(portio_action_t action, ioreq_t *p) { - int rc = X86EMUL_OKAY, i, sign = p->df ? -1 : 1; + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; uint32_t data; if ( !p->data_is_ptr ) @@ -167,8 +162,7 @@ static int process_portio_intercept(port rc = action(IOREQ_READ, p->addr, p->size, &data); if ( rc != X86EMUL_OKAY ) break; - (void)hvm_copy_to_guest_phys(p->data + sign*i*p->size, - &data, p->size); + (void)hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); } } else /* p->dir == IOREQ_WRITE */ @@ -176,8 +170,7 @@ static int process_portio_intercept(port for ( i = 0; i < p->count; i++ ) { data = 0; - switch ( hvm_copy_from_guest_phys(&data, - p->data + sign * i * p->size, + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, p->size) ) { case HVMCOPY_okay: --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -294,7 +294,7 @@ void hvm_io_assist(void) static int dpci_ioport_read(uint32_t mport, ioreq_t *p) { - int i, sign = p->df ? -1 : 1; + int i, step = p->df ? -p->size : p->size; uint32_t data = 0; for ( i = 0; i < p->count; i++ ) @@ -317,8 +317,7 @@ static int dpci_ioport_read(uint32_t mpo if ( p->data_is_ptr ) { int ret; - ret = hvm_copy_to_guest_phys(p->data + (sign * i * p->size), &data, - p->size); + ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); if ( (ret == HVMCOPY_gfn_paged_out) || (ret == HVMCOPY_gfn_shared) ) return X86EMUL_RETRY; @@ -332,7 +331,7 @@ static int dpci_ioport_read(uint32_t mpo static int dpci_ioport_write(uint32_t mport, ioreq_t *p) { - int i, sign = p->df ? -1 : 1; + int i, step = p->df ? -p->size : p->size; uint32_t data; for ( i = 0; i < p->count; i++ ) @@ -340,8 +339,7 @@ static int dpci_ioport_write(uint32_t mp data = p->data; if ( p->data_is_ptr ) { - switch ( hvm_copy_from_guest_phys(&data, - p->data + sign * i * p->size, + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, p->size) ) { case HVMCOPY_okay: --- a/xen/arch/x86/hvm/stdvga.c +++ b/xen/arch/x86/hvm/stdvga.c @@ -467,15 +467,17 @@ static uint32_t read_data; static int mmio_move(struct hvm_hw_stdvga *s, ioreq_t *p) { int i; - int sign = p->df ? -1 : 1; + uint64_t addr = p->addr; p2m_type_t p2mt; struct domain *d = current->domain; if ( p->data_is_ptr ) { + uint64_t data = p->data, tmp; + int step = p->df ? -p->size : p->size; + if ( p->dir == IOREQ_READ ) { - uint64_t addr = p->addr, data = p->data, tmp; for ( i = 0; i < p->count; i++ ) { tmp = stdvga_mem_read(addr, p->size); @@ -498,13 +500,12 @@ static int mmio_move(struct hvm_hw_stdvg ASSERT(!dp); stdvga_mem_write(data, tmp, p->size); } - data += sign * p->size; - addr += sign * p->size; + data += step; + addr += step; } } else { - uint32_t addr = p->addr, data = p->data, tmp; for ( i = 0; i < p->count; i++ ) { if ( hvm_copy_from_guest_phys(&tmp, data, p->size) !@@ -523,31 +524,18 @@ static int mmio_move(struct hvm_hw_stdvg tmp = stdvga_mem_read(data, p->size); } stdvga_mem_write(addr, tmp, p->size); - data += sign * p->size; - addr += sign * p->size; + data += step; + addr += step; } } } else { + ASSERT(p->count == 1); if ( p->dir == IOREQ_READ ) - { - uint32_t addr = p->addr; - for ( i = 0; i < p->count; i++ ) - { - p->data = stdvga_mem_read(addr, p->size); - addr += sign * p->size; - } - } + p->data = stdvga_mem_read(addr, p->size); else - { - uint32_t addr = p->addr; - for ( i = 0; i < p->count; i++ ) - { - stdvga_mem_write(addr, p->data, p->size); - addr += sign * p->size; - } - } + stdvga_mem_write(addr, p->data, p->size); } read_data = p->data; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-30 12:57 UTC
[PATCH 2/5] x86/HVM: fix direct PCI port I/O emulation retry and error handling
dpci_ioport_{read,write}() guest memory access failure handling should be modelled after process_portio_intercept()''s (and others): Upon encountering an error on other than the first iteration, the count successfully handled needs to be stored and X86EMUL_OKAY returned, in order for the generic instruction emulator to update register state correctly before reporting failure or retrying (both of which would only happen after re-invoking emulation). Further we leverage (and slightly extend, due to the above mentioned need to return X86EMUL_OKAY) the "large MMIO" retry model. Note that there is still a special case not explicitly taken care of here: While the first retry on the last iteration of a "rep ins" correctly recovers the already read data, an eventual subsequent retry is being handled by the pre-existing mmio-large logic (through hvmemul_do_io() storing the [recovered] data [again], also taking into consideration that the emulator converts a single iteration "ins" to ->read_io() plus ->write()). Also fix an off-by-one in the mmio-large-read logic, and slightly simplify the copying of the data. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -173,6 +173,13 @@ static int hvmemul_do_io( (p_data == NULL) ? HVMIO_dispatched : HVMIO_awaiting_completion; vio->io_size = size; + /* + * When retrying a repeated string instruction, force exit to guest after + * completion of the retried iteration to allow handling of interrupts. + */ + if ( vio->mmio_retrying ) + *reps = 1; + p->dir = dir; p->data_is_ptr = value_is_ptr; p->type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO; @@ -202,8 +209,14 @@ static int hvmemul_do_io( case X86EMUL_RETRY: *reps = p->count; p->state = STATE_IORESP_READY; - hvm_io_assist(); - vio->io_state = HVMIO_none; + if ( !vio->mmio_retry ) + { + hvm_io_assist(); + vio->io_state = HVMIO_none; + } + else + /* Defer hvm_io_assist() invocation to hvm_do_resume(). */ + vio->io_state = HVMIO_handle_mmio_awaiting_completion; break; case X86EMUL_UNHANDLEABLE: rc = X86EMUL_RETRY; @@ -249,10 +262,9 @@ static int hvmemul_do_io( if ( bytes == 0 ) pa = vio->mmio_large_read_pa = addr; if ( (addr == (pa + bytes)) && - ((bytes + size) < - sizeof(vio->mmio_large_read)) ) + ((bytes + size) <= sizeof(vio->mmio_large_read)) ) { - memcpy(&vio->mmio_large_read[addr - pa], p_data, size); + memcpy(&vio->mmio_large_read[bytes], p_data, size); vio->mmio_large_read_bytes += size; } } @@ -1151,9 +1163,13 @@ int hvm_emulate_one( ? sizeof(hvmemul_ctxt->insn_buf) : 0; hvmemul_ctxt->exn_pending = 0; + vio->mmio_retrying = vio->mmio_retry; + vio->mmio_retry = 0; rc = x86_emulate(&hvmemul_ctxt->ctxt, &hvm_emulate_ops); + if ( rc == X86EMUL_OKAY && vio->mmio_retry ) + rc = X86EMUL_RETRY; if ( rc != X86EMUL_RETRY ) vio->mmio_large_read_bytes = vio->mmio_large_write_bytes = 0; --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -294,12 +294,21 @@ void hvm_io_assist(void) static int dpci_ioport_read(uint32_t mport, ioreq_t *p) { - int i, step = p->df ? -p->size : p->size; + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; uint32_t data = 0; for ( i = 0; i < p->count; i++ ) { - switch ( p->size ) + if ( vio->mmio_retrying ) + { + if ( vio->mmio_large_read_bytes != p->size ) + return X86EMUL_UNHANDLEABLE; + memcpy(&data, vio->mmio_large_read, p->size); + vio->mmio_large_read_bytes = 0; + vio->mmio_retrying = 0; + } + else switch ( p->size ) { case 1: data = inb(mport); @@ -316,22 +325,51 @@ static int dpci_ioport_read(uint32_t mpo if ( p->data_is_ptr ) { - int ret; - ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); - if ( (ret == HVMCOPY_gfn_paged_out) || - (ret == HVMCOPY_gfn_shared) ) - return X86EMUL_RETRY; + switch ( hvm_copy_to_guest_phys(p->data + step * i, + &data, p->size) ) + { + case HVMCOPY_okay: + break; + case HVMCOPY_gfn_paged_out: + case HVMCOPY_gfn_shared: + rc = X86EMUL_RETRY; + break; + case HVMCOPY_bad_gfn_to_mfn: + /* Drop the write as real hardware would. */ + continue; + case HVMCOPY_bad_gva_to_gfn: + ASSERT(0); + /* fall through */ + default: + rc = X86EMUL_UNHANDLEABLE; + break; + } + if ( rc != X86EMUL_OKAY) + break; } else p->data = data; } - return X86EMUL_OKAY; + if ( rc == X86EMUL_RETRY ) + { + vio->mmio_retry = 1; + vio->mmio_large_read_bytes = p->size; + memcpy(vio->mmio_large_read, &data, p->size); + } + + if ( i != 0 ) + { + p->count = i; + rc = X86EMUL_OKAY; + } + + return rc; } static int dpci_ioport_write(uint32_t mport, ioreq_t *p) { - int i, step = p->df ? -p->size : p->size; + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; uint32_t data; for ( i = 0; i < p->count; i++ ) @@ -346,7 +384,8 @@ static int dpci_ioport_write(uint32_t mp break; case HVMCOPY_gfn_paged_out: case HVMCOPY_gfn_shared: - return X86EMUL_RETRY; + rc = X86EMUL_RETRY; + break; case HVMCOPY_bad_gfn_to_mfn: data = ~0; break; @@ -354,8 +393,11 @@ static int dpci_ioport_write(uint32_t mp ASSERT(0); /* fall through */ default: - return X86EMUL_UNHANDLEABLE; + rc = X86EMUL_UNHANDLEABLE; + break; } + if ( rc != X86EMUL_OKAY) + break; } switch ( p->size ) @@ -374,7 +416,16 @@ static int dpci_ioport_write(uint32_t mp } } - return X86EMUL_OKAY; + if ( rc == X86EMUL_RETRY ) + current->arch.hvm_vcpu.hvm_io.mmio_retry = 1; + + if ( i != 0 ) + { + p->count = i; + rc = X86EMUL_OKAY; + } + + return rc; } int dpci_ioport_intercept(ioreq_t *p) --- a/xen/include/asm-x86/hvm/vcpu.h +++ b/xen/include/asm-x86/hvm/vcpu.h @@ -66,6 +66,11 @@ struct hvm_vcpu_io { /* We may write up to m256 as a number of device-model transactions. */ unsigned int mmio_large_write_bytes; paddr_t mmio_large_write_pa; + /* + * For string instruction emulation we need to be able to signal a + * necessary retry through other than function return codes. + */ + bool_t mmio_retry, mmio_retrying; }; #define VMCX_EADDR (~0ULL) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-30 12:58 UTC
[PATCH 3/5] x86/HVM: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept
Building upon the extended retry logic we can now also make sure to not ignore errors resulting from writing data back to guest memory. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/intercept.c +++ b/xen/arch/x86/hvm/intercept.c @@ -47,6 +47,7 @@ static int hvm_mmio_access(struct vcpu * hvm_mmio_read_t read_handler, hvm_mmio_write_t write_handler) { + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; unsigned long data; int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; @@ -54,7 +55,16 @@ static int hvm_mmio_access(struct vcpu * { if ( p->dir == IOREQ_READ ) { - rc = read_handler(v, p->addr, p->size, &data); + if ( vio->mmio_retrying ) + { + if ( vio->mmio_large_read_bytes != p->size ) + return X86EMUL_UNHANDLEABLE; + memcpy(&data, vio->mmio_large_read, p->size); + vio->mmio_large_read_bytes = 0; + vio->mmio_retrying = 0; + } + else + rc = read_handler(v, p->addr, p->size, &data); p->data = data; } else /* p->dir == IOREQ_WRITE */ @@ -66,18 +76,48 @@ static int hvm_mmio_access(struct vcpu * { for ( i = 0; i < p->count; i++ ) { - int ret; - - rc = read_handler(v, p->addr + step * i, p->size, &data); - if ( rc != X86EMUL_OKAY ) - break; - ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); - if ( (ret == HVMCOPY_gfn_paged_out) || - (ret == HVMCOPY_gfn_shared) ) + if ( vio->mmio_retrying ) + { + if ( vio->mmio_large_read_bytes != p->size ) + return X86EMUL_UNHANDLEABLE; + memcpy(&data, vio->mmio_large_read, p->size); + vio->mmio_large_read_bytes = 0; + vio->mmio_retrying = 0; + } + else { + rc = read_handler(v, p->addr + step * i, p->size, &data); + if ( rc != X86EMUL_OKAY ) + break; + } + switch ( hvm_copy_to_guest_phys(p->data + step * i, + &data, p->size) ) + { + case HVMCOPY_okay: + break; + case HVMCOPY_gfn_paged_out: + case HVMCOPY_gfn_shared: rc = X86EMUL_RETRY; break; + case HVMCOPY_bad_gfn_to_mfn: + /* Drop the write as real hardware would. */ + continue; + case HVMCOPY_bad_gva_to_gfn: + ASSERT(0); + /* fall through */ + default: + rc = X86EMUL_UNHANDLEABLE; + break; } + if ( rc != X86EMUL_OKAY) + break; + } + + if ( rc == X86EMUL_RETRY ) + { + vio->mmio_retry = 1; + vio->mmio_large_read_bytes = p->size; + memcpy(vio->mmio_large_read, &data, p->size); } } else @@ -109,6 +149,9 @@ static int hvm_mmio_access(struct vcpu * if ( rc != X86EMUL_OKAY ) break; } + + if ( rc == X86EMUL_RETRY ) + vio->mmio_retry = 1; } if ( i != 0 ) @@ -137,6 +180,7 @@ int hvm_mmio_intercept(ioreq_t *p) static int process_portio_intercept(portio_action_t action, ioreq_t *p) { + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; uint32_t data; @@ -144,7 +188,16 @@ static int process_portio_intercept(port { if ( p->dir == IOREQ_READ ) { - rc = action(IOREQ_READ, p->addr, p->size, &data); + if ( vio->mmio_retrying ) + { + if ( vio->mmio_large_read_bytes != p->size ) + return X86EMUL_UNHANDLEABLE; + memcpy(&data, vio->mmio_large_read, p->size); + vio->mmio_large_read_bytes = 0; + vio->mmio_retrying = 0; + } + else + rc = action(IOREQ_READ, p->addr, p->size, &data); p->data = data; } else @@ -159,10 +212,48 @@ static int process_portio_intercept(port { for ( i = 0; i < p->count; i++ ) { - rc = action(IOREQ_READ, p->addr, p->size, &data); - if ( rc != X86EMUL_OKAY ) + if ( vio->mmio_retrying ) + { + if ( vio->mmio_large_read_bytes != p->size ) + return X86EMUL_UNHANDLEABLE; + memcpy(&data, vio->mmio_large_read, p->size); + vio->mmio_large_read_bytes = 0; + vio->mmio_retrying = 0; + } + else + { + rc = action(IOREQ_READ, p->addr, p->size, &data); + if ( rc != X86EMUL_OKAY ) + break; + } + switch ( hvm_copy_to_guest_phys(p->data + step * i, + &data, p->size) ) + { + case HVMCOPY_okay: + break; + case HVMCOPY_gfn_paged_out: + case HVMCOPY_gfn_shared: + rc = X86EMUL_RETRY; break; - (void)hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); + case HVMCOPY_bad_gfn_to_mfn: + /* Drop the write as real hardware would. */ + continue; + case HVMCOPY_bad_gva_to_gfn: + ASSERT(0); + /* fall through */ + default: + rc = X86EMUL_UNHANDLEABLE; + break; + } + if ( rc != X86EMUL_OKAY) + break; + } + + if ( rc == X86EMUL_RETRY ) + { + vio->mmio_retry = 1; + vio->mmio_large_read_bytes = p->size; + memcpy(vio->mmio_large_read, &data, p->size); } } else /* p->dir == IOREQ_WRITE */ @@ -195,6 +286,9 @@ static int process_portio_intercept(port if ( rc != X86EMUL_OKAY ) break; } + + if ( rc == X86EMUL_RETRY ) + vio->mmio_retry = 1; } if ( i != 0 ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-30 12:58 UTC
[PATCH 4/5] x86/HVM: properly deal with hvm_copy_*_guest_phys() errors
In memory read/write handling the default case should tell the caller that the operation cannot be handled rather than the operation having succeeded, so that when new HVMCOPY_* states get added not handling them explicitly will not result in errors being ignored. In task switch emulation code stop handling some errors, but not others. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -504,10 +504,10 @@ static int __hvmemul_read( switch ( rc ) { + case HVMCOPY_okay: + break; case HVMCOPY_bad_gva_to_gfn: return X86EMUL_EXCEPTION; - case HVMCOPY_unhandleable: - return X86EMUL_UNHANDLEABLE; case HVMCOPY_bad_gfn_to_mfn: if ( access_type == hvm_access_insn_fetch ) return X86EMUL_UNHANDLEABLE; @@ -535,11 +535,10 @@ static int __hvmemul_read( } return rc; case HVMCOPY_gfn_paged_out: - return X86EMUL_RETRY; case HVMCOPY_gfn_shared: return X86EMUL_RETRY; default: - break; + return X86EMUL_UNHANDLEABLE; } return X86EMUL_OKAY; @@ -634,10 +633,10 @@ static int hvmemul_write( switch ( rc ) { + case HVMCOPY_okay: + break; case HVMCOPY_bad_gva_to_gfn: return X86EMUL_EXCEPTION; - case HVMCOPY_unhandleable: - return X86EMUL_UNHANDLEABLE; case HVMCOPY_bad_gfn_to_mfn: rc = hvmemul_linear_to_phys(addr, &gpa, chunk, &reps, pfec, hvmemul_ctxt); @@ -663,11 +662,10 @@ static int hvmemul_write( } return rc; case HVMCOPY_gfn_paged_out: - return X86EMUL_RETRY; case HVMCOPY_gfn_shared: return X86EMUL_RETRY; default: - break; + return X86EMUL_UNHANDLEABLE; } return X86EMUL_OKAY; --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2345,11 +2345,7 @@ void hvm_task_switch( rc = hvm_copy_to_guest_virt( prev_tr.base, &tss, sizeof(tss), PFEC_page_present); - if ( rc == HVMCOPY_bad_gva_to_gfn ) - goto out; - if ( rc == HVMCOPY_gfn_paged_out ) - goto out; - if ( rc == HVMCOPY_gfn_shared ) + if ( rc != HVMCOPY_okay ) goto out; rc = hvm_copy_from_guest_virt( @@ -2396,9 +2392,7 @@ void hvm_task_switch( tr.base, &tss, sizeof(tss), PFEC_page_present); if ( rc == HVMCOPY_bad_gva_to_gfn ) exn_raised = 1; - if ( rc == HVMCOPY_gfn_paged_out ) - goto out; - if ( rc == HVMCOPY_gfn_shared ) + else if ( rc != HVMCOPY_okay ) goto out; if ( (tss.trace & 1) && !exn_raised ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Sep-30 12:59 UTC
[PATCH 5/5] x86/HVM: cache emulated instruction for retry processing
Rather than re-reading the instruction bytes upon retry processing, stash away and re-use tha what we already read. That way we can be certain that the retry won''t do something different from what requested the retry, getting once again closer to real hardware behavior (where what we use retries for is simply a bus operation, not involving redundant decoding of instructions). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -569,9 +569,19 @@ static int hvmemul_insn_fetch( /* Fall back if requested bytes are not in the prefetch cache. */ if ( unlikely((insn_off + bytes) > hvmemul_ctxt->insn_buf_bytes) ) - return __hvmemul_read( - seg, offset, p_data, bytes, - hvm_access_insn_fetch, hvmemul_ctxt); + { + int rc = __hvmemul_read(seg, offset, p_data, bytes, + hvm_access_insn_fetch, hvmemul_ctxt); + + if ( rc == X86EMUL_OKAY ) + { + ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf)); + memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes); + hvmemul_ctxt->insn_buf_bytes = insn_off + bytes; + } + + return rc; + } /* Hit the cache. Simple memcpy. */ memcpy(p_data, &hvmemul_ctxt->insn_buf[insn_off], bytes); @@ -1148,17 +1158,27 @@ int hvm_emulate_one( pfec |= PFEC_user_mode; hvmemul_ctxt->insn_buf_eip = regs->eip; - hvmemul_ctxt->insn_buf_bytes - hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) - ? : - (hvm_virtual_to_linear_addr( - x86_seg_cs, &hvmemul_ctxt->seg_reg[x86_seg_cs], - regs->eip, sizeof(hvmemul_ctxt->insn_buf), - hvm_access_insn_fetch, hvmemul_ctxt->ctxt.addr_size, &addr) && - !hvm_fetch_from_guest_virt_nofault( - hvmemul_ctxt->insn_buf, addr, - sizeof(hvmemul_ctxt->insn_buf), pfec)) - ? sizeof(hvmemul_ctxt->insn_buf) : 0; + if ( !vio->mmio_insn_bytes ) + { + hvmemul_ctxt->insn_buf_bytes + hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?: + (hvm_virtual_to_linear_addr(x86_seg_cs, + &hvmemul_ctxt->seg_reg[x86_seg_cs], + regs->eip, + sizeof(hvmemul_ctxt->insn_buf), + hvm_access_insn_fetch, + hvmemul_ctxt->ctxt.addr_size, + &addr) && + hvm_fetch_from_guest_virt_nofault(hvmemul_ctxt->insn_buf, addr, + sizeof(hvmemul_ctxt->insn_buf), + pfec) == HVMCOPY_okay) ? + sizeof(hvmemul_ctxt->insn_buf) : 0; + } + else + { + hvmemul_ctxt->insn_buf_bytes = vio->mmio_insn_bytes; + memcpy(hvmemul_ctxt->insn_buf, vio->mmio_insn, vio->mmio_insn_bytes); + } hvmemul_ctxt->exn_pending = 0; vio->mmio_retrying = vio->mmio_retry; @@ -1169,7 +1189,16 @@ int hvm_emulate_one( if ( rc == X86EMUL_OKAY && vio->mmio_retry ) rc = X86EMUL_RETRY; if ( rc != X86EMUL_RETRY ) + { vio->mmio_large_read_bytes = vio->mmio_large_write_bytes = 0; + vio->mmio_insn_bytes = 0; + } + else + { + BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt->insn_buf)); + vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes; + memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); + } if ( rc != X86EMUL_OKAY ) return rc; --- a/xen/include/asm-x86/hvm/vcpu.h +++ b/xen/include/asm-x86/hvm/vcpu.h @@ -66,6 +66,9 @@ struct hvm_vcpu_io { /* We may write up to m256 as a number of device-model transactions. */ unsigned int mmio_large_write_bytes; paddr_t mmio_large_write_pa; + /* For retries we shouldn''t re-fetch the instruction. */ + unsigned int mmio_insn_bytes; + unsigned char mmio_insn[16]; /* * For string instruction emulation we need to be able to signal a * necessary retry through other than function return codes. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Andrew Cooper
2013-Sep-30 13:07 UTC
Re: [PATCH 4/5] x86/HVM: properly deal with hvm_copy_*_guest_phys() errors
On 30/09/13 13:58, Jan Beulich wrote:> In memory read/write handling the default case should tell the caller > that the operation cannot be handled rather than the operation having > succeeded, so that when new HVMCOPY_* states get added not handling > them explicitly will not result in errors being ignored. > > In task switch emulation code stop handling some errors, but not > others. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -504,10 +504,10 @@ static int __hvmemul_read( > > switch ( rc ) > { > + case HVMCOPY_okay: > + break; > case HVMCOPY_bad_gva_to_gfn: > return X86EMUL_EXCEPTION; > - case HVMCOPY_unhandleable: > - return X86EMUL_UNHANDLEABLE; > case HVMCOPY_bad_gfn_to_mfn: > if ( access_type == hvm_access_insn_fetch ) > return X86EMUL_UNHANDLEABLE; > @@ -535,11 +535,10 @@ static int __hvmemul_read( > } > return rc; > case HVMCOPY_gfn_paged_out: > - return X86EMUL_RETRY; > case HVMCOPY_gfn_shared: > return X86EMUL_RETRY; > default: > - break; > + return X86EMUL_UNHANDLEABLE; > } > > return X86EMUL_OKAY; > @@ -634,10 +633,10 @@ static int hvmemul_write( > > switch ( rc ) > { > + case HVMCOPY_okay: > + break; > case HVMCOPY_bad_gva_to_gfn: > return X86EMUL_EXCEPTION; > - case HVMCOPY_unhandleable: > - return X86EMUL_UNHANDLEABLE; > case HVMCOPY_bad_gfn_to_mfn: > rc = hvmemul_linear_to_phys(addr, &gpa, chunk, &reps, pfec, > hvmemul_ctxt); > @@ -663,11 +662,10 @@ static int hvmemul_write( > } > return rc; > case HVMCOPY_gfn_paged_out: > - return X86EMUL_RETRY; > case HVMCOPY_gfn_shared: > return X86EMUL_RETRY; > default: > - break; > + return X86EMUL_UNHANDLEABLE; > } > > return X86EMUL_OKAY; > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2345,11 +2345,7 @@ void hvm_task_switch( > > rc = hvm_copy_to_guest_virt( > prev_tr.base, &tss, sizeof(tss), PFEC_page_present); > - if ( rc == HVMCOPY_bad_gva_to_gfn ) > - goto out; > - if ( rc == HVMCOPY_gfn_paged_out ) > - goto out; > - if ( rc == HVMCOPY_gfn_shared ) > + if ( rc != HVMCOPY_okay ) > goto out; > > rc = hvm_copy_from_guest_virt( > @@ -2396,9 +2392,7 @@ void hvm_task_switch( > tr.base, &tss, sizeof(tss), PFEC_page_present); > if ( rc == HVMCOPY_bad_gva_to_gfn ) > exn_raised = 1; > - if ( rc == HVMCOPY_gfn_paged_out ) > - goto out; > - if ( rc == HVMCOPY_gfn_shared ) > + else if ( rc != HVMCOPY_okay ) > goto out; > > if ( (tss.trace & 1) && !exn_raised ) > > > > > > _______________________________________________ > 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 30.09.13 at 14:51, "Jan Beulich" <JBeulich@suse.com> wrote: > 1: properly handle backward string instruction emulation > 2: fix direct PCI port I/O emulation retry and error handling > 3: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept > 4: properly deal with hvm_copy_*_guest_phys() errors > 5: cache emulated instruction for retry processing > > Signed-off-by: Jan Beulich <jbeulich@suse.com>The only response so far was a review of patch 4 by Andrew... Jan
Andrew Cooper
2013-Oct-08 16:36 UTC
Re: [PATCH 1/5] x86/HVM: properly handle backward string instruction emulation
On 30/09/13 13:57, Jan Beulich wrote:> Multiplying a signed 32-bit quantity with an unsigned 32-bit quantity > produces an unsigned 32-bit result, yet for emulation of backward > string instructions we need the result sign extended before getting > added to the base address. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> > --- a/xen/arch/x86/hvm/intercept.c > +++ b/xen/arch/x86/hvm/intercept.c > @@ -48,7 +48,7 @@ static int hvm_mmio_access(struct vcpu * > hvm_mmio_write_t write_handler) > { > unsigned long data; > - int rc = X86EMUL_OKAY, i, sign = p->df ? -1 : 1; > + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; > > if ( !p->data_is_ptr ) > { > @@ -68,13 +68,10 @@ static int hvm_mmio_access(struct vcpu * > { > int ret; > > - rc = read_handler(v, p->addr + (sign * i * p->size), p->size, > - &data); > + rc = read_handler(v, p->addr + step * i, p->size, &data); > if ( rc != X86EMUL_OKAY ) > break; > - ret = hvm_copy_to_guest_phys(p->data + (sign * i * p->size), > - &data, > - p->size); > + ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > if ( (ret == HVMCOPY_gfn_paged_out) || > (ret == HVMCOPY_gfn_shared) ) > { > @@ -87,8 +84,7 @@ static int hvm_mmio_access(struct vcpu * > { > for ( i = 0; i < p->count; i++ ) > { > - switch ( hvm_copy_from_guest_phys(&data, > - p->data + sign * i * p->size, > + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, > p->size) ) > { > case HVMCOPY_okay: > @@ -109,8 +105,7 @@ static int hvm_mmio_access(struct vcpu * > } > if ( rc != X86EMUL_OKAY ) > break; > - rc = write_handler(v, p->addr + (sign * i * p->size), p->size, > - data); > + rc = write_handler(v, p->addr + step * i, p->size, data); > if ( rc != X86EMUL_OKAY ) > break; > } > @@ -142,7 +137,7 @@ int hvm_mmio_intercept(ioreq_t *p) > > static int process_portio_intercept(portio_action_t action, ioreq_t *p) > { > - int rc = X86EMUL_OKAY, i, sign = p->df ? -1 : 1; > + int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; > uint32_t data; > > if ( !p->data_is_ptr ) > @@ -167,8 +162,7 @@ static int process_portio_intercept(port > rc = action(IOREQ_READ, p->addr, p->size, &data); > if ( rc != X86EMUL_OKAY ) > break; > - (void)hvm_copy_to_guest_phys(p->data + sign*i*p->size, > - &data, p->size); > + (void)hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > } > } > else /* p->dir == IOREQ_WRITE */ > @@ -176,8 +170,7 @@ static int process_portio_intercept(port > for ( i = 0; i < p->count; i++ ) > { > data = 0; > - switch ( hvm_copy_from_guest_phys(&data, > - p->data + sign * i * p->size, > + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, > p->size) ) > { > case HVMCOPY_okay: > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -294,7 +294,7 @@ void hvm_io_assist(void) > > static int dpci_ioport_read(uint32_t mport, ioreq_t *p) > { > - int i, sign = p->df ? -1 : 1; > + int i, step = p->df ? -p->size : p->size; > uint32_t data = 0; > > for ( i = 0; i < p->count; i++ ) > @@ -317,8 +317,7 @@ static int dpci_ioport_read(uint32_t mpo > if ( p->data_is_ptr ) > { > int ret; > - ret = hvm_copy_to_guest_phys(p->data + (sign * i * p->size), &data, > - p->size); > + ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > if ( (ret == HVMCOPY_gfn_paged_out) || > (ret == HVMCOPY_gfn_shared) ) > return X86EMUL_RETRY; > @@ -332,7 +331,7 @@ static int dpci_ioport_read(uint32_t mpo > > static int dpci_ioport_write(uint32_t mport, ioreq_t *p) > { > - int i, sign = p->df ? -1 : 1; > + int i, step = p->df ? -p->size : p->size; > uint32_t data; > > for ( i = 0; i < p->count; i++ ) > @@ -340,8 +339,7 @@ static int dpci_ioport_write(uint32_t mp > data = p->data; > if ( p->data_is_ptr ) > { > - switch ( hvm_copy_from_guest_phys(&data, > - p->data + sign * i * p->size, > + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, > p->size) ) > { > case HVMCOPY_okay: > --- a/xen/arch/x86/hvm/stdvga.c > +++ b/xen/arch/x86/hvm/stdvga.c > @@ -467,15 +467,17 @@ static uint32_t read_data; > static int mmio_move(struct hvm_hw_stdvga *s, ioreq_t *p) > { > int i; > - int sign = p->df ? -1 : 1; > + uint64_t addr = p->addr; > p2m_type_t p2mt; > struct domain *d = current->domain; > > if ( p->data_is_ptr ) > { > + uint64_t data = p->data, tmp; > + int step = p->df ? -p->size : p->size; > + > if ( p->dir == IOREQ_READ ) > { > - uint64_t addr = p->addr, data = p->data, tmp; > for ( i = 0; i < p->count; i++ ) > { > tmp = stdvga_mem_read(addr, p->size); > @@ -498,13 +500,12 @@ static int mmio_move(struct hvm_hw_stdvg > ASSERT(!dp); > stdvga_mem_write(data, tmp, p->size); > } > - data += sign * p->size; > - addr += sign * p->size; > + data += step; > + addr += step; > } > } > else > { > - uint32_t addr = p->addr, data = p->data, tmp; > for ( i = 0; i < p->count; i++ ) > { > if ( hvm_copy_from_guest_phys(&tmp, data, p->size) !> @@ -523,31 +524,18 @@ static int mmio_move(struct hvm_hw_stdvg > tmp = stdvga_mem_read(data, p->size); > } > stdvga_mem_write(addr, tmp, p->size); > - data += sign * p->size; > - addr += sign * p->size; > + data += step; > + addr += step; > } > } > } > else > { > + ASSERT(p->count == 1); > if ( p->dir == IOREQ_READ ) > - { > - uint32_t addr = p->addr; > - for ( i = 0; i < p->count; i++ ) > - { > - p->data = stdvga_mem_read(addr, p->size); > - addr += sign * p->size; > - } > - } > + p->data = stdvga_mem_read(addr, p->size); > else > - { > - uint32_t addr = p->addr; > - for ( i = 0; i < p->count; i++ ) > - { > - stdvga_mem_write(addr, p->data, p->size); > - addr += sign * p->size; > - } > - } > + stdvga_mem_write(addr, p->data, p->size); > } > > read_data = p->data; > > > > > _______________________________________________ > 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
Andrew Cooper
2013-Oct-08 18:13 UTC
Re: [PATCH 2/5] x86/HVM: fix direct PCI port I/O emulation retry and error handling
On 30/09/13 13:57, Jan Beulich wrote:> dpci_ioport_{read,write}() guest memory access failure handling should > be modelled after process_portio_intercept()''s (and others): Upon > encountering an error on other than the first iteration, the count > successfully handled needs to be stored and X86EMUL_OKAY returned, in > order for the generic instruction emulator to update register state > correctly before reporting failure or retrying (both of which would > only happen after re-invoking emulation). > > Further we leverage (and slightly extend, due to the above mentioned > need to return X86EMUL_OKAY) the "large MMIO" retry model. > > Note that there is still a special case not explicitly taken care of > here: While the first retry on the last iteration of a "rep ins" > correctly recovers the already read data, an eventual subsequent retry > is being handled by the pre-existing mmio-large logic (through > hvmemul_do_io() storing the [recovered] data [again], also taking into > consideration that the emulator converts a single iteration "ins" to > ->read_io() plus ->write()). > > Also fix an off-by-one in the mmio-large-read logic, and slightly > simplify the copying of the data. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> One trivial thought comes to mind which you could easily do when committing the patch...> @@ -316,22 +325,51 @@ static int dpci_ioport_read(uint32_t mpo > > if ( p->data_is_ptr ) > { > - int ret; > - ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > - if ( (ret == HVMCOPY_gfn_paged_out) || > - (ret == HVMCOPY_gfn_shared) ) > - return X86EMUL_RETRY; > + switch ( hvm_copy_to_guest_phys(p->data + step * i, > + &data, p->size) ) > + { > + case HVMCOPY_okay: > + break; > + case HVMCOPY_gfn_paged_out: > + case HVMCOPY_gfn_shared: > + rc = X86EMUL_RETRY; > + break; > + case HVMCOPY_bad_gfn_to_mfn: > + /* Drop the write as real hardware would. */ > + continue; > + case HVMCOPY_bad_gva_to_gfn: > + ASSERT(0); > + /* fall through */ > + default: > + rc = X86EMUL_UNHANDLEABLE; > + break; > + } > + if ( rc != X86EMUL_OKAY) > + break; > } > else > p->data = data; > } >Nuke the trailing whitespace on the line above here, which will fractionally increase the size of the hunk below. ~Andrew> - return X86EMUL_OKAY; > + if ( rc == X86EMUL_RETRY ) > + { > + vio->mmio_retry = 1; > + vio->mmio_large_read_bytes = p->size; > + memcpy(vio->mmio_large_read, &data, p->size); > + } > + > + if ( i != 0 ) > + { > + p->count = i; > + rc = X86EMUL_OKAY; > + } > + > + return rc; > } >
Andrew Cooper
2013-Oct-08 18:20 UTC
Re: [PATCH 3/5] x86/HVM: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept
On 30/09/13 13:58, Jan Beulich wrote:> Building upon the extended retry logic we can now also make sure to > not ignore errors resulting from writing data back to guest memory. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/intercept.c > +++ b/xen/arch/x86/hvm/intercept.c > @@ -47,6 +47,7 @@ static int hvm_mmio_access(struct vcpu * > hvm_mmio_read_t read_handler, > hvm_mmio_write_t write_handler) > { > + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io;struct vcpu is passed in to this function, which can be used in preference to current. (And is possibly fractionally faster unless the compiler can prove that v is always current) Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> unsigned long data; > int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; > > @@ -54,7 +55,16 @@ static int hvm_mmio_access(struct vcpu * > { > if ( p->dir == IOREQ_READ ) > { > - rc = read_handler(v, p->addr, p->size, &data); > + if ( vio->mmio_retrying ) > + { > + if ( vio->mmio_large_read_bytes != p->size ) > + return X86EMUL_UNHANDLEABLE; > + memcpy(&data, vio->mmio_large_read, p->size); > + vio->mmio_large_read_bytes = 0; > + vio->mmio_retrying = 0; > + } > + else > + rc = read_handler(v, p->addr, p->size, &data); > p->data = data; > } > else /* p->dir == IOREQ_WRITE */ > @@ -66,18 +76,48 @@ static int hvm_mmio_access(struct vcpu * > { > for ( i = 0; i < p->count; i++ ) > { > - int ret; > - > - rc = read_handler(v, p->addr + step * i, p->size, &data); > - if ( rc != X86EMUL_OKAY ) > - break; > - ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > - if ( (ret == HVMCOPY_gfn_paged_out) || > - (ret == HVMCOPY_gfn_shared) ) > + if ( vio->mmio_retrying ) > + { > + if ( vio->mmio_large_read_bytes != p->size ) > + return X86EMUL_UNHANDLEABLE; > + memcpy(&data, vio->mmio_large_read, p->size); > + vio->mmio_large_read_bytes = 0; > + vio->mmio_retrying = 0; > + } > + else > { > + rc = read_handler(v, p->addr + step * i, p->size, &data); > + if ( rc != X86EMUL_OKAY ) > + break; > + } > + switch ( hvm_copy_to_guest_phys(p->data + step * i, > + &data, p->size) ) > + { > + case HVMCOPY_okay: > + break; > + case HVMCOPY_gfn_paged_out: > + case HVMCOPY_gfn_shared: > rc = X86EMUL_RETRY; > break; > + case HVMCOPY_bad_gfn_to_mfn: > + /* Drop the write as real hardware would. */ > + continue; > + case HVMCOPY_bad_gva_to_gfn: > + ASSERT(0); > + /* fall through */ > + default: > + rc = X86EMUL_UNHANDLEABLE; > + break; > } > + if ( rc != X86EMUL_OKAY) > + break; > + } > + > + if ( rc == X86EMUL_RETRY ) > + { > + vio->mmio_retry = 1; > + vio->mmio_large_read_bytes = p->size; > + memcpy(vio->mmio_large_read, &data, p->size); > } > } > else > @@ -109,6 +149,9 @@ static int hvm_mmio_access(struct vcpu * > if ( rc != X86EMUL_OKAY ) > break; > } > + > + if ( rc == X86EMUL_RETRY ) > + vio->mmio_retry = 1; > } > > if ( i != 0 ) > @@ -137,6 +180,7 @@ int hvm_mmio_intercept(ioreq_t *p) > > static int process_portio_intercept(portio_action_t action, ioreq_t *p) > { > + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; > int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; > uint32_t data; > > @@ -144,7 +188,16 @@ static int process_portio_intercept(port > { > if ( p->dir == IOREQ_READ ) > { > - rc = action(IOREQ_READ, p->addr, p->size, &data); > + if ( vio->mmio_retrying ) > + { > + if ( vio->mmio_large_read_bytes != p->size ) > + return X86EMUL_UNHANDLEABLE; > + memcpy(&data, vio->mmio_large_read, p->size); > + vio->mmio_large_read_bytes = 0; > + vio->mmio_retrying = 0; > + } > + else > + rc = action(IOREQ_READ, p->addr, p->size, &data); > p->data = data; > } > else > @@ -159,10 +212,48 @@ static int process_portio_intercept(port > { > for ( i = 0; i < p->count; i++ ) > { > - rc = action(IOREQ_READ, p->addr, p->size, &data); > - if ( rc != X86EMUL_OKAY ) > + if ( vio->mmio_retrying ) > + { > + if ( vio->mmio_large_read_bytes != p->size ) > + return X86EMUL_UNHANDLEABLE; > + memcpy(&data, vio->mmio_large_read, p->size); > + vio->mmio_large_read_bytes = 0; > + vio->mmio_retrying = 0; > + } > + else > + { > + rc = action(IOREQ_READ, p->addr, p->size, &data); > + if ( rc != X86EMUL_OKAY ) > + break; > + } > + switch ( hvm_copy_to_guest_phys(p->data + step * i, > + &data, p->size) ) > + { > + case HVMCOPY_okay: > + break; > + case HVMCOPY_gfn_paged_out: > + case HVMCOPY_gfn_shared: > + rc = X86EMUL_RETRY; > break; > - (void)hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); > + case HVMCOPY_bad_gfn_to_mfn: > + /* Drop the write as real hardware would. */ > + continue; > + case HVMCOPY_bad_gva_to_gfn: > + ASSERT(0); > + /* fall through */ > + default: > + rc = X86EMUL_UNHANDLEABLE; > + break; > + } > + if ( rc != X86EMUL_OKAY) > + break; > + } > + > + if ( rc == X86EMUL_RETRY ) > + { > + vio->mmio_retry = 1; > + vio->mmio_large_read_bytes = p->size; > + memcpy(vio->mmio_large_read, &data, p->size); > } > } > else /* p->dir == IOREQ_WRITE */ > @@ -195,6 +286,9 @@ static int process_portio_intercept(port > if ( rc != X86EMUL_OKAY ) > break; > } > + > + if ( rc == X86EMUL_RETRY ) > + vio->mmio_retry = 1; > } > > if ( i != 0 ) > > > > > _______________________________________________ > 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
Jan Beulich
2013-Oct-09 07:48 UTC
Re: [PATCH 3/5] x86/HVM: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept
>>> On 08.10.13 at 20:20, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 30/09/13 13:58, Jan Beulich wrote: >> Building upon the extended retry logic we can now also make sure to >> not ignore errors resulting from writing data back to guest memory. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/hvm/intercept.c >> +++ b/xen/arch/x86/hvm/intercept.c >> @@ -47,6 +47,7 @@ static int hvm_mmio_access(struct vcpu * >> hvm_mmio_read_t read_handler, >> hvm_mmio_write_t write_handler) >> { >> + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; > > struct vcpu is passed in to this function, which can be used in > preference to current. (And is possibly fractionally faster unless the > compiler can prove that v is always current)Right, I simply overlooked this. Adjusted.> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>Thanks, Jan>> unsigned long data; >> int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; >> >> @@ -54,7 +55,16 @@ static int hvm_mmio_access(struct vcpu * >> { >> if ( p->dir == IOREQ_READ ) >> { >> - rc = read_handler(v, p->addr, p->size, &data); >> + if ( vio->mmio_retrying ) >> + { >> + if ( vio->mmio_large_read_bytes != p->size ) >> + return X86EMUL_UNHANDLEABLE; >> + memcpy(&data, vio->mmio_large_read, p->size); >> + vio->mmio_large_read_bytes = 0; >> + vio->mmio_retrying = 0; >> + } >> + else >> + rc = read_handler(v, p->addr, p->size, &data); >> p->data = data; >> } >> else /* p->dir == IOREQ_WRITE */ >> @@ -66,18 +76,48 @@ static int hvm_mmio_access(struct vcpu * >> { >> for ( i = 0; i < p->count; i++ ) >> { >> - int ret; >> - >> - rc = read_handler(v, p->addr + step * i, p->size, &data); >> - if ( rc != X86EMUL_OKAY ) >> - break; >> - ret = hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); >> - if ( (ret == HVMCOPY_gfn_paged_out) || >> - (ret == HVMCOPY_gfn_shared) ) >> + if ( vio->mmio_retrying ) >> + { >> + if ( vio->mmio_large_read_bytes != p->size ) >> + return X86EMUL_UNHANDLEABLE; >> + memcpy(&data, vio->mmio_large_read, p->size); >> + vio->mmio_large_read_bytes = 0; >> + vio->mmio_retrying = 0; >> + } >> + else >> { >> + rc = read_handler(v, p->addr + step * i, p->size, &data); >> + if ( rc != X86EMUL_OKAY ) >> + break; >> + } >> + switch ( hvm_copy_to_guest_phys(p->data + step * i, >> + &data, p->size) ) >> + { >> + case HVMCOPY_okay: >> + break; >> + case HVMCOPY_gfn_paged_out: >> + case HVMCOPY_gfn_shared: >> rc = X86EMUL_RETRY; >> break; >> + case HVMCOPY_bad_gfn_to_mfn: >> + /* Drop the write as real hardware would. */ >> + continue; >> + case HVMCOPY_bad_gva_to_gfn: >> + ASSERT(0); >> + /* fall through */ >> + default: >> + rc = X86EMUL_UNHANDLEABLE; >> + break; >> } >> + if ( rc != X86EMUL_OKAY) >> + break; >> + } >> + >> + if ( rc == X86EMUL_RETRY ) >> + { >> + vio->mmio_retry = 1; >> + vio->mmio_large_read_bytes = p->size; >> + memcpy(vio->mmio_large_read, &data, p->size); >> } >> } >> else >> @@ -109,6 +149,9 @@ static int hvm_mmio_access(struct vcpu * >> if ( rc != X86EMUL_OKAY ) >> break; >> } >> + >> + if ( rc == X86EMUL_RETRY ) >> + vio->mmio_retry = 1; >> } >> >> if ( i != 0 ) >> @@ -137,6 +180,7 @@ int hvm_mmio_intercept(ioreq_t *p) >> >> static int process_portio_intercept(portio_action_t action, ioreq_t *p) >> { >> + struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; >> int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; >> uint32_t data; >> >> @@ -144,7 +188,16 @@ static int process_portio_intercept(port >> { >> if ( p->dir == IOREQ_READ ) >> { >> - rc = action(IOREQ_READ, p->addr, p->size, &data); >> + if ( vio->mmio_retrying ) >> + { >> + if ( vio->mmio_large_read_bytes != p->size ) >> + return X86EMUL_UNHANDLEABLE; >> + memcpy(&data, vio->mmio_large_read, p->size); >> + vio->mmio_large_read_bytes = 0; >> + vio->mmio_retrying = 0; >> + } >> + else >> + rc = action(IOREQ_READ, p->addr, p->size, &data); >> p->data = data; >> } >> else >> @@ -159,10 +212,48 @@ static int process_portio_intercept(port >> { >> for ( i = 0; i < p->count; i++ ) >> { >> - rc = action(IOREQ_READ, p->addr, p->size, &data); >> - if ( rc != X86EMUL_OKAY ) >> + if ( vio->mmio_retrying ) >> + { >> + if ( vio->mmio_large_read_bytes != p->size ) >> + return X86EMUL_UNHANDLEABLE; >> + memcpy(&data, vio->mmio_large_read, p->size); >> + vio->mmio_large_read_bytes = 0; >> + vio->mmio_retrying = 0; >> + } >> + else >> + { >> + rc = action(IOREQ_READ, p->addr, p->size, &data); >> + if ( rc != X86EMUL_OKAY ) >> + break; >> + } >> + switch ( hvm_copy_to_guest_phys(p->data + step * i, >> + &data, p->size) ) >> + { >> + case HVMCOPY_okay: >> + break; >> + case HVMCOPY_gfn_paged_out: >> + case HVMCOPY_gfn_shared: >> + rc = X86EMUL_RETRY; >> break; >> - (void)hvm_copy_to_guest_phys(p->data + step * i, &data, p->size); >> + case HVMCOPY_bad_gfn_to_mfn: >> + /* Drop the write as real hardware would. */ >> + continue; >> + case HVMCOPY_bad_gva_to_gfn: >> + ASSERT(0); >> + /* fall through */ >> + default: >> + rc = X86EMUL_UNHANDLEABLE; >> + break; >> + } >> + if ( rc != X86EMUL_OKAY) >> + break; >> + } >> + >> + if ( rc == X86EMUL_RETRY ) >> + { >> + vio->mmio_retry = 1; >> + vio->mmio_large_read_bytes = p->size; >> + memcpy(vio->mmio_large_read, &data, p->size); >> } >> } >> else /* p->dir == IOREQ_WRITE */ >> @@ -195,6 +286,9 @@ static int process_portio_intercept(port >> if ( rc != X86EMUL_OKAY ) >> break; >> } >> + >> + if ( rc == X86EMUL_RETRY ) >> + vio->mmio_retry = 1; >> } >> >> if ( i != 0 ) >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel
Andrew Cooper
2013-Oct-10 11:35 UTC
Re: [PATCH 5/5] x86/HVM: cache emulated instruction for retry processing
On 30/09/13 13:59, Jan Beulich wrote:> Rather than re-reading the instruction bytes upon retry processing, > stash away and re-use tha what we already read. That way we can be > certain that the retry won''t do something different from what requested > the retry, getting once again closer to real hardware behavior (where > what we use retries for is simply a bus operation, not involving > redundant decoding of instructions). > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> It occurs to me that this caching is also needed in the shadow emulation path, but that is probably better in a separate patch/series than polluting this HVM series.> > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -569,9 +569,19 @@ static int hvmemul_insn_fetch( > > /* Fall back if requested bytes are not in the prefetch cache. */ > if ( unlikely((insn_off + bytes) > hvmemul_ctxt->insn_buf_bytes) ) > - return __hvmemul_read( > - seg, offset, p_data, bytes, > - hvm_access_insn_fetch, hvmemul_ctxt); > + { > + int rc = __hvmemul_read(seg, offset, p_data, bytes, > + hvm_access_insn_fetch, hvmemul_ctxt); > + > + if ( rc == X86EMUL_OKAY ) > + { > + ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf)); > + memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes); > + hvmemul_ctxt->insn_buf_bytes = insn_off + bytes; > + } > + > + return rc; > + } > > /* Hit the cache. Simple memcpy. */ > memcpy(p_data, &hvmemul_ctxt->insn_buf[insn_off], bytes); > @@ -1148,17 +1158,27 @@ int hvm_emulate_one( > pfec |= PFEC_user_mode; > > hvmemul_ctxt->insn_buf_eip = regs->eip; > - hvmemul_ctxt->insn_buf_bytes > - hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) > - ? : > - (hvm_virtual_to_linear_addr( > - x86_seg_cs, &hvmemul_ctxt->seg_reg[x86_seg_cs], > - regs->eip, sizeof(hvmemul_ctxt->insn_buf), > - hvm_access_insn_fetch, hvmemul_ctxt->ctxt.addr_size, &addr) && > - !hvm_fetch_from_guest_virt_nofault( > - hvmemul_ctxt->insn_buf, addr, > - sizeof(hvmemul_ctxt->insn_buf), pfec)) > - ? sizeof(hvmemul_ctxt->insn_buf) : 0; > + if ( !vio->mmio_insn_bytes ) > + { > + hvmemul_ctxt->insn_buf_bytes > + hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?: > + (hvm_virtual_to_linear_addr(x86_seg_cs, > + &hvmemul_ctxt->seg_reg[x86_seg_cs], > + regs->eip, > + sizeof(hvmemul_ctxt->insn_buf), > + hvm_access_insn_fetch, > + hvmemul_ctxt->ctxt.addr_size, > + &addr) && > + hvm_fetch_from_guest_virt_nofault(hvmemul_ctxt->insn_buf, addr, > + sizeof(hvmemul_ctxt->insn_buf), > + pfec) == HVMCOPY_okay) ? > + sizeof(hvmemul_ctxt->insn_buf) : 0; > + } > + else > + { > + hvmemul_ctxt->insn_buf_bytes = vio->mmio_insn_bytes; > + memcpy(hvmemul_ctxt->insn_buf, vio->mmio_insn, vio->mmio_insn_bytes); > + } > > hvmemul_ctxt->exn_pending = 0; > vio->mmio_retrying = vio->mmio_retry; > @@ -1169,7 +1189,16 @@ int hvm_emulate_one( > if ( rc == X86EMUL_OKAY && vio->mmio_retry ) > rc = X86EMUL_RETRY; > if ( rc != X86EMUL_RETRY ) > + { > vio->mmio_large_read_bytes = vio->mmio_large_write_bytes = 0; > + vio->mmio_insn_bytes = 0; > + } > + else > + { > + BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt->insn_buf)); > + vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes; > + memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes); > + } > > if ( rc != X86EMUL_OKAY ) > return rc; > --- a/xen/include/asm-x86/hvm/vcpu.h > +++ b/xen/include/asm-x86/hvm/vcpu.h > @@ -66,6 +66,9 @@ struct hvm_vcpu_io { > /* We may write up to m256 as a number of device-model transactions. */ > unsigned int mmio_large_write_bytes; > paddr_t mmio_large_write_pa; > + /* For retries we shouldn''t re-fetch the instruction. */ > + unsigned int mmio_insn_bytes; > + unsigned char mmio_insn[16]; > /* > * For string instruction emulation we need to be able to signal a > * necessary retry through other than function return codes. > > > > > _______________________________________________ > 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 30/09/2013 13:51, "Jan Beulich" <JBeulich@suse.com> wrote:> 1: properly handle backward string instruction emulation > 2: fix direct PCI port I/O emulation retry and error handling > 3: don''t ignore hvm_copy_to_guest_phys() errors during I/O intercept > 4: properly deal with hvm_copy_*_guest_phys() errors > 5: cache emulated instruction for retry processing > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Acked-by: Keir Fraser <keir@xen.org>