[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 10:18, Paul Durrant wrote:
-----Original Message-----
From: Julien Grall <julien@xxxxxxx>
Sent: 06 April 2020 10:08
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 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-----
[snip]
+
+#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?
There isn't any dependency at the moment so need I say anything? If an ordering
dependency is introduced by a future patch then surely that would be time to
call it out.
If we don't spell out a dependency now, then the risk is we have to
modify the toolstack at the same time as spelling out the dependency.
I am not sure whether this matters thought. This would only affect the
save part as the restore part should be just reading records one by one.
* 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?
The commit message says:
"Domain context is state held in the hypervisor that does not come under
the category of 'HVM state' but is instead 'PV state' that is common
between PV guests and enlightened HVM guests (i.e. those that have PV
drivers) such as event channel state, grant entry state, etc."
This does not seem to cover all the cases. For instance, where would you
save IOREQ servers information?
Do you think this should also appear in a comment? Do we really care? Nothing
fundamentally prevents the mechanism being used for HVM state, but that may
introduce an ordering dependency.
I don't particularly like the idea to let the contributor decide where
"HVM context" or as part of the "Domain context".
This is going to result to unwanted dependency and possibly
bikeshedding on where they should be saved.
My preference would be to mark the existing framework as deprecated and
force all the new states to be saved as part of the new "Domain Context".
* Are we going to deprecate the existing framework?
There is no intention as yet.
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.
Ok, I can add a paragraph to try to explain a little more.
That would be appreciated. Thank you!
Cheers,
--
Julien Grall
|
