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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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

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