Re: [PATCH v2 4/5] common/domain: add a domain context record for shared_info...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2020 17:37, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xxxxxxx>
>> Sent: 20 April 2020 18:35
>> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Ian Jackson 
>> <ian.jackson@xxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>;
>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
>> <george.dunlap@xxxxxxxxxx>; Jan Beulich
>> <jbeulich@xxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Subject: Re: [PATCH v2 4/5] common/domain: add a domain context record for 
>> shared_info...
>>
>> Hi Paul,
>>
>> On 07/04/2020 18:38, Paul Durrant wrote:
>>> ... and update xen-domctx to dump some information describing the record.
>>>
>>> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
>>> ---
>>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>> Cc: Wei Liu <wl@xxxxxxx>
>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Cc: Julien Grall <julien@xxxxxxx>
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>
>>> v2:
>>>   - Drop the header change to define a 'Xen' page size and instead use a
>>>     variable length struct now that the framework makes this is feasible
>>>   - Guard use of 'has_32bit_shinfo' in common code with CONFIG_COMPAT
>>> ---
>>>   tools/misc/xen-domctx.c   | 11 ++++++
>>>   xen/common/domain.c       | 81 +++++++++++++++++++++++++++++++++++++++
>>>   xen/include/public/save.h | 10 ++++-
>>>   3 files changed, 101 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
>>> index d663522a8b..a8d3922321 100644
>>> --- a/tools/misc/xen-domctx.c
>>> +++ b/tools/misc/xen-domctx.c
>>> @@ -59,6 +59,16 @@ static void dump_header(struct domain_save_descriptor 
>>> *desc)
>>>       off += desc->length;
>>>   }
>>>
>>> +static void dump_shared_info(struct domain_save_descriptor *desc)
>>> +{
>>> +    DOMAIN_SAVE_TYPE(SHARED_INFO) s;
>>> +    READ(s);
>>> +    printf("    SHARED_INFO: field_width %u buffer size: %lu\n",
>>> +           s.field_width, desc->length - sizeof(s));
>>> +
>>> +    off += desc->length;
>>> +}
>>> +
>>>   static void dump_end(struct domain_save_descriptor *desc)
>>>   {
>>>       DOMAIN_SAVE_TYPE(END) e;
>>> @@ -125,6 +135,7 @@ int main(int argc, char **argv)
>>>           switch (desc.typecode)
>>>           {
>>>           case DOMAIN_SAVE_CODE(HEADER): dump_header(&desc); break;
>>> +        case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(&desc); break;
>>>           case DOMAIN_SAVE_CODE(END): dump_end(&desc); return 0;
>>>           default:
>>>               printf("Unknown type %u: skipping\n", desc.typecode);
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 3dcd73f67c..8b72462e07 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -33,6 +33,7 @@
>>>   #include <xen/xenoprof.h>
>>>   #include <xen/irq.h>
>>>   #include <xen/argo.h>
>>> +#include <xen/save.h>
>>>   #include <asm/debugger.h>
>>>   #include <asm/p2m.h>
>>>   #include <asm/processor.h>
>>> @@ -1646,6 +1647,86 @@ int continue_hypercall_on_cpu(
>>>       return 0;
>>>   }
>>>
>>> +static int save_shared_info(const struct vcpu *v, struct domain_context *c,
>>> +                            bool dry_run)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    struct domain_shared_info_context ctxt = {};
>>> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
>>> +    size_t size = hdr_size + PAGE_SIZE;
>>> +    int rc;
>>> +
>>> +    rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, v, size);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    if ( !dry_run )
>>
>> NIT: I think the if is not necessary here as you don't skip that much code.
>>
> 
> I know, but it is illustrative so I'd rather keep it.

While I agree with the "illustrative", I'd really see this be part
of the struct initializer. Plus its use here made me wonder ...

>>> +        ctxt.field_width =
>>> +#ifdef CONFIG_COMPAT
>>> +            has_32bit_shinfo(d) ? 4 :
>>> +#endif
>>> +            8;
>>> +
>>> +    rc = domain_save_data(c, &ctxt, hdr_size);
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    rc = domain_save_data(c, d->shared_info, PAGE_SIZE);
>>> +    if ( rc )
>>> +        return rc;

... why these don't get skipped. It took me going back through
earlier patches to find that there's effectively redundancy in
the passed arguments in that the write callback chosen varies with
whether "dry_run" is true or false.

Jan