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:22
> 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 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.

> >>> +        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
> >>> +             (c.desc.vcpu_id >= d->max_vcpus) )
> >>> +            break;
> >>> +
> >>> +        v = d->vcpu[c.desc.vcpu_id];
> >>> +
> >>> +        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
> >>> +        {
> >>> +            /* Sink the data */
> >>> +            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
> >>> +                                   v, NULL, c.desc.length, true);
> >>
> >> IOW the read handlers need to be able to deal with a NULL dst?
> >> Sounds a little fragile to me. Is there a reason
> >> domain_load_data() can't skip the call to read()?
> >
> > Something has to move the cursor so sinking the data using a
> > NULL dst seemed like the best way to avoid the need for a
> > separate callback function.
> 
> And domain_load_data() can't itself advance the cursor in such
> a case, with no callback involved at all?
> 

How could it do that without a callback? Doing such a thing negates the utility 
of the ignore flag anyway since it implies the caller is going to edit the load 
stream in advance. Since I'm going to drop the ignore flag in v3 anyway, this 
is all a bit academic.

> >>> +    uint16_t typecode;
> >>> +    /*
> >>> +     * Each entry will contain either to global or per-vcpu domain state.
> >>> +     * Entries relating to global state should have zero in this field.
> >>
> >> Is there a reason to specify zero?
> >>
> >
> > Not particularly but I thought it best to at least specify a value in all 
> > cases.
> 
> A specific value is certainly a good idea; I wonder though whether
> an obviously invalid one (like ~0) wouldn't be better then,
> allowing this ID to later be assigned meaning in such cases if
> need be.
> 

Ok, that sounds reasonable. I'll #define something for convenience.

> >>> +     */
> >>> +    uint16_t vcpu_id;
> >>
> >> Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
> >> wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
> >> per-vCPU state?
> >
> > True, a generic 'instance' would be needed for such records. I'll so as you 
> > suggest.
> 
> Which, along with my comment on domain_save_begin() taking a
> struct vcpu * right now, will then indeed require changing
> to a (struct domain *, unsigned int instance) tuple, I guess.
> 

Yes.

  Paul