diff mbox series

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

Message ID 20200407173847.1595-5-paul@xen.org (mailing list archive)
State Superseded
Headers show
Series domain context infrastructure | expand

Commit Message

Paul Durrant April 7, 2020, 5:38 p.m. UTC
... and update xen-domctx to dump some information describing the record.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>

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

Comments

Julien Grall April 20, 2020, 5:34 p.m. UTC | #1
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@amazon.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> 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.

> +        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;
> +
> +    return domain_save_end(c);
> +}
> +
> +static int load_shared_info(struct vcpu *v, struct domain_context *c)
> +{
> +    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;
> +    unsigned int i;
> +    int rc;
> +
> +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, v, size, true);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_load_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
> +        if ( ctxt.pad[i] )
> +            return -EINVAL;
> +
> +    switch ( ctxt.field_width )
> +    {
> +#ifdef CONFIG_COMPAT
> +    case 4:
> +        d->arch.has_32bit_shinfo = 1;
> +        break;
> +#endif
> +    case 8:
> +#ifdef CONFIG_COMPAT
> +        d->arch.has_32bit_shinfo = 0;
> +#endif
> +        break;
> +
> +    default:
> +        rc = -EINVAL;
> +        break;
> +    }
> +
> +    rc = domain_load_data(c, d->shared_info, PAGE_SIZE);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_load_end(c);
> +}
> +
> +DOMAIN_REGISTER_SAVE_RESTORE(SHARED_INFO, false, save_shared_info,
> +                             load_shared_info);
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> index 7e5f8752bd..ed994a8765 100644
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -79,6 +79,14 @@ struct domain_save_header {
>   };
>   DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
>   
> -#define DOMAIN_SAVE_CODE_MAX 1
> +struct domain_shared_info_context {
> +    uint8_t field_width;
> +    uint8_t pad[7];
> +    uint8_t buffer[]; /* Implementation specific size */

I would recommend to use buffer[XEN_FLEX_ARRAY_DIM].

Cheers,
Paul Durrant April 28, 2020, 3:37 p.m. UTC | #2
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 20 April 2020 18:35
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>;
> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>
> 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@amazon.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <george.dunlap@citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> >
> > 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.

> > +        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;
> > +
> > +    return domain_save_end(c);
> > +}
> > +
> > +static int load_shared_info(struct vcpu *v, struct domain_context *c)
> > +{
> > +    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;
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, v, size, true);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = domain_load_data(c, &ctxt, hdr_size);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
> > +        if ( ctxt.pad[i] )
> > +            return -EINVAL;
> > +
> > +    switch ( ctxt.field_width )
> > +    {
> > +#ifdef CONFIG_COMPAT
> > +    case 4:
> > +        d->arch.has_32bit_shinfo = 1;
> > +        break;
> > +#endif
> > +    case 8:
> > +#ifdef CONFIG_COMPAT
> > +        d->arch.has_32bit_shinfo = 0;
> > +#endif
> > +        break;
> > +
> > +    default:
> > +        rc = -EINVAL;
> > +        break;
> > +    }
> > +
> > +    rc = domain_load_data(c, d->shared_info, PAGE_SIZE);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    return domain_load_end(c);
> > +}
> > +
> > +DOMAIN_REGISTER_SAVE_RESTORE(SHARED_INFO, false, save_shared_info,
> > +                             load_shared_info);
> > +
> >   /*
> >    * Local variables:
> >    * mode: C
> > diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> > index 7e5f8752bd..ed994a8765 100644
> > --- a/xen/include/public/save.h
> > +++ b/xen/include/public/save.h
> > @@ -79,6 +79,14 @@ struct domain_save_header {
> >   };
> >   DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
> >
> > -#define DOMAIN_SAVE_CODE_MAX 1
> > +struct domain_shared_info_context {
> > +    uint8_t field_width;
> > +    uint8_t pad[7];
> > +    uint8_t buffer[]; /* Implementation specific size */
> 
> I would recommend to use buffer[XEN_FLEX_ARRAY_DIM].
> 

Ok.

  Paul

> Cheers,
> 
> --
> Julien Grall
Jan Beulich April 30, 2020, 11:29 a.m. UTC | #3
On 28.04.2020 17:37, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 20 April 2020 18:35
>> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
>> Cc: Paul Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>;
>> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Jan Beulich
>> <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>
>> 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@amazon.com>
>>> ---
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Wei Liu <wl@xen.org>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: George Dunlap <george.dunlap@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Julien Grall <julien@xen.org>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> 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
Jan Beulich April 30, 2020, 11:56 a.m. UTC | #4
On 07.04.2020 19:38, Paul Durrant wrote:
> --- 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;
> +}

And nothing about the actual contents of the shared info struct?

> @@ -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;

const?

> +    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 )
> +        ctxt.field_width =
> +#ifdef CONFIG_COMPAT
> +            has_32bit_shinfo(d) ? 4 :
> +#endif
> +            8;

What are 4 and 8 to represent here? Taking Arm32 into account, the
only things I could think of are sizeof(xen_ulong_t) or
sizeof(guest_handle). I'd prefer if literal numbers could be avoided
here, such that it would become clear what property is really meant.

> +    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;
> +
> +    return domain_save_end(c);
> +}
> +
> +static int load_shared_info(struct vcpu *v, struct domain_context *c)
> +{
> +    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;
> +    unsigned int i;
> +    int rc;
> +
> +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, v, size, true);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_load_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
> +        if ( ctxt.pad[i] )
> +            return -EINVAL;
> +
> +    switch ( ctxt.field_width )
> +    {
> +#ifdef CONFIG_COMPAT
> +    case 4:
> +        d->arch.has_32bit_shinfo = 1;

true and ...

> +        break;
> +#endif
> +    case 8:
> +#ifdef CONFIG_COMPAT
> +        d->arch.has_32bit_shinfo = 0;

... false respectively, please.

> +#endif
> +        break;
> +
> +    default:
> +        rc = -EINVAL;
> +        break;
> +    }
> +
> +    rc = domain_load_data(c, d->shared_info, PAGE_SIZE);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_load_end(c);
> +}

While you check the padding fields of the header above, what about
currently unused fields of the shared_info struct itself?

There's a connection between shared_info and vcpu_info - this way
you may or may not restore vcpu_info - depending on guest behavior.
There not being a patch in the series to deal with vcpu_info, the
description here should imo at least outline the intended
interaction (including e.g. ordering between the [supposed] records).

Jan
diff mbox series

Patch

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 )
+        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;
+
+    return domain_save_end(c);
+}
+
+static int load_shared_info(struct vcpu *v, struct domain_context *c)
+{
+    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;
+    unsigned int i;
+    int rc;
+
+    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, v, size, true);
+    if ( rc )
+        return rc;
+
+    rc = domain_load_data(c, &ctxt, hdr_size);
+    if ( rc )
+        return rc;
+
+    for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
+        if ( ctxt.pad[i] )
+            return -EINVAL;
+
+    switch ( ctxt.field_width )
+    {
+#ifdef CONFIG_COMPAT
+    case 4:
+        d->arch.has_32bit_shinfo = 1;
+        break;
+#endif
+    case 8:
+#ifdef CONFIG_COMPAT
+        d->arch.has_32bit_shinfo = 0;
+#endif
+        break;
+
+    default:
+        rc = -EINVAL;
+        break;
+    }
+
+    rc = domain_load_data(c, d->shared_info, PAGE_SIZE);
+    if ( rc )
+        return rc;
+
+    return domain_load_end(c);
+}
+
+DOMAIN_REGISTER_SAVE_RESTORE(SHARED_INFO, false, save_shared_info,
+                             load_shared_info);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index 7e5f8752bd..ed994a8765 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -79,6 +79,14 @@  struct domain_save_header {
 };
 DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
 
-#define DOMAIN_SAVE_CODE_MAX 1
+struct domain_shared_info_context {
+    uint8_t field_width;
+    uint8_t pad[7];
+    uint8_t buffer[]; /* Implementation specific size */
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
+
+#define DOMAIN_SAVE_CODE_MAX 2
 
 #endif /* __XEN_PUBLIC_SAVE_H__ */