> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 22 May 2020 15:25
> To: Paul Durrant <p...@xen.org>
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurr...@amazon.co.uk>;
> Julien Grall
> <jul...@xen.org>; Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>;
> Ian Jackson <ian.jack...@eu.citrix.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Wei Liu
> <w...@xen.org>; Volodymyr Babchuk <volodymyr_babc...@epam.com>; Roger Pau
> Monné <roger....@citrix.com>
> 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 <pdurr...@amazon.com>
> > Acked-by: Julien Grall <jul...@xen.org>
>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
