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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 May 2020 08:40
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Paul Durrant' <pdurrant@xxxxxxxxxx>; 
> 'Andrew Cooper'
> <andrew.cooper3@xxxxxxxxxx>; 'George Dunlap' <george.dunlap@xxxxxxxxxx>; 'Ian 
> Jackson'
> <ian.jackson@xxxxxxxxxxxxx>; 'Julien Grall' <julien@xxxxxxx>; 'Stefano 
> Stabellini'
> <sstabellini@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>; 'Volodymyr Babchuk' 
> <Volodymyr_Babchuk@xxxxxxxx>;
> 'Roger Pau Monné' <roger.pau@xxxxxxxxxx>
> 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 <jbeulich@xxxxxxxx>
> >> Sent: 07 May 2020 08:22
> >>
> >> On 06.05.2020 18:44, Paul Durrant wrote:
> >>>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> 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