Re: [Xen-devel] [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 20:27, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 15.09.16 at 18:51, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>>>          pfec |= PFEC_user_mode;
>>>
>>>      hvmemul_ctxt->insn_buf_eip = regs->eip;
>>> -    if ( !vio->mmio_insn_bytes )
>>> +
>>> +    if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
>>> +    {
>>> +        BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
>>> +                     sizeof(curr->arch.vm_event->emul.insn));
>>
>> This should quite clearly be !=, and I think it builds only because you
>> use the wrong operand in the first sizeof().
>>
>>> +        hvmemul_ctxt->insn_buf_bytes = 
>>> sizeof(curr->arch.vm_event->emul.insn);
>>> +        memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
>>> +               hvmemul_ctxt->insn_buf_bytes);
>>
>> This memcpy()s between dissimilar types. Please omit the & and
>> properly add .data on the second argument (and this .data
>> addition should then also be mirrored in the BUILD_BUG_ON()).
>>
>>> +    }
>>> +    else if ( !vio->mmio_insn_bytes )
>>
>> And then - I'm sorry for not having thought of this before - I think
>> this would better not live here, or have an effect more explicitly
>> only when coming here through hvm_emulate_one_vm_event().
>> Since the former seems impractical, I think giving _hvm_emulate_one()
>> one or two extra parameters would be the most straightforward
>> approach.
> 
> So this is the spot where the mmio insn buffer is getting copied as
> well instead of fetching the instructions from the guest memory. So
> having the vm_event buffer getting copied here too makes the most
> sense. Having the vm_event insn buffer getting copied in somewhere
> else, while the mmio insn buffer getting copied here, IMHO just
> fragments the flow even more making it harder to see what is actually
> happening.

And I didn't unconditionally ask to move the copying elsewhere.
The alternative - passing the override in as function argument(s),
which would then be NULL/zero for all cases except the VM event
one, would be as suitable. It is in particular ...

> How about adjusting the if-else here to be:
> 
> if ( !vio->mmio_insn_bytes && !hvmemul_ctxt->set_context_insn  )
> ...
> else if ( vio->mmio_insn_bytes )
> ...
> else if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )

... this curr->arch.vm_event reference which I'd like to see gone
from this specific code path. The ordering in your original patch,
otoh, would then be fine (check for the override first with unlikely(),
else do what is being done today). Such a code structure would
then also ease a possible second way of overriding the insn by
some other party, without having to touch the code here again.

Jan


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