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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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.

Roger.

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