Re: [Xen-devel] [PATCH v5 00/15] Argo: hypervisor-mediated interdomain communication

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Thu, Jan 31, 2019 at 5:39 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Wed, Jan 30, 2019 at 08:05:30PM -0800, Christopher Clark wrote:
> > On Tue, Jan 22, 2019 at 6:19 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> 
> > wrote:
> > >
> > > On Mon, Jan 21, 2019 at 01:59:40AM -0800, Christopher Clark wrote:
> > > > Version five of this patch series:
> > > >
> > > > * Changes are primarily addressing feedback from the v4 series reviews.
> > > >   Many points noted on the invididual commit posts.
> > > >
> > > > * Critical sections have been shrunk, with allocations and frees
> > > >   pulled outside where possible, reordering logic within hypercall ops.
> > > >
> > > > * A new ring hash function implemented, derived from the djb2 string
> > > >   hash function.
> > > >
> > > > * Flags returned by the notify op have been simplified.
> > > >
> > > > * Now uses a single argo boot parameter, taking a list:
> > > >   - top level boolean to enable/disable Argo
> > > >   - mac-permissive option to enable/disable wildcard rings
> > > >   - command line doc edit: no "CONFIG_ARGO" but refers to build config
> > > >
> > > > * Switched to use the standard list data structures used by Xen's
> > > >   common code.
> > >
> > > AFAIK this was not requested by any reviewer, so I wonder why you made
> > > such change. The more that you open coded some of the list_ macros
> > > instead of just doing a s/hlist_/list_/ replacement.
> > > I'm fine with using list instead of hlist,
> >
> > At your request, v7 replaces open coding with Xen's list macros. The
> > hlist macros were not used by any of the common code in Xen.
> >
> > > but I don't understand why
> > > you decided to open code list_for_each and list_for_each_safe instead
> > > of using the macros provided by Xen. Is there an issue with such
> > > macros?
> >
> > As discussed offline:
> >
> > - Using Xen's list macros will expedite Argo's merge for Xen 4.12
> > - List macros in Xen list.h originated in Linux list.h and have diverged
> > - OpenXT has use cases for measured launch and nested virtualization,
> >   which influence downstream performance and security requirements for
> >   Argo and Xen
> > - OpenXT can temporarily patch Xen 4.12 for downstream use
> >
> > > I've made a couple of minor comments, but I think the current status
> > > is good, and fixing those minor comments is going to be trivial.
> >
> > Ack, thanks. Hopefully v7 looks good.
>
> As a note, the common flow of interactions usually involves the
> contributor replying to the comments made by the reviewer in order to
> try to reach an agreement before sending a new version.

Yes, v7 was sent to address Jan and Julien's review comments in parallel
with our ongoing discussion on v5 macros. v7 also provided a checkpoint
for Argo testers to maximize test coverage as the series converges into
a Xen 4.12 merge candidate for Juergen. It addressed:

 - Jan's v6 review comments
 - Julien's v1 review comment
 - most of your xen-devel and offline review comments

> There are comments from v5 that haven't been fixed in v7
> (the mask usage and list_first_entry_or_null for example)
> and the reply to the reviewer's comment was sent at the same time as
> v7, leaving no time for further discussion (and for reaching an
> agreement suitable to both parties) before sending v7.

Code changes from our ongoing discussion will be addressed in v8. A
proposal to address mask usage has been put forward in the parallel
thread. Your proposed usage of list_first_entry_or_null will be made in
v8, subject to the previous offline discussion about list macros
(duplicated here for convenience):

> > As discussed offline:
> >
> > - Using Xen's list macros will expedite Argo's merge for Xen 4.12
> > - List macros in Xen list.h originated in Linux list.h and have diverged
> > - OpenXT has use cases for measured launch and nested virtualization,
> >   which influence downstream performance and security requirements for
> >   Argo and Xen
> > - OpenXT can temporarily patch Xen 4.12 for downstream use

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel