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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 22 May 2020 15:25
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; 
> Julien Grall
> <julien@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <george.dunlap@xxxxxxxxxx>;
> Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH v5 1/5] xen/common: introduce a new framework 
> for save/restore of
> 'domain' context
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 21.05.2020 18:19, Paul Durrant wrote:
> > To allow enlightened HVM guests (i.e. those that have PV drivers) to be
> > migrated without their co-operation it will be necessary to transfer 'PV'
> > state such as event channel state, grant entry state, etc.
> >
> > Currently there is a framework (entered via the hvm_save/load() functions)
> > that allows a domain's 'HVM' (architectural) state to be transferred but
> > 'PV' state is also common with pure PV guests and so this framework is not
> > really suitable.
> >
> > This patch adds the new public header and low level implementation of a new
> > common framework, entered via the domain_save/load() functions. Subsequent
> > patches will introduce other parts of the framework, and code that will
> > make use of it within the current version of the libxc migration stream.
> >
> > This patch also marks the HVM-only framework as deprecated in favour of the
> > new framework.
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > Acked-by: Julien Grall <julien@xxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> with one remark:
> 
> > +int domain_load_end(struct domain_context *c)
> > +{
> > +    struct domain *d = c->domain;
> > +    size_t len = c->desc.length - c->len;
> > +
> > +    while ( c->len != c->desc.length ) /* unconsumed data or pad */
> > +    {
> > +        uint8_t pad;
> > +        int rc = domain_load_data(c, &pad, sizeof(pad));
> > +
> > +        if ( rc )
> > +            return rc;
> > +
> > +        if ( pad )
> > +            return -EINVAL;
> > +    }
> > +
> > +    gdprintk(XENLOG_INFO, "%pd load: %s[%u] +%zu (-%zu)\n", d, c->name,
> > +             c->desc.instance, c->len, len);
> 
> Unlike on the save side you assume c->name to be non-NULL here.
> We're not going to crash because of this, but it feels a little
> odd anyway, specifically with the function being non-static
> (albeit on the positive side we have the type being private to
> this file).

Yes, I didn't think it was worth the test since it is supposed to be a "can't 
happen" case. If we're worried about the load handler calling in with a bad 
value of c then we could add a magic number into the struct and ASSERT it is 
correct in domain_[save|load]_[begin|data|end].

  Paul

> 
> Jan