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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

>>> +        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?

>>> +    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.

>>> +     */
>>> +    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.

>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>
>> In new code I'd like to ask for no leading underscores on macro
>> parameters as well as no unnecessary parenthesization of macro
>> parameters (e.g. when they simply get passed on to another macro
>> or function without being part of a larger expression).
> 
> Personally I think it is generally good practice to parenthesize
> but I can drop if you prefer.

To be honest - it's more than just "prefer": Excess parentheses
hamper readability. There shouldn't be too few, and since macros
already require quite a lot of them, imo we should strive to
have exactly as many as are needed.

Jan