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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.09.14 at 08:24, <rcojocaru@xxxxxxxxxxxxxxx> 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:

> --- 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).

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.

Jan


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