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

2019-02-13 Thread Stefano Stabellini
On Wed, 13 Feb 2019, Rich Persaud wrote:
> On Feb 4, 2019, at 13:22, Stefano Stabellini  wrote:
> 
>   On Mon, 4 Feb 2019, Roger Pau Monné wrote:
>   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.
> 
> 
>   There is one more reason to require public comments which I have only
>   learned recently: for safety certifications we need to keep a record of
>   all review comments and patches that address them for traceability.
> 
> 
> Do you mean:
> 
> (A) all _merged_ patches and their review comments
> 
>  or
> 
> (B) all comments and patches (merged or not) that address them
> 
> i.e. would the certification process be seeking traceability of 
> safety-impacting patches (code, scenario A) or decisions
> (including decisions to leave code unchanged, scenario B)?

I meant (A), however, I don't know specifically if anything from (B) is
also required.


> If you mean (B), would we need an update to the Xen Security Problem Response 
> Process [1]?  e.g. public archive of all comments
> from pre-disclosure discussion, along with content hashes stored immutably?  
> Rich
> 
> [1] https://www.xenproject.org/security-policy.html

I don't think the archives of the pre-disclosure security discussions
need to be public, but they probably need to be available.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2019-02-13 Thread Rich Persaud

> On Feb 4, 2019, at 05:07, Roger Pau Monné  wrote:
> 
>> 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é  
>> 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é  
>> 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.

I've advocated for open-source principles in several large organizations.  At 
XenSource and Citrix, we created organizational separation between the OSS Xen 
dev team and product teams.  I don't know if that structure remains today, but 
it was once helpful in reducing conflict between public OSS and private product 
roadmaps.

The separation between server and client Xen product teams was less ideal, 
which eventually lead to OpenXT.  Six years after v4v was posted to xen-devel, 
Xen Argo is the first step to possible reunification, a small chance at 
reversal, via public open-source, of architectural and resource fragmentation 
that took place privately.

Like QubesOS, OpenXT (and predecessor Citrix XenClient) development is spread 
across many open-source projects, including Xen, enabling user workflows that 
balance hardware-assisted security with usability.  Spanning ecosystems, OpenXT 
is:

- unbundling OSS capabilities, e.g. TrenchBoot and coreboot for launch integrity
- moving code upstream (Argo, stubdom, blktap, Qemu, OpenEmbedded meta-virt)
- refactoring for peer & downstream derivatives, on client devices and beyond

To achieve this cross-community integration, we work with many stakeholders 

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

2019-02-13 Thread Rich Persaud
> On Feb 4, 2019, at 13:22, Stefano Stabellini  wrote:
> 
> On Mon, 4 Feb 2019, Roger Pau Monné wrote:
>>> 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.
> 
> There is one more reason to require public comments which I have only
> learned recently: for safety certifications we need to keep a record of
> all review comments and patches that address them for traceability.

Do you mean:

(A) all _merged_ patches and their review comments

 or

(B) all comments and patches (merged or not) that address them

i.e. would the certification process be seeking traceability of 
safety-impacting patches (code, scenario A) or decisions (including decisions 
to leave code unchanged, scenario B)?

If you mean (B), would we need an update to the Xen Security Problem Response 
Process [1]?  e.g. public archive of all comments from pre-disclosure 
discussion, along with content hashes stored immutably?  

Rich

[1] https://www.xenproject.org/security-policy.html


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

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

2019-02-06 Thread Roger Pau Monné
Wrong 'To:' field in the previous email, sorry.

Gentle ping on the questions below.

On Mon, Feb 04, 2019 at 11:07:12AM +0100, Roger Pau Monné wrote:
> 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é  
> > 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é  
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 21, 2019 at 01:59:40AM -0800, Christopher Clark wrote:
> > > > 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@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

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

2019-02-06 Thread Roger Pau Monné
Gentle ping on the questions below.

On Mon, Feb 04, 2019 at 11:07:12AM +0100, Roger Pau Monné wrote:
> 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é  
> > 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é  
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 21, 2019 at 01:59:40AM -0800, Christopher Clark wrote:
> > > > 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@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

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

2019-02-04 Thread Stefano Stabellini
On Mon, 4 Feb 2019, Roger Pau Monné wrote:
> > 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.

There is one more reason to require public comments which I have only
learned recently: for safety certifications we need to keep a record of
all review comments and patches that address them for traceability.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2019-02-04 Thread Roger Pau Monné
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é  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é  
> > > 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 tempo

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

2019-02-03 Thread Christopher Clark
On Thu, Jan 31, 2019 at 5:39 AM Roger Pau Monné  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é  
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2019-01-31 Thread Roger Pau Monné
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é  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2019-01-30 Thread Christopher Clark
On Tue, Jan 22, 2019 at 6:19 AM Roger Pau Monné  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.

Christopher

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

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

2019-01-22 Thread Roger Pau Monné
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, 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?

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.

Roger.

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

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

2019-01-21 Thread Christopher Clark
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.

* Further removal of uses of fixed-width types.

* Added a new patch to add Argo to the MAINTAINERS file.

Christopher Clark (15):
  argo: Introduce the Kconfig option to govern inclusion of Argo
  argo: introduce the argo_op hypercall boilerplate
  argo: define argo_dprintk for subsystem debugging
  argo: init, destroy and soft-reset, with enable command line opt
  errno: add POSIX error codes EMSGSIZE, ECONNREFUSED to the ABI
  xen/arm: introduce guest_handle_for_field()
  argo: implement the register op
  argo: implement the unregister op
  argo: implement the sendv op; evtchn: expose send_guest_global_virq
  argo: implement the notify op
  xsm, argo: XSM control for argo register
  xsm, argo: XSM control for argo message send operation
  xsm, argo: XSM control for any access to argo by a domain
  xsm, argo: notify: don't describe rings that cannot be sent to
  MAINTAINERS: add new section for Argo and self as maintainer

 MAINTAINERS  |8 +
 docs/misc/xen-command-line.pandoc|   22 +
 tools/flask/policy/modules/guest_features.te |7 +
 xen/arch/x86/guest/hypercall_page.S  |2 +-
 xen/arch/x86/hvm/hypercall.c |3 +
 xen/arch/x86/hypercall.c |3 +
 xen/arch/x86/pv/hypercall.c  |3 +
 xen/common/Kconfig   |   19 +
 xen/common/Makefile  |3 +-
 xen/common/argo.c| 2281 ++
 xen/common/compat/argo.c |   62 +
 xen/common/domain.c  |9 +
 xen/common/event_channel.c   |2 +-
 xen/include/Makefile |1 +
 xen/include/asm-arm/guest_access.h   |3 +
 xen/include/public/argo.h|  280 
 xen/include/public/errno.h   |2 +
 xen/include/public/xen.h |4 +-
 xen/include/xen/argo.h   |   44 +
 xen/include/xen/event.h  |7 +
 xen/include/xen/hypercall.h  |9 +
 xen/include/xen/sched.h  |5 +
 xen/include/xlat.lst |8 +
 xen/include/xsm/dummy.h  |   25 +
 xen/include/xsm/xsm.h|   31 +
 xen/xsm/dummy.c  |6 +
 xen/xsm/flask/hooks.c|   41 +-
 xen/xsm/flask/policy/access_vectors  |   16 +
 xen/xsm/flask/policy/security_classes|1 +
 29 files changed, 2899 insertions(+), 8 deletions(-)
 create mode 100644 xen/common/argo.c
 create mode 100644 xen/common/compat/argo.c
 create mode 100644 xen/include/public/argo.h
 create mode 100644 xen/include/xen/argo.h

-- 
2.7.4


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