Xen project Mailing List

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

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 amid competing priorities for limited dev resources. It has taken six years to turn the ship from a Xen separation which began within one organization. This progress was accrued across multiple organizations and policies.

Argo was improved by the Xen upstreaming effort. Future Xen code contributions will benefit from the lessons learned.

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.

The issue most relevant to xen-devel is the divergence of list macros in Xen list.h from their origin in Linux list.h. Since this issue is independent of Argo, I've started a separate thread on "macro supply chains" [1].

I think a little bit more context

would be helpful here in order to understand the issue.

For more context on nested virtualization, see Ian Pratt's talk [2] on AX, uXen and nested Hyper-V. If a production system is designed to meet performance and security requirements that are delivered by multiple hypervisors, which could be open-source (e.g. Xen or uXen), proprietary (e.g. Hyper-V) or in firmware (e.g. AX on HP laptops), then a measured launch increases the level of assurance that the system is booted with validated hypervisors that can cooperate to meet those requirements.

For more context on measured launch, see the boot integrity talks from PSEC 2018 [3].

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

This will be determined during an upcoming OpenXT development, testing and certification cycle, when upstream Xen Argo is evaluated in the context of OpenXT and derivative use cases.

Rich

[1] Macro supply chains

https://lists.xen.org/archives/html/xen-devel/2019-02/msg00832.html

[2] "Hypervisor Security — Lessons Learned", Ian Pratt, 2018
https://www.platformsecuritysummit.com/2018/speaker/pratt/

[3] Boot Integrity presentations, 2018
https://www.platformsecuritysummit.com/2018/topic/boot/

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