[v7,8/9] x86/time: add a domain context record for tsc_info...

Commit Message

Paul Durrant Aug. 18, 2020, 10:30 a.m. UTC

From: Paul Durrant <[e-mail]>

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

NOTE: Whilst the record definition is x86 specific, it is visible directly
      in the common header as context record numbers should be unique across
      all architectures.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>

v7:
 - New in v7
---
 tools/misc/xen-domctx.c    | 12 ++++++++++++
 xen/arch/x86/time.c        | 34 +++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/time.h |  5 +++--
 xen/include/public/save.h  | 13 ++++++++++++-
 4 files changed, 60 insertions(+), 4 deletions(-)

Comments

Jan Beulich Aug. 26, 2020, 2:02 p.m. UTC | #1

On 18.08.2020 12:30, Paul Durrant wrote:
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -93,7 +93,18 @@ struct domain_shared_info_context {
>  
>  DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
>  
> -#define DOMAIN_SAVE_CODE_MAX 2
> +#if defined(__i386__) || defined(__x86_64__)
> +struct domain_tsc_info_context {
> +    uint32_t mode;
> +    uint32_t incarnation;
> +    uint64_t elapsed_nsec;
> +    uint32_t khz;
> +};

sizeof() for this struct varies between 32-bit and 64-bit - is
this not a problem? (alignof() varies too, but there I think
it's indeed not a problem, albeit it could still be taken care
of by using uint64_aligned_t, alongside the addition of an
explicit padding field).

Jan

Paul Durrant Aug. 28, 2020, 11:08 a.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 26 August 2020 15:03
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Ian Jackson
> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>; George
> Dunlap <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> Subject: RE: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 18.08.2020 12:30, Paul Durrant wrote:
> > --- a/xen/include/public/save.h
> > +++ b/xen/include/public/save.h
> > @@ -93,7 +93,18 @@ struct domain_shared_info_context {
> >
> >  DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
> >
> > -#define DOMAIN_SAVE_CODE_MAX 2
> > +#if defined(__i386__) || defined(__x86_64__)
> > +struct domain_tsc_info_context {
> > +    uint32_t mode;
> > +    uint32_t incarnation;
> > +    uint64_t elapsed_nsec;
> > +    uint32_t khz;
> > +};
> 
> sizeof() for this struct varies between 32-bit and 64-bit - is
> this not a problem? (alignof() varies too, but there I think
> it's indeed not a problem, albeit it could still be taken care
> of by using uint64_aligned_t, alongside the addition of an
> explicit padding field).

I don't think it should matter because domain context records have implicit padding to align up to the next 64-bit boundary, so as long as fields within the struct don't move (which I think is true in this case) then we should be ok.

  Paul

> 
> Jan

Jan Beulich Aug. 28, 2020, 3:53 p.m. UTC | #3

On 28.08.2020 13:08, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 26 August 2020 15:03
>> To: Paul Durrant <[e-mail]>
>> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Ian Jackson
>> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>; George
>> Dunlap <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Roger Pau Monné <[e-mail]>
>> Subject: RE: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open
>> attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 18.08.2020 12:30, Paul Durrant wrote:
>>> --- a/xen/include/public/save.h
>>> +++ b/xen/include/public/save.h
>>> @@ -93,7 +93,18 @@ struct domain_shared_info_context {
>>>
>>>  DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
>>>
>>> -#define DOMAIN_SAVE_CODE_MAX 2
>>> +#if defined(__i386__) || defined(__x86_64__)
>>> +struct domain_tsc_info_context {
>>> +    uint32_t mode;
>>> +    uint32_t incarnation;
>>> +    uint64_t elapsed_nsec;
>>> +    uint32_t khz;
>>> +};
>>
>> sizeof() for this struct varies between 32-bit and 64-bit - is
>> this not a problem? (alignof() varies too, but there I think
>> it's indeed not a problem, albeit it could still be taken care
>> of by using uint64_aligned_t, alongside the addition of an
>> explicit padding field).
> 
> I don't think it should matter because domain context records have
> implicit padding to align up to the next 64-bit boundary,

Could you remind me where this is written down and enforced?

> so as long as fields within the struct don't move (which I think
> is true in this case) then we should be ok.

Right.

Jan

Paul Durrant Aug. 28, 2020, 4:36 p.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 28 August 2020 16:53
> To: [e-mail]
> Cc: [e-mail]; 'Ian Jackson' <[e-mail]>; 'Wei Liu' <[e-mail]>;
> 'Andrew Cooper' <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Julien
> Grall' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>; 'Roger Pau Monné'
> <[e-mail]>
> Subject: Re: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
> 
> On 28.08.2020 13:08, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 26 August 2020 15:03
> >> To: Paul Durrant <[e-mail]>
> >> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Ian Jackson
> >> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>;
> George
> >> Dunlap <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; Roger Pau Monné <[e-mail]>
> >> Subject: RE: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
> >>
> >> CAUTION: This email originated from outside of the organization. Do not click links or open
> >> attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 18.08.2020 12:30, Paul Durrant wrote:
> >>> --- a/xen/include/public/save.h
> >>> +++ b/xen/include/public/save.h
> >>> @@ -93,7 +93,18 @@ struct domain_shared_info_context {
> >>>
> >>>  DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
> >>>
> >>> -#define DOMAIN_SAVE_CODE_MAX 2
> >>> +#if defined(__i386__) || defined(__x86_64__)
> >>> +struct domain_tsc_info_context {
> >>> +    uint32_t mode;
> >>> +    uint32_t incarnation;
> >>> +    uint64_t elapsed_nsec;
> >>> +    uint32_t khz;
> >>> +};
> >>
> >> sizeof() for this struct varies between 32-bit and 64-bit - is
> >> this not a problem? (alignof() varies too, but there I think
> >> it's indeed not a problem, albeit it could still be taken care
> >> of by using uint64_aligned_t, alongside the addition of an
> >> explicit padding field).
> >
> > I don't think it should matter because domain context records have
> > implicit padding to align up to the next 64-bit boundary,
> 
> Could you remind me where this is written down and enforced?
> 

With the series fully applied, see xen/include/public/save.h line 62-68 for the comment and then see domain_save_end() in xen/common/save.c for where the padding is applied.

  Paul

> > so as long as fields within the struct don't move (which I think
> > is true in this case) then we should be ok.
> 
> Right.
> 
> Jan

Jan Beulich Aug. 31, 2020, 7:23 a.m. UTC | #5

On 28.08.2020 18:36, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 28 August 2020 16:53
>> To: [e-mail]
>> Cc: [e-mail]; 'Ian Jackson' <[e-mail]>; 'Wei Liu' <[e-mail]>;
>> 'Andrew Cooper' <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Julien
>> Grall' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>; 'Roger Pau Monné'
>> <[e-mail]>
>> Subject: Re: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
>>
>> On 28.08.2020 13:08, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <[e-mail]>
>>>> Sent: 26 August 2020 15:03
>>>> To: Paul Durrant <[e-mail]>
>>>> Cc: [e-mail]; Durrant, Paul <[e-mail]>; Ian Jackson
>>>> <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper <[e-mail]>;
>> George
>>>> Dunlap <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
>>>> <[e-mail]>; Roger Pau Monné <[e-mail]>
>>>> Subject: RE: [EXTERNAL] [PATCH v7 8/9] x86/time: add a domain context record for tsc_info...
>>>>
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open
>>>> attachments unless you can confirm the sender and know the content is safe.
>>>>
>>>>
>>>>
>>>> On 18.08.2020 12:30, Paul Durrant wrote:
>>>>> --- a/xen/include/public/save.h
>>>>> +++ b/xen/include/public/save.h
>>>>> @@ -93,7 +93,18 @@ struct domain_shared_info_context {
>>>>>
>>>>>  DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
>>>>>
>>>>> -#define DOMAIN_SAVE_CODE_MAX 2
>>>>> +#if defined(__i386__) || defined(__x86_64__)
>>>>> +struct domain_tsc_info_context {
>>>>> +    uint32_t mode;
>>>>> +    uint32_t incarnation;
>>>>> +    uint64_t elapsed_nsec;
>>>>> +    uint32_t khz;
>>>>> +};
>>>>
>>>> sizeof() for this struct varies between 32-bit and 64-bit - is
>>>> this not a problem? (alignof() varies too, but there I think
>>>> it's indeed not a problem, albeit it could still be taken care
>>>> of by using uint64_aligned_t, alongside the addition of an
>>>> explicit padding field).
>>>
>>> I don't think it should matter because domain context records have
>>> implicit padding to align up to the next 64-bit boundary,
>>
>> Could you remind me where this is written down and enforced?
>>
> 
> With the series fully applied, see xen/include/public/save.h
> line 62-68 for the comment and then see domain_save_end() in
> xen/common/save.c for where the padding is applied.

Ah, yes, this helped find the places in the patches. Therefore with
the stray blank line addition removed from tools/misc/xen-domctx.c
Reviewed-by: Jan Beulich <[e-mail]>

Jan

Patch

diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 6ead7ea89d..e09d75cf68 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -59,9 +59,20 @@  static void dump_header(void)
 
     printf("    HEADER: magic %#x, version %u\n",
            h->magic, h->version);
+}
+
+static void dump_tsc_info(void)
+{
+    DOMAIN_SAVE_TYPE(TSC_INFO) *t;
 
+    GET_PTR(t);
+
+    printf("    TSC_INFO: mode: %u incarnation: %u\n"
+           "              khz %u elapsed_nsec: %"PRIu64"\n",
+           t->mode, t->incarnation, t->khz, t->elapsed_nsec);
 }
 
+
 static void print_binary(const char *prefix, const void *val, size_t size,
                          const char *suffix)
 {
@@ -251,6 +262,7 @@  int main(int argc, char **argv)
             {
             case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
             case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
+            case DOMAIN_SAVE_CODE(TSC_INFO): dump_tsc_info(); break;
             case DOMAIN_SAVE_CODE(END): dump_end(); break;
             default:
                 printf("Unknown type %u: skipping\n", desc->typecode);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 505e54ebd7..05f65fbf12 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -26,6 +26,7 @@ 
 #include <xen/symbols.h>
 #include <xen/keyhandler.h>
 #include <xen/guest_access.h>
+#include <xen/save.h>
 #include <asm/io.h>
 #include <asm/iocap.h>
 #include <asm/msr.h>
@@ -2334,7 +2335,7 @@  int host_tsc_is_safe(void)
  * called to collect tsc-related data only for save file or live
  * migrate; called after last rdtsc is done on this incarnation
  */
-void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
+void tsc_get_info(const struct domain *d, uint32_t *tsc_mode,
                   uint64_t *elapsed_nsec, uint32_t *gtsc_khz,
                   uint32_t *incarnation)
 {
@@ -2453,6 +2454,37 @@  int tsc_set_info(struct domain *d,
     return 0;
 }
 
+static int save_tsc_info(const struct domain *d, struct domain_context *c,
+                         bool dry_run)
+{
+    struct domain_tsc_info_context ctxt;
+
+    if ( !dry_run )
+        tsc_get_info(d, &ctxt.mode, &ctxt.elapsed_nsec, &ctxt.khz,
+                     &ctxt.incarnation);
+
+    return DOMAIN_SAVE_ENTRY(TSC_INFO, c, 0, &ctxt, sizeof(ctxt));
+}
+
+static int load_tsc_info(struct domain *d, struct domain_context *c)
+{
+    struct domain_tsc_info_context ctxt;
+    unsigned int i;
+    int rc;
+
+    rc = DOMAIN_LOAD_ENTRY(TSC_INFO, c, &i, &ctxt, sizeof(ctxt));
+    if ( rc )
+        return rc;
+
+    if ( i ) /* expect only a single instance */
+        return -ENXIO;
+
+    return tsc_set_info(d, ctxt.mode, ctxt.elapsed_nsec, ctxt.khz,
+                        ctxt.incarnation);
+}
+
+DOMAIN_REGISTER_SAVE_LOAD(TSC_INFO, save_tsc_info, load_tsc_info);
+
 /* vtsc may incur measurable performance degradation, diagnose with this */
 static void dump_softtsc(unsigned char key)
 {
diff --git a/xen/include/asm-x86/time.h b/xen/include/asm-x86/time.h
index f347311cc4..7f2ce6226a 100644
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -59,8 +59,9 @@  u64 gtsc_to_gtime(struct domain *d, u64 tsc);
 int tsc_set_info(struct domain *d, uint32_t tsc_mode, uint64_t elapsed_nsec,
                  uint32_t gtsc_khz, uint32_t incarnation);
 
-void tsc_get_info(struct domain *d, uint32_t *tsc_mode, uint64_t *elapsed_nsec,
-                  uint32_t *gtsc_khz, uint32_t *incarnation);
+void tsc_get_info(const struct domain *d, uint32_t *tsc_mode,
+                  uint64_t *elapsed_nsec, uint32_t *gtsc_khz,
+                  uint32_t *incarnation);
    
 
 void force_update_vcpu_system_time(struct vcpu *v);
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index 0e855a4b97..aeb17298eb 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -93,7 +93,18 @@  struct domain_shared_info_context {
 
 DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
 
-#define DOMAIN_SAVE_CODE_MAX 2
+#if defined(__i386__) || defined(__x86_64__)
+struct domain_tsc_info_context {
+    uint32_t mode;
+    uint32_t incarnation;
+    uint64_t elapsed_nsec;
+    uint32_t khz;
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(TSC_INFO, 3, struct domain_tsc_info_context);
+#endif
+
+#define DOMAIN_SAVE_CODE_MAX 3
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */