Re: [PATCH RFC V6 1/5] xen: Emulate with no writes

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/12/2014 05:57 PM, Jan Beulich wrote:
>>>> On 11.08.14 at 17:08, <rcojocaru@bitdefender.com> wrote:
>> +static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
>> +    .read          = hvmemul_read,
>> +    .insn_fetch    = hvmemul_insn_fetch,
>> +    .write         = hvmemul_write_discard,
>> +    .cmpxchg       = hvmemul_cmpxchg_discard,
>> +    .rep_ins       = hvmemul_rep_ins_discard,
>> +    .rep_outs      = hvmemul_rep_outs_discard,
>> +    .rep_movs      = hvmemul_rep_movs_discard,
>> +    .read_segment  = hvmemul_read_segment,
>> +    .write_segment = hvmemul_write_segment,
>> +    .read_io       = hvmemul_read_io_discard,
>> +    .write_io      = hvmemul_write_io_discard,
>> +    .read_cr       = hvmemul_read_cr,
>> +    .write_cr      = hvmemul_write_cr,
>> +    .read_msr      = hvmemul_read_msr,
>> +    .write_msr     = hvmemul_write_msr,
>> +    .wbinvd        = hvmemul_wbinvd,
> 
> How about these last two?

It would likely be safer to provide discard versions of those as well,
thank you for pointing that out.

>> +void hvm_emulate_one_full(bool_t nowrite, unsigned int trapnr,
>> +    unsigned int errcode)
>> +{
>> +    struct hvm_emulate_ctxt ctx = {{ 0 }};
>> +    int rc;
>> +
>> +    hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>> +
>> +    if ( nowrite )
>> +        rc = hvm_emulate_one_no_write(&ctx);
>> +    else
>> +        rc = hvm_emulate_one(&ctx);
>> +
>> +    switch ( rc )
>> +    {
>> +    case X86EMUL_UNHANDLEABLE:
>> +        gdprintk(XENLOG_DEBUG, "Emulation failed @ %04x:%lx: "
>> +               "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
>> +               hvmemul_get_seg_reg(x86_seg_cs, &ctx)->sel,
>> +               ctx.insn_buf_eip,
>> +               ctx.insn_buf[0], ctx.insn_buf[1],
>> +               ctx.insn_buf[2], ctx.insn_buf[3],
>> +               ctx.insn_buf[4], ctx.insn_buf[5],
>> +               ctx.insn_buf[6], ctx.insn_buf[7],
>> +               ctx.insn_buf[8], ctx.insn_buf[9]);
>> +        hvm_inject_hw_exception(trapnr, errcode);
>> +        break;
>> +    case X86EMUL_EXCEPTION:
>> +        if ( ctx.exn_pending )
>> +            hvm_inject_hw_exception(ctx.exn_vector, ctx.exn_error_code);
>> +        /* fall through */
>> +    default:
>> +        hvm_emulate_writeback(&ctx);
> 
> Shouldn't this be pulled out of the switch to also cover the exception
> injection in the X86EMUL_UNHANDLEABLE case?

I'm not sure, that's the way it's handled in xen/arch/x86/hvm/io.c
(handle_mmio()):

 80 int handle_mmio(void)
 81 {
 82     struct hvm_emulate_ctxt ctxt;
 83     struct vcpu *curr = current;
 84     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
 85     int rc;
 86
 87     ASSERT(!is_pvh_vcpu(curr));
 88
 89     hvm_emulate_prepare(&ctxt, guest_cpu_user_regs());
 90
 91     rc = hvm_emulate_one(&ctxt);
 92
 93     if ( rc != X86EMUL_RETRY )
 94         vio->io_state = HVMIO_none;
 95     if ( vio->io_state == HVMIO_awaiting_completion )
 96         vio->io_state = HVMIO_handle_mmio_awaiting_completion;
 97     else
 98         vio->mmio_gva = 0;
 99
100     switch ( rc )
101     {
102     case X86EMUL_UNHANDLEABLE:
103         gdprintk(XENLOG_WARNING,
104                  "MMIO emulation failed @ %04x:%lx: "
105                  "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
106                  hvmemul_get_seg_reg(x86_seg_cs, &ctxt)->sel,
107                  ctxt.insn_buf_eip,
108                  ctxt.insn_buf[0], ctxt.insn_buf[1],
109                  ctxt.insn_buf[2], ctxt.insn_buf[3],
110                  ctxt.insn_buf[4], ctxt.insn_buf[5],
111                  ctxt.insn_buf[6], ctxt.insn_buf[7],
112                  ctxt.insn_buf[8], ctxt.insn_buf[9]);
113         return 0;
114     case X86EMUL_EXCEPTION:
115         if ( ctxt.exn_pending )
116             hvm_inject_hw_exception(ctxt.exn_vector,
ctxt.exn_error_code);
117         break;
118     default:
119         break;
120     }
121
122     hvm_emulate_writeback(&ctxt);
123
124     return 1;
125 }

There's a return there in the X86EMUL_UNHANDLEABLE case, so
hvm_emulate_writeback(&ctxt) doesn't get called.


Thanks,
Razvan Cojocaru