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 Wed, Sep 21, 2016 at 3:03 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 20.09.16 at 17:54, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> 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.
>
> No. Please look back at my original reply (still visible in context
> above). You're comparing apples and oranges - the existing override
> is an integral part of the emulation logic, while yours is an add-on.
> And btw., see how
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00897.html
> even factors out that part.
>
> It might even be an option to simply copy your override data right
> into vio->mmio_insn{,_bytes}, in the vm-event specific function,
> allowing all other code to remain untouched.

We can do that, though that seems to be a bit hack-ish. OTOH it would
not require any other code-changes, as you say, so if that's really an
option, I'm fine with it.

Tamas

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