[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Sun, Feb 03, 2019 at 10:04:29AM -0800, Christopher Clark wrote:
> 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

I think it will benefit the community to give this review in public,
so other reviewers know whats going on. IMO getting this private
review makes it harder for me (as a reviewer) to know the motivation
of some of the changes between versions, and likely also makes it
harder for you since you have to keep track of comments from multiple
sources on different channels.

Is there anything that prevents those people from making the review
comments publicly on xen-devel?

We should very much try to fix that so everyone can make review
comments on the public mailing list.

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

FWIW, I don't see the connection between nested virtualization or
measured launch and the list macros. I think a little bit more context
would be helpful here in order to understand the issue.

> > > - OpenXT can temporarily patch Xen 4.12 for downstream use

Patching the macros for OpenXT is perfectly fine, but it would be
better to understand and fix the problem upstream if possible.

How are you patching the macros?

What are you trying to achieve by patching them?

Thanks, Roger.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.