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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 20.09.16 at 17:14, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 20.09.16 at 16:56, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>> 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.
>>>>
>>>> So that check is one that Razvan asked to be added. I think it is
>>>> necessary too as there seems to be a race-condition if vm_event gets
>>>> shutdown after the response flag is set but before this emulation path
>>>> takes place. Effectively set_context_insn may be set but the
>>>> arch.vm_event already gotten freed. Razvan, is that correct?
>>>
>>> Well, in case you misunderstood: I didn't ask for the check to be
>>> _removed_, but for it to be _moved elsewhere_.
>>>
>>
>> So as Razvan pointed out, there is a check already in hvm_do_resume
>> for exactly the same effect, so then what you are asking for is
>> already done.
>
> Partly - I really meant all curr->arch.vm_event uses to go away from
> that path. The other part (passing in the override buffer instead of
> special casing vm-event handling here) still would need to be done.
>

I don't really follow what exactly you are looking for. You want the
buffer to be sent in as an input? We can do that but I mean the mmio
case doesn't do that either.. And what do you mean not "special casing
vm_event handling"? We need to handle it in an if-statement because by
default the buffer is fetched from memory. We don't want to do that,
just as the mmio case doesn't want that either. So I think if we want
to be consistent we do what the mmio case is doing, fetching the
buffer from curr->arch.hvm_vcpu.hvm_io, only we fetch it from
curr->arch.vm_event.

Tamas

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