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

Re: [PATCH 1/5] xen/common: introduce a new framework for save/restore of 'domain' context

Hi Paul,

On 06/04/2020 09:27, Paul Durrant wrote:
-----Original Message-----
From: Julien Grall <julien@xxxxxxx>
Sent: 03 April 2020 18:24
To: paul@xxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>; 'George Dunlap' 
<george.dunlap@xxxxxxxxxx>; 'Ian
Jackson' <ian.jackson@xxxxxxxxxxxxx>; 'Jan Beulich' <jbeulich@xxxxxxxx>; 
'Stefano Stabellini'
<sstabellini@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>
Subject: Re: [PATCH 1/5] xen/common: introduce a new framework for save/restore 
of 'domain' context

Hi Paul,

On 03/04/2020 16:55, Paul Durrant wrote:
-----Original Message-----
+#include <xen/save.h>
+struct domain_context {
+    bool log;
+    struct domain_save_descriptor desc;
+    domain_copy_entry copy;

As your new framework is technically an extension of existing one, it
would be good to explain why we diverge in the definitions.

I don't follow. What is diverging? I explain in the commit comment that this is 
a parallel
framework. Do I need to justify why it is not a carbon copy of the HVM one?

Well, they are both restoring/saving guest state. The only difference is
the existing one is focusing on HVM state.

So it would make sense long term to have only one hypercall and tell
what you want to save. In fact, some of the improvement here would
definitely make the HVM one nicer to use (at least in the context of LU).

I guess we could move the HVM save records over to the new framework, but it 
works for the moment so I don't want to bring it into scope now.

And I agree, hence why I say "long term" :).

  From the commit message, it is not clear to me why a new framework and
why the infrastructure is at the same time different but not.

An alternative would be to move the HVM save code into common code and then try 
to adapt it. I think that would result in more code churn and ultimately be 
harder to review. The extra infrastructure introduced here is fairly minimal 
and, for the moment, only targeting PV state. As I said above there's nothing 
stopping the HVM records being ported over later once any initial issues have 
been shaken out.

Code churn is always going to necessary one day or another if we want to consolidate the two.

Having two frameworks is not without risks. There are a few unknown to be answered:
  * Is there any dependency between the two? If yes, what is the ordering?
* The name of the hypercall does not say anything about "PV". So a contributor could think it would be fine to save/restore new HVM state in the new generic hypercall. Is it going to be an issue? If so, how do we prevent it?
  * Are we going to deprecate the existing framework?

I am not suggesting we should not go with two frameworks, but the reasons and implications are not clear to me. Hence, why I think the commit message should be expanded with some rationale.


Julien Grall



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