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

[Julien Grall <julien@xxxxxxx>]

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?
  * 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.
Cheers,

--
Julien Grall