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

[Tamas K Lengyel]

On Wed, Sep 21, 2016 at 3:03 AM, Jan Beulich  wrote:
 On 20.09.16 at 17:54,  wrote:
>> On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich  wrote:
>> On 20.09.16 at 17:14,  wrote:
 On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:
 On 20.09.16 at 16:56,  wrote:
>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
>> On 19.09.16 at 20:27,  wrote:
 On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
 On 15.09.16 at 18:51,  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, 
>> >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 

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

[Jan Beulich]

>>> On 20.09.16 at 17:54,  wrote:
> On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich  wrote:
> On 20.09.16 at 17:14,  wrote:
>>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:
>>> On 20.09.16 at 16:56,  wrote:
> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
> On 19.09.16 at 20:27,  wrote:
>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
>>> On 15.09.16 at 18:51,  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, 
> >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 

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

[Tamas K Lengyel]

On Tue, Sep 20, 2016 at 9:39 AM, Jan Beulich  wrote:
 On 20.09.16 at 17:14,  wrote:
>> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:
>> On 20.09.16 at 16:56,  wrote:
 On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
 On 19.09.16 at 20:27,  wrote:
>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
>> On 15.09.16 at 18:51,  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, 
 >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 

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

[Jan Beulich]

>>> On 20.09.16 at 17:14,  wrote:
> On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:
> On 20.09.16 at 16:56,  wrote:
>>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
>>> On 19.09.16 at 20:27,  wrote:
> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
> On 15.09.16 at 18:51,  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, >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.

Jan

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

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

[Tamas K Lengyel]

On Mon, Sep 19, 2016 at 6:36 PM, Tian, Kevin  wrote:
>> From: Tamas K Lengyel
>> Sent: Tuesday, September 20, 2016 12:40 AM
>>
>> >> --- a/xen/arch/x86/hvm/hvm.c
>> >> +++ b/xen/arch/x86/hvm/hvm.c
>> >> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>> >>
>> >>  if ( v->arch.vm_event->emulate_flags &
>> >>   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> >> -kind = EMUL_KIND_SET_CONTEXT;
>> >> +kind = EMUL_KIND_SET_CONTEXT_DATA;
>> >>  else if ( v->arch.vm_event->emulate_flags &
>> >>VM_EVENT_FLAG_EMULATE_NOWRITE )
>> >>  kind = EMUL_KIND_NOWRITE;
>> >> +else if ( v->arch.vm_event->emulate_flags &
>> >> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> >> +kind = EMUL_KIND_SET_CONTEXT_INSN;
>> >
>> > The header talking about incompatibilities between these flags -
>> > wouldn't it be a good idea to actually -EINVAL such combinations?
>>
>> The header just points out that setting all these flags at the same
>> time won't have a "combined" effect - evident from the if-else
>> treatment above. There is really no point to do an error, the error
>> would never reach the user. Setting response flags through vm_event
>> doesn't have an error-path back.
>>
>
> Maybe you can have an explicit comment on priority of those flags,
> so consistent behavior can be expected moving forward. e.g. above
> code implies read_data>nowrite>insn_data. w/o a proper guidance
> here, future code changes by others may break that sequence...
>

Fair point, will do that.

Thanks,
Tamas

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

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

[Tamas K Lengyel]

On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:
 On 20.09.16 at 16:56,  wrote:
>> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
>> On 19.09.16 at 20:27,  wrote:
 On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
 On 15.09.16 at 18:51,  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, >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.

Tamas

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

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

[Jan Beulich]

>>> On 20.09.16 at 16:56,  wrote:
> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
> On 19.09.16 at 20:27,  wrote:
>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
>>> On 15.09.16 at 18:51,  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, >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_.

Jan


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

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

[Razvan Cojocaru]

On 09/20/2016 05:56 PM, Tamas K Lengyel wrote:
> On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
> On 19.09.16 at 20:27,  wrote:
>>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
>>> On 15.09.16 at 18:51,  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, >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?

Yes, that's the general idea, but there's also already a check in
hvm_do_resume() right before calling hvm_mem_access_emulate_one(), so as
long as that's the only code path leading here it should be alright to
remove the check. The check adds extra safety but it's not strictly
necessary here, so if Jan prefers it taken out it should, theoretically,
be fine for now.


Thanks,
Razvan

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

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

[Tamas K Lengyel]

On Tue, Sep 20, 2016 at 1:26 AM, Jan Beulich  wrote:
 On 19.09.16 at 20:27,  wrote:
>> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
>> On 15.09.16 at 18:51,  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, >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?

Tamas

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

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

[Jan Beulich]

>>> On 19.09.16 at 20:27,  wrote:
> On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
> On 15.09.16 at 18:51,  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, >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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Tian, Kevin]

> From: Tamas K Lengyel
> Sent: Tuesday, September 20, 2016 12:40 AM
> 
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
> >>
> >>  if ( v->arch.vm_event->emulate_flags &
> >>   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> >> -kind = EMUL_KIND_SET_CONTEXT;
> >> +kind = EMUL_KIND_SET_CONTEXT_DATA;
> >>  else if ( v->arch.vm_event->emulate_flags &
> >>VM_EVENT_FLAG_EMULATE_NOWRITE )
> >>  kind = EMUL_KIND_NOWRITE;
> >> +else if ( v->arch.vm_event->emulate_flags &
> >> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> >> +kind = EMUL_KIND_SET_CONTEXT_INSN;
> >
> > The header talking about incompatibilities between these flags -
> > wouldn't it be a good idea to actually -EINVAL such combinations?
> 
> The header just points out that setting all these flags at the same
> time won't have a "combined" effect - evident from the if-else
> treatment above. There is really no point to do an error, the error
> would never reach the user. Setting response flags through vm_event
> doesn't have an error-path back.
> 

Maybe you can have an explicit comment on priority of those flags,
so consistent behavior can be expected moving forward. e.g. above
code implies read_data>nowrite>insn_data. w/o a proper guidance
here, future code changes by others may break that sequence...

Thanks
Kevin
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Tamas K Lengyel]

On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
 On 15.09.16 at 18:51,  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, >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. 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 )

?

Tamas

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

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

[Tamas K Lengyel]

On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich  wrote:
 On 15.09.16 at 18:51,  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().

Yea.. I don't know what I was thinking =)

>
>> +hvmemul_ctxt->insn_buf_bytes = 
>> sizeof(curr->arch.vm_event->emul.insn);
>> +memcpy(hvmemul_ctxt->insn_buf, >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()).

Ack.

>
>> +}
>> +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.
>
>> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, 
>> unsigned int trapnr,
>>  case EMUL_KIND_NOWRITE:
>>  rc = hvm_emulate_one_no_write();
>>  break;
>> -case EMUL_KIND_SET_CONTEXT:
>> -ctx.set_context = 1;
>> -/* Intentional fall-through. */
>>  default:
>> +ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
>> +ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
>>  rc = hvm_emulate_one();
>> +break;
>>  }
>
> Thanks - this is a lot better readable now!
>
>> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
>>  hvmemul_ctxt->ctxt.force_writeback = 1;
>>  hvmemul_ctxt->seg_reg_accessed = 0;
>>  hvmemul_ctxt->seg_reg_dirty = 0;
>> -hvmemul_ctxt->set_context = 0;
>> +hvmemul_ctxt->set_context_data = 0;
>> +hvmemul_ctxt->set_context_insn = 0;
>
> I think you can almost guess the comment here and on the declaration
> of these fields: Please use false here and plain bool there.
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>>
>>  if ( v->arch.vm_event->emulate_flags &
>>   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> -kind = EMUL_KIND_SET_CONTEXT;
>> +kind = EMUL_KIND_SET_CONTEXT_DATA;
>>  else if ( v->arch.vm_event->emulate_flags &
>>VM_EVENT_FLAG_EMULATE_NOWRITE )
>>  kind = EMUL_KIND_NOWRITE;
>> +else if ( v->arch.vm_event->emulate_flags &
>> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> +kind = EMUL_KIND_SET_CONTEXT_INSN;
>
> The header talking about incompatibilities between these flags -
> wouldn't it be a good idea to actually -EINVAL such combinations?

The header just points out that setting all these flags at the same
time won't have a "combined" effect - evident from the if-else
treatment above. There is really no point to do an error, the error
would never reach the user. Setting response flags through vm_event
doesn't have an error-path back.

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -57,6 +57,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>
> I have to admit that looking through the header changes you do I
> can't figure why this adjustment is necessary.

Yea, it's just a residue (the patch was first made when this header
was still included).

>
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, 
>> vm_event_response_t *rsp)
>>  if ( p2m_mem_access_emulate_check(v, rsp) )
>>  {
>>  if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> -v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>> +v->arch.vm_event->emul.read = rsp->data.emul.read;
>>
>>  v->arch.vm_event->emulate_flags = rsp->flags;
>>  }
>>  break;
>> +case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
>> +if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> +{
>> +v->arch.vm_event->emul.insn = rsp->data.emul.insn;
>> +v->arch.vm_event->emulate_flags = rsp->flags;
>> +}
>> +break;
>>  default:
>
> Blank lines between non-fall-through 

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

[Jan Beulich]

>>> On 15.09.16 at 18:51,  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, >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.

> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, 
> unsigned int trapnr,
>  case EMUL_KIND_NOWRITE:
>  rc = hvm_emulate_one_no_write();
>  break;
> -case EMUL_KIND_SET_CONTEXT:
> -ctx.set_context = 1;
> -/* Intentional fall-through. */
>  default:
> +ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> +ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
>  rc = hvm_emulate_one();
> +break;
>  }

Thanks - this is a lot better readable now!

> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
>  hvmemul_ctxt->ctxt.force_writeback = 1;
>  hvmemul_ctxt->seg_reg_accessed = 0;
>  hvmemul_ctxt->seg_reg_dirty = 0;
> -hvmemul_ctxt->set_context = 0;
> +hvmemul_ctxt->set_context_data = 0;
> +hvmemul_ctxt->set_context_insn = 0;

I think you can almost guess the comment here and on the declaration
of these fields: Please use false here and plain bool there.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>  
>  if ( v->arch.vm_event->emulate_flags &
>   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -kind = EMUL_KIND_SET_CONTEXT;
> +kind = EMUL_KIND_SET_CONTEXT_DATA;
>  else if ( v->arch.vm_event->emulate_flags &
>VM_EVENT_FLAG_EMULATE_NOWRITE )
>  kind = EMUL_KIND_NOWRITE;
> +else if ( v->arch.vm_event->emulate_flags &
> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +kind = EMUL_KIND_SET_CONTEXT_INSN;

The header talking about incompatibilities between these flags -
wouldn't it be a good idea to actually -EINVAL such combinations?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -57,6 +57,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 

I have to admit that looking through the header changes you do I
can't figure why this adjustment is necessary.

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, 
> vm_event_response_t *rsp)
>  if ( p2m_mem_access_emulate_check(v, rsp) )
>  {
>  if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +v->arch.vm_event->emul.read = rsp->data.emul.read;
>  
>  v->arch.vm_event->emulate_flags = rsp->flags;
>  }
>  break;
> +case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> +if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +{
> +v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> +v->arch.vm_event->emulate_flags = rsp->flags;
> +}
> +break;
>  default:

Blank lines between non-fall-through case statements please (part
of me thinks I've said so before).

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>   * In some cases the response type needs extra handling, so here
>   * we call the appropriate handlers.
>   */
> -
>  /* Check flags which apply only when the vCPU is paused */

Stray change.

Jan

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

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

[Tamas K Lengyel]

On Sat, Sep 17, 2016 at 10:40 AM, Razvan Cojocaru
 wrote:
> On 09/15/16 19:51, Tamas K Lengyel wrote:
>> When emulating instructions Xen's emulator maintains a small i-cache fetched
>> from the guest memory. This patch extends the vm_event interface to allow
>> overwriting this i-cache via a buffer returned in the vm_event response.
>>
>> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
>> normally has to remove the INT3 from memory - singlestep - place back INT3
>> to allow the guest to continue execution. This routine however is susceptible
>> to a race-condition on multi-vCPU guests. By allowing the subscriber to 
>> return
>> the i-cache to be used for emulation it can side-step the problem by 
>> returning
>> a clean buffer without the INT3 present.
>>
>> As part of this patch we rename hvm_mem_access_emulate_one to
>> hvm_emulate_one_vm_event to better reflect that it is used in various 
>> vm_event
>> scenarios now, not just in response to mem_access events.
>>
>> Signed-off-by: Tamas K Lengyel 
>> ---
>> Cc: Paul Durrant 
>> Cc: Jan Beulich 
>> Cc: Andrew Cooper 
>> Cc: Jun Nakajima 
>> Cc: Kevin Tian 
>> Cc: George Dunlap 
>> Cc: Razvan Cojocaru 
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>>
>> v2: rework hvm_mem_access_emulate_one switch statement
>> add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match
>>
>> Note: this patch has now been fully tested and works as intended
>
> Acked-by: Razvan Cojocaru 
>
> On a side note, I see that you're using an email address that's
> different from the one in MAINTAINERS. Should we update the MAINTAINERS
> file?

It's fine for now (both go to the same place at the end anyway).

Tamas

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

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

[Razvan Cojocaru]

On 09/15/16 19:51, Tamas K Lengyel wrote:
> When emulating instructions Xen's emulator maintains a small i-cache fetched
> from the guest memory. This patch extends the vm_event interface to allow
> overwriting this i-cache via a buffer returned in the vm_event response.
> 
> When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
> normally has to remove the INT3 from memory - singlestep - place back INT3
> to allow the guest to continue execution. This routine however is susceptible
> to a race-condition on multi-vCPU guests. By allowing the subscriber to return
> the i-cache to be used for emulation it can side-step the problem by returning
> a clean buffer without the INT3 present.
> 
> As part of this patch we rename hvm_mem_access_emulate_one to
> hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
> scenarios now, not just in response to mem_access events.
> 
> Signed-off-by: Tamas K Lengyel 
> ---
> Cc: Paul Durrant 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Jun Nakajima 
> Cc: Kevin Tian 
> Cc: George Dunlap 
> Cc: Razvan Cojocaru 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> 
> v2: rework hvm_mem_access_emulate_one switch statement
> add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match
> 
> Note: this patch has now been fully tested and works as intended

Acked-by: Razvan Cojocaru 

On a side note, I see that you're using an email address that's
different from the one in MAINTAINERS. Should we update the MAINTAINERS
file?


Thanks,
Razvan

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

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

[Tamas K Lengyel]

When emulating instructions Xen's emulator maintains a small i-cache fetched
from the guest memory. This patch extends the vm_event interface to allow
overwriting this i-cache via a buffer returned in the vm_event response.

When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
normally has to remove the INT3 from memory - singlestep - place back INT3
to allow the guest to continue execution. This routine however is susceptible
to a race-condition on multi-vCPU guests. By allowing the subscriber to return
the i-cache to be used for emulation it can side-step the problem by returning
a clean buffer without the INT3 present.

As part of this patch we rename hvm_mem_access_emulate_one to
hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
scenarios now, not just in response to mem_access events.

Signed-off-by: Tamas K Lengyel 
---
Cc: Paul Durrant 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Jun Nakajima 
Cc: Kevin Tian 
Cc: George Dunlap 
Cc: Razvan Cojocaru 
Cc: Stefano Stabellini 
Cc: Julien Grall 

v2: rework hvm_mem_access_emulate_one switch statement
add BUILD_BUG_ON to ensure internal and vm_event buffer sizes match

Note: this patch has now been fully tested and works as intended
---
 xen/arch/x86/hvm/emulate.c| 37 -
 xen/arch/x86/hvm/hvm.c|  9 ++---
 xen/arch/x86/hvm/vmx/vmx.c|  1 +
 xen/arch/x86/vm_event.c   |  9 -
 xen/common/vm_event.c |  1 -
 xen/include/asm-x86/hvm/emulate.h |  8 +---
 xen/include/asm-x86/vm_event.h|  5 -
 xen/include/public/vm_event.h | 16 +++-
 8 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index cc25676..acae998 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -76,9 +76,9 @@ static int set_context_data(void *buffer, unsigned int size)
 if ( curr->arch.vm_event )
 {
 unsigned int safe_size =
-min(size, curr->arch.vm_event->emul_read_data.size);
+min(size, curr->arch.vm_event->emul.read.size);
 
-memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+memcpy(buffer, curr->arch.vm_event->emul.read.data, safe_size);
 memset(buffer + safe_size, 0, size - safe_size);
 return X86EMUL_OKAY;
 }
@@ -827,7 +827,7 @@ static int hvmemul_read(
 struct hvm_emulate_ctxt *hvmemul_ctxt =
 container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
 
-if ( unlikely(hvmemul_ctxt->set_context) )
+if ( unlikely(hvmemul_ctxt->set_context_data) )
 return set_context_data(p_data, bytes);
 
 return __hvmemul_read(
@@ -1029,7 +1029,7 @@ static int hvmemul_cmpxchg(
 struct hvm_emulate_ctxt *hvmemul_ctxt =
 container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
 
-if ( unlikely(hvmemul_ctxt->set_context) )
+if ( unlikely(hvmemul_ctxt->set_context_data) )
 {
 int rc = set_context_data(p_new, bytes);
 
@@ -1122,7 +1122,7 @@ static int hvmemul_rep_outs(
 p2m_type_t p2mt;
 int rc;
 
-if ( unlikely(hvmemul_ctxt->set_context) )
+if ( unlikely(hvmemul_ctxt->set_context_data) )
 return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
 bytes_per_rep, reps, ctxt);
 
@@ -1264,7 +1264,7 @@ static int hvmemul_rep_movs(
 if ( buf == NULL )
 return X86EMUL_UNHANDLEABLE;
 
-if ( unlikely(hvmemul_ctxt->set_context) )
+if ( unlikely(hvmemul_ctxt->set_context_data) )
 {
 rc = set_context_data(buf, bytes);
 
@@ -1470,7 +1470,7 @@ static int hvmemul_read_io(
 
 *val = 0;
 
-if ( unlikely(hvmemul_ctxt->set_context) )
+if ( unlikely(hvmemul_ctxt->set_context_data) )
 return set_context_data(val, bytes);
 
 return hvmemul_do_pio_buffer(port, bytes, IOREQ_READ, val);
@@ -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));
+
+hvmemul_ctxt->insn_buf_bytes = sizeof(curr->arch.vm_event->emul.insn);
+memcpy(hvmemul_ctxt->insn_buf, >arch.vm_event->emul.insn,
+   hvmemul_ctxt->insn_buf_bytes);
+}
+else if ( !vio->mmio_insn_bytes )
 {
 hvmemul_ctxt->insn_buf_bytes =
 hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:
@@ -1931,7 +1941,7 @@