Re: [PATCH V7 for-4.5 4/4] xen: Handle resumed instruction based on previous mem_event reply

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/11/2014 06:02 PM, Jan Beulich wrote:
>>>> On 11.09.14 at 16:02, <rcojocaru@bitdefender.com> wrote:
>> On 09/11/2014 04:35 PM, Jan Beulich wrote:
>>>>>> On 11.09.14 at 15:15, <rcojocaru@bitdefender.com> wrote:

>>>> +            if ( violation )
>>>> +                v->arch.mem_event.emulate_flags = rsp.flags;
>>>
>>> ... a second time here (rather making this one simply a conditional
>>> expression)?
>>
>> I'll assign to v->arch.mem_event.emulate_flags directly in the switch.
> 
> I doubt that's going to result in better code.

You're right, I thought I'd simplify by getting rid of "violation" but
the code looks worse now. I'll remove the first assignment and modify
the second.

>>> And I further wonder whether all the MEM_EVENT_FLAG_* values are
>>> really potentially useful in v->arch.mem_event.emulate_flags - right
>>> now it rather looks like this field could be a simple bool_t (likely with
>>> a different name), which would at once make the
>>> hvm_mem_event_emulate_one() a little better readable.
>>
>> The value is checked here:
>>
>> +        hvm_mem_event_emulate_one((v->arch.mem_event.emulate_flags &
>> +                                   MEM_EVENT_FLAG_EMULATE_NOWRITE) != 0,
>> +                                  TRAP_invalid_op,
>> HVM_DELIVER_NO_ERROR_CODE);
>>
>> where it matters if MEM_EVENT_FLAG_EMULATE_NOWRITE is set.
> 
> Right, and this would reduce by quite a bit if you could just pass the
> boolean variable.

Sorry, I don't quite follow that. Currently,
v->arch.mem_event.emulate_flags serves both as a way to know if
emulation is required (0 if no emulation is required, and != 0 if
emulation is required), and as a way to know which kind of emulation is
required ("regular" emulation, or emulation with writes disabled).

Making it into a bool_t would make it possible to know that emulation is
required, but not which kind of emulation:

+    if ( v->arch.mem_event.emulate_flags )
+    {
+        hvm_mem_event_emulate_one((v->arch.mem_event.emulate_flags &
+                                   MEM_EVENT_FLAG_EMULATE_NOWRITE) != 0,
+                                  TRAP_invalid_op,
HVM_DELIVER_NO_ERROR_CODE);
+
+        v->arch.mem_event.emulate_flags = 0;
+        return 1;
+    }

So even in a scenario where we don't care about MEM_EVENT_FLAG_SKIP, we
still care about two different types response flags:
MEM_EVENT_FLAG_EMULATE for "regular" emulation and
MEM_EVENT_FLAG_EMULATE_NOWRITE that emulates with the dummy write
handlers (or-ed together for an "emulate no write" response).


Thanks,
Razvan Cojocaru