> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 07 May 2020 08:40
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurr...@amazon.com>;
> 'Andrew Cooper'
> <andrew.coop...@citrix.com>; 'George Dunlap' <george.dun...@citrix.com>; 'Ian
> Jackson'
> <ian.jack...@eu.citrix.com>; 'Julien Grall' <jul...@xen.org>; 'Stefano
> Stabellini'
> <sstabell...@kernel.org>; 'Wei Liu' <w...@xen.org>; 'Volodymyr Babchuk'
> <volodymyr_babc...@epam.com>;
> 'Roger Pau Monné' <roger....@citrix.com>
> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for
> save/restore of 'domain' context
>
> On 07.05.2020 09:34, Paul Durrant wrote:
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: 07 May 2020 08:22
> >>
> >> On 06.05.2020 18:44, Paul Durrant wrote:
> >>>> From: Jan Beulich <jbeul...@suse.com>
> >>>> Sent: 29 April 2020 12:02
> >>>>
> >>>> On 07.04.2020 19:38, Paul Durrant wrote:
> >>>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
> >>>>> + const char *name, const struct vcpu *v, size_t
> >>>>> len,
> >>>>> + bool exact)
> >>>>> +{
> >>>>> + if ( c->log )
> >>>>> + gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
> >>>>> + (unsigned long)len);
> >>>>> +
> >>>>> + BUG_ON(tc != c->desc.typecode);
> >>>>> + BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> >>>>> +
> >>>>> + if ( (exact && (len != c->desc.length)) ||
> >>>>> + (len < c->desc.length) )
> >>>>> + return -EINVAL;
> >>>>
> >>>> How about
> >>>>
> >>>> if ( exact ? len != c->desc.length
> >>>> : len < c->desc.length )
> >>>>
> >>>
> >>> Yes, that doesn't look too bad.
> >>>
> >>>> ? I'm also unsure about the < - don't you mean > instead? Too
> >>>> little data would be compensated by zero padding, but too
> >>>> much data can't be dealt with. But maybe I'm getting the sense
> >>>> of len wrong ...
> >>>
> >>> I think the < is correct. The caller needs to have at least enough
> >>> space to accommodate the context record.
> >>
> >> But this is load, not save - the caller supplies the data. If
> >> there's less data than can be fit, it'll be zero-extended. If
> >> there's too much data, the excess you don't know what to do
> >> with (it might be okay to tolerate it being all zero).
> >>
> >
> > But this is a callback. The outer load function iterates over
> > the records calling the appropriate hander for each one. Those
> > handlers then call this function saying how much data they
> > expect and whether they want exactly that amount, or whether
> > they can tolerate less (i.e. zero-extend). Hence
> > len < c->desc.length is an error, because it means the
> > descriptor contains more data than the hander knows how to
> > handle.
>
> Oh, I see - "But maybe I'm getting the sense of len wrong ..."
> then indeed applies.
>
> Any thoughts on tolerating the excess data being zero?
>
Well the point of the check here is to not tolerate excess data... Are you
suggesting that it might be a reasonable idea? If so, then yes, insisting it is
all zero would be an alternative.
Paul
