Inconsistent use of set_context_data()?

Inconsistent use of set_context_data()?

[Jan Beulich]

Hello,

what's the point of this being used by hvmemul_read() and
hvmemul_cmpxchg(), but (namely but not limited to) not by
hvmemul_write()?

Thanks, Jan


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

Re: Inconsistent use of set_context_data()?

[Razvan Cojocaru]

Hello,

> what's the point of this being used by hvmemul_read() and
> hvmemul_cmpxchg(), but (namely but not limited to) not by
> hvmemul_write()?

To do introspection work, we sometimes need to modify the guest memory,
and there are cases, namely during hibernate / resume of Windows guests,
when we need to serve the "old" version of that memory to the current
instruction reading from it for the process to work reliably.

The design choice here has been that the introspection application is
smart enough to handle writes (after all, it is the one managing the
buffer sent via vm_event reply), so it is intended behaviour.


Thanks,
Razvan

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

Re: Inconsistent use of set_context_data()?

[Jan Beulich]

>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>> what's the point of this being used by hvmemul_read() and
>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>> hvmemul_write()?
> 
> To do introspection work, we sometimes need to modify the guest memory,
> and there are cases, namely during hibernate / resume of Windows guests,
> when we need to serve the "old" version of that memory to the current
> instruction reading from it for the process to work reliably.
> 
> The design choice here has been that the introspection application is
> smart enough to handle writes (after all, it is the one managing the
> buffer sent via vm_event reply), so it is intended behaviour.

Well - the confusing thing is that for cmpxchg it's the value to be
written which gets altered, not the value to be compared against,
i.e. it acts as if set_context_data() was also intended to be
present in hvmemul_write().

Jan


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

Re: Inconsistent use of set_context_data()?

[Jan Beulich]

>>> On 05.10.16 at 14:29, <JBeulich@suse.com> wrote:
>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>> what's the point of this being used by hvmemul_read() and
>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>> hvmemul_write()?
>> 
>> To do introspection work, we sometimes need to modify the guest memory,
>> and there are cases, namely during hibernate / resume of Windows guests,
>> when we need to serve the "old" version of that memory to the current
>> instruction reading from it for the process to work reliably.
>> 
>> The design choice here has been that the introspection application is
>> smart enough to handle writes (after all, it is the one managing the
>> buffer sent via vm_event reply), so it is intended behaviour.
> 
> Well - the confusing thing is that for cmpxchg it's the value to be
> written which gets altered, not the value to be compared against,
> i.e. it acts as if set_context_data() was also intended to be
> present in hvmemul_write().

And note how this is the only case where caller supplied data gets
modified - after all at least the p_new pointer should really be const
(for p_old one might argue that upon failure the old value could be
passed back to the caller). And if that altering of caller data was
intended, then there's at least one case where it doesn't take
effect right now: protmode_load_seg()'s updating of the accessed
bit or-s in a_flag into the local copy instead of using new_desc_b.

IOW - I don't really understand what (consistent) behavior is
expected here.

Jan


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

Re: Inconsistent use of set_context_data()?

[Andrew Cooper]

On 05/10/16 13:29, Jan Beulich wrote:
>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>> what's the point of this being used by hvmemul_read() and
>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>> hvmemul_write()?
>> To do introspection work, we sometimes need to modify the guest memory,
>> and there are cases, namely during hibernate / resume of Windows guests,
>> when we need to serve the "old" version of that memory to the current
>> instruction reading from it for the process to work reliably.
>>
>> The design choice here has been that the introspection application is
>> smart enough to handle writes (after all, it is the one managing the
>> buffer sent via vm_event reply), so it is intended behaviour.
> Well - the confusing thing is that for cmpxchg it's the value to be
> written which gets altered, not the value to be compared against,
> i.e. it acts as if set_context_data() was also intended to be
> present in hvmemul_write().

Can I highly suggest that writing an Introspection feature doc,
explaining some bits and pieces like this might be a very good idea?

Also, commiseration to whichever poor sole had to debug that issue...

~Andrew

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

Re: Inconsistent use of set_context_data()?

[Razvan Cojocaru]

On 10/05/2016 03:29 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>> what's the point of this being used by hvmemul_read() and
>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>> hvmemul_write()?
>>
>> To do introspection work, we sometimes need to modify the guest memory,
>> and there are cases, namely during hibernate / resume of Windows guests,
>> when we need to serve the "old" version of that memory to the current
>> instruction reading from it for the process to work reliably.
>>
>> The design choice here has been that the introspection application is
>> smart enough to handle writes (after all, it is the one managing the
>> buffer sent via vm_event reply), so it is intended behaviour.
> 
> Well - the confusing thing is that for cmpxchg it's the value to be
> written which gets altered, not the value to be compared against,
> i.e. it acts as if set_context_data() was also intended to be
> present in hvmemul_write().

That's an accident, in hvmemul_cmpxchg() p_old is not being used at all
so IIRC I've simply latched onto p_new. All we're interested in are
reads from the supplied buffer overriding the guest's actual memory.


Thanks,
Razvan

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

Re: Inconsistent use of set_context_data()?

[Jan Beulich]

>>> On 05.10.16 at 14:44, <rcojocaru@bitdefender.com> wrote:
> On 10/05/2016 03:29 PM, Jan Beulich wrote:
>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>> what's the point of this being used by hvmemul_read() and
>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>> hvmemul_write()?
>>>
>>> To do introspection work, we sometimes need to modify the guest memory,
>>> and there are cases, namely during hibernate / resume of Windows guests,
>>> when we need to serve the "old" version of that memory to the current
>>> instruction reading from it for the process to work reliably.
>>>
>>> The design choice here has been that the introspection application is
>>> smart enough to handle writes (after all, it is the one managing the
>>> buffer sent via vm_event reply), so it is intended behaviour.
>> 
>> Well - the confusing thing is that for cmpxchg it's the value to be
>> written which gets altered, not the value to be compared against,
>> i.e. it acts as if set_context_data() was also intended to be
>> present in hvmemul_write().
> 
> That's an accident, in hvmemul_cmpxchg() p_old is not being used at all
> so IIRC I've simply latched onto p_new. All we're interested in are
> reads from the supplied buffer overriding the guest's actual memory.

So am I to understand you'll submit a patch replacing p_old with
p_new there (which would have the same effect as simply deleting
that code, except for the doc aspect)?

Jan


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

Re: Inconsistent use of set_context_data()?

[Razvan Cojocaru]

On 10/05/2016 03:47 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 14:44, <rcojocaru@bitdefender.com> wrote:
>> On 10/05/2016 03:29 PM, Jan Beulich wrote:
>>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>>> what's the point of this being used by hvmemul_read() and
>>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>>> hvmemul_write()?
>>>>
>>>> To do introspection work, we sometimes need to modify the guest memory,
>>>> and there are cases, namely during hibernate / resume of Windows guests,
>>>> when we need to serve the "old" version of that memory to the current
>>>> instruction reading from it for the process to work reliably.
>>>>
>>>> The design choice here has been that the introspection application is
>>>> smart enough to handle writes (after all, it is the one managing the
>>>> buffer sent via vm_event reply), so it is intended behaviour.
>>>
>>> Well - the confusing thing is that for cmpxchg it's the value to be
>>> written which gets altered, not the value to be compared against,
>>> i.e. it acts as if set_context_data() was also intended to be
>>> present in hvmemul_write().
>>
>> That's an accident, in hvmemul_cmpxchg() p_old is not being used at all
>> so IIRC I've simply latched onto p_new. All we're interested in are
>> reads from the supplied buffer overriding the guest's actual memory.
> 
> So am I to understand you'll submit a patch replacing p_old with
> p_new there (which would have the same effect as simply deleting
> that code, except for the doc aspect)?

Yes, I'll get on that. I'll also const-ify the p_new pointer unless
there are objections.


Thanks,
Razvan

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

Re: Inconsistent use of set_context_data()?

[Jan Beulich]

>>> On 05.10.16 at 14:53, <rcojocaru@bitdefender.com> wrote:
> On 10/05/2016 03:47 PM, Jan Beulich wrote:
>>>>> On 05.10.16 at 14:44, <rcojocaru@bitdefender.com> wrote:
>>> On 10/05/2016 03:29 PM, Jan Beulich wrote:
>>>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>>>> what's the point of this being used by hvmemul_read() and
>>>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>>>> hvmemul_write()?
>>>>>
>>>>> To do introspection work, we sometimes need to modify the guest memory,
>>>>> and there are cases, namely during hibernate / resume of Windows guests,
>>>>> when we need to serve the "old" version of that memory to the current
>>>>> instruction reading from it for the process to work reliably.
>>>>>
>>>>> The design choice here has been that the introspection application is
>>>>> smart enough to handle writes (after all, it is the one managing the
>>>>> buffer sent via vm_event reply), so it is intended behaviour.
>>>>
>>>> Well - the confusing thing is that for cmpxchg it's the value to be
>>>> written which gets altered, not the value to be compared against,
>>>> i.e. it acts as if set_context_data() was also intended to be
>>>> present in hvmemul_write().
>>>
>>> That's an accident, in hvmemul_cmpxchg() p_old is not being used at all
>>> so IIRC I've simply latched onto p_new. All we're interested in are
>>> reads from the supplied buffer overriding the guest's actual memory.
>> 
>> So am I to understand you'll submit a patch replacing p_old with
>> p_new there (which would have the same effect as simply deleting
>> that code, except for the doc aspect)?
> 
> Yes, I'll get on that. I'll also const-ify the p_new pointer unless
> there are objections.

You won't be able to - that's where I started actually.

Jan


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

Re: Inconsistent use of set_context_data()?

[Razvan Cojocaru]

On 10/05/2016 04:02 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 14:53, <rcojocaru@bitdefender.com> wrote:
>> On 10/05/2016 03:47 PM, Jan Beulich wrote:
>>>>>> On 05.10.16 at 14:44, <rcojocaru@bitdefender.com> wrote:
>>>> On 10/05/2016 03:29 PM, Jan Beulich wrote:
>>>>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>>>>> what's the point of this being used by hvmemul_read() and
>>>>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>>>>> hvmemul_write()?
>>>>>>
>>>>>> To do introspection work, we sometimes need to modify the guest memory,
>>>>>> and there are cases, namely during hibernate / resume of Windows guests,
>>>>>> when we need to serve the "old" version of that memory to the current
>>>>>> instruction reading from it for the process to work reliably.
>>>>>>
>>>>>> The design choice here has been that the introspection application is
>>>>>> smart enough to handle writes (after all, it is the one managing the
>>>>>> buffer sent via vm_event reply), so it is intended behaviour.
>>>>>
>>>>> Well - the confusing thing is that for cmpxchg it's the value to be
>>>>> written which gets altered, not the value to be compared against,
>>>>> i.e. it acts as if set_context_data() was also intended to be
>>>>> present in hvmemul_write().
>>>>
>>>> That's an accident, in hvmemul_cmpxchg() p_old is not being used at all
>>>> so IIRC I've simply latched onto p_new. All we're interested in are
>>>> reads from the supplied buffer overriding the guest's actual memory.
>>>
>>> So am I to understand you'll submit a patch replacing p_old with
>>> p_new there (which would have the same effect as simply deleting
>>> that code, except for the doc aspect)?
>>
>> Yes, I'll get on that. I'll also const-ify the p_new pointer unless
>> there are objections.
> 
> You won't be able to - that's where I started actually.

You're right - just found out the hard way. I'll just submit the smaller
patch then in a moment.


Thanks,
Razvan

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

Re: Inconsistent use of set_context_data()?

[Razvan Cojocaru]

On 10/05/2016 03:40 PM, Andrew Cooper wrote:
> On 05/10/16 13:29, Jan Beulich wrote:
>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>> what's the point of this being used by hvmemul_read() and
>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>> hvmemul_write()?
>>> To do introspection work, we sometimes need to modify the guest memory,
>>> and there are cases, namely during hibernate / resume of Windows guests,
>>> when we need to serve the "old" version of that memory to the current
>>> instruction reading from it for the process to work reliably.
>>>
>>> The design choice here has been that the introspection application is
>>> smart enough to handle writes (after all, it is the one managing the
>>> buffer sent via vm_event reply), so it is intended behaviour.
>> Well - the confusing thing is that for cmpxchg it's the value to be
>> written which gets altered, not the value to be compared against,
>> i.e. it acts as if set_context_data() was also intended to be
>> present in hvmemul_write().
> 
> Can I highly suggest that writing an Introspection feature doc,
> explaining some bits and pieces like this might be a very good idea?

Fair point, I suppose what we'd need to figure out at this point is A)
where would the best place for this information to appear be (I assume
somewhere on the project Wiki), and B) what information is interesting,
or at least complex, enough to warrant higher level descriptions?

Tamas probably has additional thoughts on this.


Thanks,
Razvan

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

Re: Inconsistent use of set_context_data()?

[Andrew Cooper]

On 05/10/16 14:12, Razvan Cojocaru wrote:
> On 10/05/2016 03:40 PM, Andrew Cooper wrote:
>> On 05/10/16 13:29, Jan Beulich wrote:
>>>>>> On 05.10.16 at 14:22, <rcojocaru@bitdefender.com> wrote:
>>>>> what's the point of this being used by hvmemul_read() and
>>>>> hvmemul_cmpxchg(), but (namely but not limited to) not by
>>>>> hvmemul_write()?
>>>> To do introspection work, we sometimes need to modify the guest memory,
>>>> and there are cases, namely during hibernate / resume of Windows guests,
>>>> when we need to serve the "old" version of that memory to the current
>>>> instruction reading from it for the process to work reliably.
>>>>
>>>> The design choice here has been that the introspection application is
>>>> smart enough to handle writes (after all, it is the one managing the
>>>> buffer sent via vm_event reply), so it is intended behaviour.
>>> Well - the confusing thing is that for cmpxchg it's the value to be
>>> written which gets altered, not the value to be compared against,
>>> i.e. it acts as if set_context_data() was also intended to be
>>> present in hvmemul_write().
>> Can I highly suggest that writing an Introspection feature doc,
>> explaining some bits and pieces like this might be a very good idea?
> Fair point, I suppose what we'd need to figure out at this point is A)
> where would the best place for this information to appear be (I assume
> somewhere on the project Wiki)

docs/features/  in tree please.

See the migration and levelling docs as examples, and the template for
suggested layout.

> , and B) what information is interesting,
> or at least complex, enough to warrant higher level descriptions?

In this case, I would suggested suggest that the Technical Details
section include some details as to what an introspection agent needs to
be able to do, using the interface, (and preferably why).

Something like "override the values from memory on an emulated read"
would certainly be helpful when referring to the emulation code.

~Andrew

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