* Re: [RFC V0 PATCH 1/1] Replace handle_mmio calls in svm/vmx
2014-08-23 1:15 ` [RFC V0 PATCH 1/1] " Mukesh Rathor
@ 2014-08-23 13:26 ` Andrew Cooper
2014-08-25 8:28 ` Jan Beulich
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2014-08-23 13:26 UTC (permalink / raw)
To: Mukesh Rathor, jbeulich; +Cc: xen-devel, roger.pau
On 23/08/14 02:15, Mukesh Rathor wrote:
> svm/vmx IO and CR intercepts call handle_mmio to emulate which
> is slightly inappropriate. Create hvm_emulate() and replace those
> calls with hvm_emulate().
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
For cr/dr operations, this looks plausible (although I feel
"hvm_emulate()" is too generic for a name).
However, for ins/outs, this will break HVM. ins/outs need to get to the
pio handler in Xen, and for HVM, also to Qemu. Amongst other things,
our windows PV drivers use outs for efficient logging to the qemu debug
port.
~Andrew
> ---
> xen/arch/x86/hvm/emulate.c | 26 ++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 12 ++++++------
> xen/arch/x86/hvm/vmx/vmx.c | 10 +++-------
> xen/include/asm-x86/hvm/emulate.h | 1 +
> 4 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index eac159f..c568196 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1252,6 +1252,32 @@ void hvm_emulate_prepare(
> hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
> }
>
> +void hvm_emulate(struct cpu_user_regs *regs)
> +{
> + int rc;
> + struct hvm_emulate_ctxt ctxt;
> +
> + hvm_emulate_prepare(&ctxt, regs);
> + rc = hvm_emulate_one(&ctxt);
> +
> + switch ( rc )
> + {
> + case X86EMUL_UNHANDLEABLE:
> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + break;
> + case X86EMUL_EXCEPTION:
> + {
> + uint8_t vector = ctxt.exn_pending ? ctxt.exn_vector : TRAP_gp_fault;
> + int32_t errcode = ctxt.exn_pending ? ctxt.exn_error_code : 0;
> + hvm_inject_hw_exception(vector, errcode);
> + /* fall thru */
> + }
> + default:
> + hvm_emulate_writeback(&ctxt);
> + break;
> + }
> +}
> +
> void hvm_emulate_writeback(
> struct hvm_emulate_ctxt *hvmemul_ctxt)
> {
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 89df9f7..6c24ad2 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2475,16 +2475,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> if ( handle_pio(port, bytes, dir) )
> __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
> }
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
> break;
>
> case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
> case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
> if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
> svm_vmexit_do_cr_access(vmcb, regs);
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
> break;
>
> case VMEXIT_INVLPG:
> @@ -2493,8 +2493,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> svm_invlpg_intercept(vmcb->exitinfo1);
> __update_guest_eip(regs, vmcb->nextrip - vmcb->rip);
> }
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
> break;
>
> case VMEXIT_INVLPGA:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index dd969dc..415b73e 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3008,8 +3008,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> break;
>
> case EXIT_REASON_APIC_ACCESS:
> - if ( !vmx_handle_eoi_write() && !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + if ( !vmx_handle_eoi_write() )
> + hvm_emulate(regs);
> break;
>
> case EXIT_REASON_EOI_INDUCED:
> @@ -3026,11 +3026,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> case EXIT_REASON_IO_INSTRUCTION:
> __vmread(EXIT_QUALIFICATION, &exit_qualification);
> if ( exit_qualification & 0x10 )
> - {
> - /* INS, OUTS */
> - if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> - }
> + hvm_emulate(regs); /* INS, OUTS */
> else
> {
> /* IN, OUT */
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
> index 00a06cc..bc4a249 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -35,6 +35,7 @@ struct hvm_emulate_ctxt {
> uint32_t intr_shadow;
> };
>
> +void hvm_emulate(struct cpu_user_regs *regs);
> int hvm_emulate_one(
> struct hvm_emulate_ctxt *hvmemul_ctxt);
> void hvm_emulate_prepare(
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC V0 PATCH 1/1] Replace handle_mmio calls in svm/vmx
2014-08-23 1:15 ` [RFC V0 PATCH 1/1] " Mukesh Rathor
2014-08-23 13:26 ` Andrew Cooper
@ 2014-08-25 8:28 ` Jan Beulich
1 sibling, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2014-08-25 8:28 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: xen-devel, roger.pau
>>> On 23.08.14 at 03:15, <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1252,6 +1252,32 @@ void hvm_emulate_prepare(
> hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
> }
>
> +void hvm_emulate(struct cpu_user_regs *regs)
This surely is too generic a name.
> +{
> + int rc;
> + struct hvm_emulate_ctxt ctxt;
> +
> + hvm_emulate_prepare(&ctxt, regs);
> + rc = hvm_emulate_one(&ctxt);
As said in an earlier mail, I'm not sure this is the right thing to do
(namely continuing to use the full hvm_emulate_ops).
> + case X86EMUL_EXCEPTION:
> + {
> + uint8_t vector = ctxt.exn_pending ? ctxt.exn_vector : TRAP_gp_fault;
> + int32_t errcode = ctxt.exn_pending ? ctxt.exn_error_code : 0;
> + hvm_inject_hw_exception(vector, errcode);
This looks ugly. Either do this with if/else, or put the conditional
operators right as function arguments. (And style-wise if it
remained the way it is it would at least need a blank line between
declarations and statements.)
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2475,16 +2475,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> if ( handle_pio(port, bytes, dir) )
> __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
> }
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
As said in the earlier mail, I think there is a reason for this to call
handle_mmio().
> case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
> case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
> if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
> svm_vmexit_do_cr_access(vmcb, regs);
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
While this one indeed doesn't need it, nor does it (as said above)
need the full hvm_emulate_ops.
> @@ -2493,8 +2493,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> svm_invlpg_intercept(vmcb->exitinfo1);
> __update_guest_eip(regs, vmcb->nextrip - vmcb->rip);
> }
> - else if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + else
> + hvm_emulate(regs);
Same here.
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3008,8 +3008,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> break;
>
> case EXIT_REASON_APIC_ACCESS:
> - if ( !vmx_handle_eoi_write() && !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> + if ( !vmx_handle_eoi_write() )
> + hvm_emulate(regs);
How can this not need handle_mmio()? The LAPIC page is an
(emulated) MMIO one after all.
> @@ -3026,11 +3026,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> case EXIT_REASON_IO_INSTRUCTION:
> __vmread(EXIT_QUALIFICATION, &exit_qualification);
> if ( exit_qualification & 0x10 )
> - {
> - /* INS, OUTS */
> - if ( !handle_mmio() )
> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
> - }
> + hvm_emulate(regs); /* INS, OUTS */
Same as for AMD's IOIO case.
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread