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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/15/2014 01:31 PM, Jan Beulich wrote:
>>>> On 15.09.14 at 08:24, <rcojocaru@bitdefender.com> wrote:
>> In a scenario where a page fault that triggered a mem_event occured,
>> p2m_mem_access_check() will now be able to either 1) emulate the
>> current instruction, or 2) emulate it, but don't allow it to perform
>> any writes.
> 
> Purely from a technical perspective this patch (and hence now this
> series) looks fine to me, with one nit:

That's great news! Thanks for the review!

>> --- a/xen/include/public/mem_event.h
>> +++ b/xen/include/public/mem_event.h
>> @@ -31,11 +31,13 @@
>>  #include "io/ring.h"
>>  
>>  /* Memory event flags */
>> -#define MEM_EVENT_FLAG_VCPU_PAUSED  (1 << 0)
>> -#define MEM_EVENT_FLAG_DROP_PAGE    (1 << 1)
>> -#define MEM_EVENT_FLAG_EVICT_FAIL   (1 << 2)
>> -#define MEM_EVENT_FLAG_FOREIGN      (1 << 3)
>> -#define MEM_EVENT_FLAG_DUMMY        (1 << 4)
>> +#define MEM_EVENT_FLAG_VCPU_PAUSED     (1 << 0)
>> +#define MEM_EVENT_FLAG_DROP_PAGE       (1 << 1)
>> +#define MEM_EVENT_FLAG_EVICT_FAIL      (1 << 2)
>> +#define MEM_EVENT_FLAG_FOREIGN         (1 << 3)
>> +#define MEM_EVENT_FLAG_DUMMY           (1 << 4)
>> +#define MEM_EVENT_FLAG_EMULATE         (1 << 5)
>> +#define MEM_EVENT_FLAG_EMULATE_NOWRITE (1 << 6)
> 
> While I think that most pre-existing types here are sufficiently
> self-explaining, I don't think the new type is, and hence it
> warrants a comment. Of course the final say on this will be with
> Tim (being the maintainer).

Of course, I'll prepare a comment while waiting for Tim's reply.

> I also think that with the series having got reduced, there's no
> longer a process problem, but I'd nevertheless like to point out two
> things for you going forward (in the hope that this won't make you
> drop your Xen efforts): With the larger pieces of code additions
> you have pending on top of this series, we would really like to see
> at least PoC in-tree users of any such addition, perhaps even going
> as far as integrating them with osstest. This is (among other
> aspects like helping understanding the purpose) so that the code
> you add (and that's - at least initially - used only by you) won't
> become stale sooner or later.
> 
> The second aspect is that to help acceptance of the addition of
> changes that are large and/or very special purpose it would be
> beneficial if we would see previous smaller scale contributions by
> the exact same people (e.g. bug fixes, code reviews). This is in
> the spirit of, as is being said in the governance document, the
> project being run as a meritocracy, not a democracy.
> 
> And of course - as with any new functionality being added - it
> always helps if from the very beginning you make clear why
> existing functionality doesn't fit your needs.

Noted, thank you for your help!


Thanks,
Razvan Cojocaru