Re: [Xen-devel] [PATCH 2/2] x86/vm_event: Allow returning i-cache for emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.16 at 18:20, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 13.09.16 at 20:12, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>> When emulating instructions the emulator maintains a small i-cache fetched
>>> from the guest memory. This patch extends the vm_event interface to allow
>>> returning this i-cache via the vm_event response instead.
>>
>> I guess I'm sightly confused: Isn't the purpose to have the monitoring
>> app _write_ to the cache maintained in Xen? Or else, who is
>> "emulator" here? If that's the app, then I think subject and description
>> for hypervisor patches would better be written taking the perspective
>> of the hypervisor, especially when using potentially ambiguous terms.
> 
> Well, I thought it was obvious that the built-in emulator in Xen is
> what this patch is referring to. Otherwise none of this makes sense.

It would have been if the sentence didn't say "returning". The
internal emulator's cache gets effectively overwritten, not
returned to anything (unless I'm still misunderstanding something).

>>> @@ -1944,11 +1951,19 @@ void hvm_mem_access_emulate_one(enum emul_kind 
>>> kind, unsigned int trapnr,
>>>      case EMUL_KIND_NOWRITE:
>>>          rc = hvm_emulate_one_no_write(&ctx);
>>>          break;
>>> -    case EMUL_KIND_SET_CONTEXT:
>>> -        ctx.set_context = 1;
>>> -        /* Intentional fall-through. */
>>> -    default:
>>> +    case EMUL_KIND_SET_CONTEXT_DATA:
>>> +        ctx.set_context_data = 1;
>>> +        rc = hvm_emulate_one(&ctx);
>>> +        break;
>>> +    case EMUL_KIND_SET_CONTEXT_INSN:
>>> +        ctx.set_context_insn = 1;
>>>          rc = hvm_emulate_one(&ctx);
>>> +        break;
>>> +    case EMUL_KIND_NORMAL:
>>> +        rc = hvm_emulate_one(&ctx);
>>> +        break;
>>
>> One of the former two surely can be made fall through into the latter?
> 
> That's what I did before by turning this into if-else's and you
> requested to go back to a switch. With a switch though I'll rather
> make the cases explicit as a fall-through just makes it harder to read
> for no real benefit.

I disagree.

>>> +    default:
>>> +        return;
>>
>> Why don't you retain the previous default handling?
> 
> The default label is unused and this makes it more apparent when
> reading the code.

Just like before, imo there shouldn't be any EMUL_KIND_NORMAL
case; that should be the default label instead.

>>> @@ -265,6 +272,10 @@ struct vm_event_emul_read_data {
>>>      uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
>>>  };
>>>
>>> +struct vm_event_emul_insn_data {
>>> +    uint8_t data[16]; /* Has to be completely filled */
>>> +};
>>
>> Any reason for the 16 (rather than the architectural 15) here?
> 
> Yes, see the definition in include/asm-x86/hvm/emulate.h:
> 
> struct hvm_emulate_ctxt {
>     struct x86_emulate_ctxt ctxt;
> 
>     /* Cache of 16 bytes of instruction. */
>     uint8_t insn_buf[16];

Ah, I didn't recall we have an oversized cache there too. But such a
connection would better be documented by a BUILD_BUG_ON()
comparing the two sizeof()s.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel