diff mbox series

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

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

Commit Message

Paul Durrant May 14, 2020, 10:44 a.m. UTC 
From: Paul Durrant <pdurrant@amazon.com>

... and update xen-domctx to dump some information describing the record.

NOTE: The domain may or may not be using the embedded vcpu_info array so
      ultimately separate context records will be added for vcpu_info when
      this becomes necessary.

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>

v3:
 - Actually dump some of the content of shared_info

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   | 73 +++++++++++++++++++++++++++++++++++++++
 xen/common/domain.c       | 60 ++++++++++++++++++++++++++++++++
 xen/include/public/save.h | 11 +++++-
 3 files changed, 143 insertions(+), 1 deletion(-)

Comments

Jan Beulich May 19, 2020, 2:07 p.m. UTC | #1 
On 14.05.2020 12:44, Paul Durrant wrote:
> @@ -61,6 +62,76 @@ static void dump_header(void)
>  
>  }
>  
> +static void print_binary(const char *prefix, void *val, size_t size,

const also for val?

> +                         const char *suffix)
> +{
> +    printf("%s", prefix);
> +
> +    while (size--)

Judging from style elsewhere you look to be missing two blanks
here.

> +    {
> +        uint8_t octet = *(uint8_t *)val++;

Following the above then also better don't cast const away here.

> +        unsigned int i;
> +
> +        for ( i = 0; i < 8; i++ )
> +        {
> +            printf("%u", octet & 1);
> +            octet >>= 1;
> +        }
> +    }
> +
> +    printf("%s", suffix);
> +}
> +
> +static void dump_shared_info(void)
> +{
> +    DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
> +    shared_info_any_t *info;
> +    unsigned int i;
> +
> +    GET_PTR(s);
> +
> +    printf("    SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> +           s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
> +
> +    info = (shared_info_any_t *)s->buffer;
> +
> +#define GET_FIELD_PTR(_f) \
> +    (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))

Better cast to const void * ?

> +#define GET_FIELD_SIZE(_f) \
> +    (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> +#define GET_FIELD(_f) \
> +    (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
> +
> +    /* Array lengths are the same for 32-bit and 64-bit shared info */

Not really, no:

    xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
    xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];

> @@ -167,12 +238,14 @@ int main(int argc, char **argv)
>          if ( (typecode < 0 || typecode == desc->typecode) &&
>               (instance < 0 || instance == desc->instance) )
>          {
> +
>              printf("[%u] type: %u instance: %u length: %u\n", entry++,
>                     desc->typecode, desc->instance, desc->length);

Stray insertion of a blank line?

> @@ -1649,6 +1650,65 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +static int save_shared_info(const struct domain *d, struct domain_context *c,
> +                            bool dry_run)
> +{
> +    struct domain_shared_info_context ctxt = { .buffer_size = PAGE_SIZE };

Why not sizeof(shared_info), utilizing the zero padding on the
receiving side?

> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> +    int rc;
> +
> +    rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> +    if ( rc )
> +        return rc;
> +
> +#ifdef CONFIG_COMPAT
> +    if ( !dry_run )
> +        ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
> +#endif

Nothing will go wrong without the if(), I suppose? Better drop it
then? It could then also easily be part of the initializer of ctxt.

> +    rc = domain_save_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_save_end(c);
> +}
> +
> +static int load_shared_info(struct domain *d, struct domain_context *c)
> +{
> +    struct domain_shared_info_context ctxt;
> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> +    unsigned int i;
> +    int rc;
> +
> +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
> +    if ( rc || i ) /* expect only a single instance */
> +        return rc;
> +
> +    rc = domain_load_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
> +         ctxt.buffer_size != PAGE_SIZE )
> +        return -EINVAL;
> +
> +#ifdef CONFIG_COMPAT
> +    d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
> +#endif

There's nothing wrong with using has_32bit_shinfo(d) here as well.

> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -73,7 +73,16 @@ 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 has_32bit_shinfo;
> +    uint8_t pad[3];

32-(or 16-)bit flags, with just a single bit used for the purpose?

Jan
Paul Durrant May 19, 2020, 3:21 p.m. UTC | #2 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 15:08
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; 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>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
> 
> On 14.05.2020 12:44, Paul Durrant wrote:
> > @@ -61,6 +62,76 @@ static void dump_header(void)
> >
> >  }
> >
> > +static void print_binary(const char *prefix, void *val, size_t size,
> 
> const also for val?

Yes, it can be.

> 
> > +                         const char *suffix)
> > +{
> > +    printf("%s", prefix);
> > +
> > +    while (size--)
> 
> Judging from style elsewhere you look to be missing two blanks
> here.
> 

Yes.

> > +    {
> > +        uint8_t octet = *(uint8_t *)val++;
> 
> Following the above then also better don't cast const away here.
> 
> > +        unsigned int i;
> > +
> > +        for ( i = 0; i < 8; i++ )
> > +        {
> > +            printf("%u", octet & 1);
> > +            octet >>= 1;
> > +        }
> > +    }
> > +
> > +    printf("%s", suffix);
> > +}
> > +
> > +static void dump_shared_info(void)
> > +{
> > +    DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
> > +    shared_info_any_t *info;
> > +    unsigned int i;
> > +
> > +    GET_PTR(s);
> > +
> > +    printf("    SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> > +           s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
> > +
> > +    info = (shared_info_any_t *)s->buffer;
> > +
> > +#define GET_FIELD_PTR(_f) \
> > +    (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))
> 
> Better cast to const void * ?
> 

Ok.

> > +#define GET_FIELD_SIZE(_f) \
> > +    (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> > +#define GET_FIELD(_f) \
> > +    (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
> > +
> > +    /* Array lengths are the same for 32-bit and 64-bit shared info */
> 
> Not really, no:
> 
>     xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
>     xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
> 

Oh, I must have misread.

> > @@ -167,12 +238,14 @@ int main(int argc, char **argv)
> >          if ( (typecode < 0 || typecode == desc->typecode) &&
> >               (instance < 0 || instance == desc->instance) )
> >          {
> > +
> >              printf("[%u] type: %u instance: %u length: %u\n", entry++,
> >                     desc->typecode, desc->instance, desc->length);
> 
> Stray insertion of a blank line?
> 

Yes.

> > @@ -1649,6 +1650,65 @@ int continue_hypercall_on_cpu(
> >      return 0;
> >  }
> >
> > +static int save_shared_info(const struct domain *d, struct domain_context *c,
> > +                            bool dry_run)
> > +{
> > +    struct domain_shared_info_context ctxt = { .buffer_size = PAGE_SIZE };
> 
> Why not sizeof(shared_info), utilizing the zero padding on the
> receiving side?
> 

Ok, yes, I guess that would work.

> > +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> > +    int rc;
> > +
> > +    rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> > +    if ( rc )
> > +        return rc;
> > +
> > +#ifdef CONFIG_COMPAT
> > +    if ( !dry_run )
> > +        ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
> > +#endif
> 
> Nothing will go wrong without the if(), I suppose? Better drop it
> then? It could then also easily be part of the initializer of ctxt.
> 

Ok. I said last time I wanted to keep it as it was illustrative but I'll drop it since it has now come up twice.

> > +    rc = domain_save_data(c, &ctxt, hdr_size);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    return domain_save_end(c);
> > +}
> > +
> > +static int load_shared_info(struct domain *d, struct domain_context *c)
> > +{
> > +    struct domain_shared_info_context ctxt;
> > +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
> > +    if ( rc || i ) /* expect only a single instance */
> > +        return rc;
> > +
> > +    rc = domain_load_data(c, &ctxt, hdr_size);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
> > +         ctxt.buffer_size != PAGE_SIZE )
> > +        return -EINVAL;
> > +
> > +#ifdef CONFIG_COMPAT
> > +    d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
> > +#endif
> 
> There's nothing wrong with using has_32bit_shinfo(d) here as well.
> 

I just thought it looked odd.

> > --- a/xen/include/public/save.h
> > +++ b/xen/include/public/save.h
> > @@ -73,7 +73,16 @@ 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 has_32bit_shinfo;
> > +    uint8_t pad[3];
> 
> 32-(or 16-)bit flags, with just a single bit used for the purpose?
> 

I debated that. Given this is xen/tools-only would a bit-field be acceptable?

  Paul

> Jan
Jan Beulich May 19, 2020, 3:34 p.m. UTC | #3 
On 19.05.2020 17:21, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 May 2020 15:08
>>
>> On 14.05.2020 12:44, Paul Durrant wrote:
>>> --- a/xen/include/public/save.h
>>> +++ b/xen/include/public/save.h
>>> @@ -73,7 +73,16 @@ 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 has_32bit_shinfo;
>>> +    uint8_t pad[3];
>>
>> 32-(or 16-)bit flags, with just a single bit used for the purpose?
>>
> 
> I debated that. Given this is xen/tools-only would a bit-field be acceptable?

Looking at domctl.h and sysctl.h, the only instance I can find is a
live-patching struct, and I'd suppose the addition of bitfields there
was missed in the hasty review back then. While it might be
acceptable, I'd recommend against it. It'll bite us the latest with
a port to an arch where endianness is not fixed, and hence may vary
between hypercall caller and callee. The standard way of using
#define-s looks more future proof.

Jan
Paul Durrant May 19, 2020, 3:35 p.m. UTC | #4 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 16:34
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; '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>; 'Julien Grall' <julien@xen.org>; 'Stefano Stabellini'
> <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
> 
> On 19.05.2020 17:21, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 15:08
> >>
> >> On 14.05.2020 12:44, Paul Durrant wrote:
> >>> --- a/xen/include/public/save.h
> >>> +++ b/xen/include/public/save.h
> >>> @@ -73,7 +73,16 @@ 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 has_32bit_shinfo;
> >>> +    uint8_t pad[3];
> >>
> >> 32-(or 16-)bit flags, with just a single bit used for the purpose?
> >>
> >
> > I debated that. Given this is xen/tools-only would a bit-field be acceptable?
> 
> Looking at domctl.h and sysctl.h, the only instance I can find is a
> live-patching struct, and I'd suppose the addition of bitfields there
> was missed in the hasty review back then. While it might be
> acceptable, I'd recommend against it. It'll bite us the latest with
> a port to an arch where endianness is not fixed, and hence may vary
> between hypercall caller and callee. The standard way of using
> #define-s looks more future proof.
> 

Ok, I'll go with a flag. Probably is better in the long run.

  Paul

> Jan
diff mbox series

Patch

diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 243325dfce..b2fed5eae7 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -31,6 +31,7 @@ 
 #include <errno.h>
 
 #include <xenctrl.h>
+#include <xen-tools/libs.h>
 #include <xen/xen.h>
 #include <xen/domctl.h>
 #include <xen/save.h>
@@ -61,6 +62,76 @@  static void dump_header(void)
 
 }
 
+static void print_binary(const char *prefix, void *val, size_t size,
+                         const char *suffix)
+{
+    printf("%s", prefix);
+
+    while (size--)
+    {
+        uint8_t octet = *(uint8_t *)val++;
+        unsigned int i;
+
+        for ( i = 0; i < 8; i++ )
+        {
+            printf("%u", octet & 1);
+            octet >>= 1;
+        }
+    }
+
+    printf("%s", suffix);
+}
+
+static void dump_shared_info(void)
+{
+    DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
+    shared_info_any_t *info;
+    unsigned int i;
+
+    GET_PTR(s);
+
+    printf("    SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
+           s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
+
+    info = (shared_info_any_t *)s->buffer;
+
+#define GET_FIELD_PTR(_f) \
+    (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))
+#define GET_FIELD_SIZE(_f) \
+    (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
+#define GET_FIELD(_f) \
+    (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
+
+    /* Array lengths are the same for 32-bit and 64-bit shared info */
+
+    for ( i = 0; i < ARRAY_SIZE(info->x64.evtchn_pending); i++ )
+    {
+        const char *prefix = !i ?
+            "                 evtchn_pending: " :
+            "                                 ";
+
+        print_binary(prefix, GET_FIELD_PTR(evtchn_pending[0]),
+                 GET_FIELD_SIZE(evtchn_pending[0]), "\n");
+    }
+
+    for ( i = 0; i < ARRAY_SIZE(info->x64.evtchn_mask); i++ )
+    {
+        const char *prefix = !i ?
+            "                    evtchn_mask: " :
+            "                                 ";
+
+        print_binary(prefix, GET_FIELD_PTR(evtchn_mask[0]),
+                 GET_FIELD_SIZE(evtchn_mask[0]), "\n");
+    }
+
+    printf("                 wc: version: %u sec: %u nsec: %u\n",
+           GET_FIELD(wc_version), GET_FIELD(wc_sec), GET_FIELD(wc_nsec));
+
+#undef GET_FIELD
+#undef GET_FIELD_SIZE
+#undef GET_FIELD_PTR
+}
+
 static void dump_end(void)
 {
     DOMAIN_SAVE_TYPE(END) *e;
@@ -167,12 +238,14 @@  int main(int argc, char **argv)
         if ( (typecode < 0 || typecode == desc->typecode) &&
              (instance < 0 || instance == desc->instance) )
         {
+
             printf("[%u] type: %u instance: %u length: %u\n", entry++,
                    desc->typecode, desc->instance, desc->length);
 
             switch (desc->typecode)
             {
             case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
+            case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
             case DOMAIN_SAVE_CODE(END): dump_end(); break;
             default:
                 printf("Unknown type %u: skipping\n", desc->typecode);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7cc9526139..e4518cd28d 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>
@@ -1649,6 +1650,65 @@  int continue_hypercall_on_cpu(
     return 0;
 }
 
+static int save_shared_info(const struct domain *d, struct domain_context *c,
+                            bool dry_run)
+{
+    struct domain_shared_info_context ctxt = { .buffer_size = PAGE_SIZE };
+    size_t hdr_size = offsetof(typeof(ctxt), buffer);
+    int rc;
+
+    rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
+    if ( rc )
+        return rc;
+
+#ifdef CONFIG_COMPAT
+    if ( !dry_run )
+        ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
+#endif
+
+    rc = domain_save_data(c, &ctxt, hdr_size);
+    if ( rc )
+        return rc;
+
+    rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
+    if ( rc )
+        return rc;
+
+    return domain_save_end(c);
+}
+
+static int load_shared_info(struct domain *d, struct domain_context *c)
+{
+    struct domain_shared_info_context ctxt;
+    size_t hdr_size = offsetof(typeof(ctxt), buffer);
+    unsigned int i;
+    int rc;
+
+    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
+    if ( rc || i ) /* expect only a single instance */
+        return rc;
+
+    rc = domain_load_data(c, &ctxt, hdr_size);
+    if ( rc )
+        return rc;
+
+    if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
+         ctxt.buffer_size != PAGE_SIZE )
+        return -EINVAL;
+
+#ifdef CONFIG_COMPAT
+    d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
+#endif
+
+    rc = domain_load_data(c, d->shared_info, ctxt.buffer_size);
+    if ( rc )
+        return rc;
+
+    return domain_load_end(c);
+}
+
+DOMAIN_REGISTER_SAVE_RESTORE(SHARED_INFO, 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 834c031c51..2b633cf03d 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -73,7 +73,16 @@  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 has_32bit_shinfo;
+    uint8_t pad[3];
+    uint32_t buffer_size;
+    uint8_t buffer[XEN_FLEX_ARRAY_DIM]; /* Implementation specific size */
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
+
+#define DOMAIN_SAVE_CODE_MAX 2
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */