diff mbox series

[QEMU,v3,6/9] vmstate: Add support for kernel integer types

Message ID 20190617175658.135869-7-liran.alon@oracle.com (mailing list archive)
State New, archived
Headers show
Series : KVM: i386: Add support for save and restore of nested state | expand

Commit Message

Liran Alon June 17, 2019, 5:56 p.m. UTC 
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Maran Wilson <maran.wilson@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 include/migration/vmstate.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Dr. David Alan Gilbert June 18, 2019, 8:55 a.m. UTC | #1 
* Liran Alon (liran.alon@oracle.com) wrote:
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Maran Wilson <maran.wilson@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  include/migration/vmstate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9224370ed59a..a85424fb0483 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -797,6 +797,15 @@ extern const VMStateInfo vmstate_info_qtailq;
>  #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
>      VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)

A comment here stating they're for Linux kernel types would be nice.

> +#define VMSTATE_U8_V(_f, _s, _v)                                   \
> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint8, __u8)
> +#define VMSTATE_U16_V(_f, _s, _v)                                  \
> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, __u16)
> +#define VMSTATE_U32_V(_f, _s, _v)                                  \
> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, __u32)
> +#define VMSTATE_U64_V(_f, _s, _v)                                  \
> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, __u64)
> +

Have you checked that builds OK on a non-Linux system?

Dave

>  #define VMSTATE_BOOL(_f, _s)                                          \
>      VMSTATE_BOOL_V(_f, _s, 0)
>  
> @@ -818,6 +827,15 @@ extern const VMStateInfo vmstate_info_qtailq;
>  #define VMSTATE_UINT64(_f, _s)                                        \
>      VMSTATE_UINT64_V(_f, _s, 0)
>  
> +#define VMSTATE_U8(_f, _s)                                         \
> +    VMSTATE_U8_V(_f, _s, 0)
> +#define VMSTATE_U16(_f, _s)                                        \
> +    VMSTATE_U16_V(_f, _s, 0)
> +#define VMSTATE_U32(_f, _s)                                        \
> +    VMSTATE_U32_V(_f, _s, 0)
> +#define VMSTATE_U64(_f, _s)                                        \
> +    VMSTATE_U64_V(_f, _s, 0)
> +
>  #define VMSTATE_UINT8_EQUAL(_f, _s, _err_hint)                        \
>      VMSTATE_SINGLE_FULL(_f, _s, 0, 0,                                 \
>                          vmstate_info_uint8_equal, uint8_t, _err_hint)
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Liran Alon June 18, 2019, 3:36 p.m. UTC | #2 
> On 18 Jun 2019, at 11:55, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> 
> * Liran Alon (liran.alon@oracle.com) wrote:
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Reviewed-by: Maran Wilson <maran.wilson@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>> include/migration/vmstate.h | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>> 
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 9224370ed59a..a85424fb0483 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -797,6 +797,15 @@ extern const VMStateInfo vmstate_info_qtailq;
>> #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
>>     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
> 
> A comment here stating they're for Linux kernel types would be nice.

I didn’t want to state this because in theory these types can be used not in kernel context…
I thought commit message is sufficient. I think comments in code should be made to clarify
things. But to justify existence I think commit message should be used.
But if you insist, I have no strong objection of adding such comment.

> 
>> +#define VMSTATE_U8_V(_f, _s, _v)                                   \
>> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint8, __u8)
>> +#define VMSTATE_U16_V(_f, _s, _v)                                  \
>> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, __u16)
>> +#define VMSTATE_U32_V(_f, _s, _v)                                  \
>> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, __u32)
>> +#define VMSTATE_U64_V(_f, _s, _v)                                  \
>> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, __u64)
>> +
> 
> Have you checked that builds OK on a non-Linux system?

Hmm that’s a good point. No. :P

-Liran

> 
> Dave
> 
>> #define VMSTATE_BOOL(_f, _s)                                          \
>>     VMSTATE_BOOL_V(_f, _s, 0)
>> 
>> @@ -818,6 +827,15 @@ extern const VMStateInfo vmstate_info_qtailq;
>> #define VMSTATE_UINT64(_f, _s)                                        \
>>     VMSTATE_UINT64_V(_f, _s, 0)
>> 
>> +#define VMSTATE_U8(_f, _s)                                         \
>> +    VMSTATE_U8_V(_f, _s, 0)
>> +#define VMSTATE_U16(_f, _s)                                        \
>> +    VMSTATE_U16_V(_f, _s, 0)
>> +#define VMSTATE_U32(_f, _s)                                        \
>> +    VMSTATE_U32_V(_f, _s, 0)
>> +#define VMSTATE_U64(_f, _s)                                        \
>> +    VMSTATE_U64_V(_f, _s, 0)
>> +
>> #define VMSTATE_UINT8_EQUAL(_f, _s, _err_hint)                        \
>>     VMSTATE_SINGLE_FULL(_f, _s, 0, 0,                                 \
>>                         vmstate_info_uint8_equal, uint8_t, _err_hint)
>> -- 
>> 2.20.1
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert June 18, 2019, 3:42 p.m. UTC | #3 
* Liran Alon (liran.alon@oracle.com) wrote:
> 
> 
> > On 18 Jun 2019, at 11:55, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > 
> > * Liran Alon (liran.alon@oracle.com) wrote:
> >> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> >> Reviewed-by: Maran Wilson <maran.wilson@oracle.com>
> >> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> >> ---
> >> include/migration/vmstate.h | 18 ++++++++++++++++++
> >> 1 file changed, 18 insertions(+)
> >> 
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 9224370ed59a..a85424fb0483 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -797,6 +797,15 @@ extern const VMStateInfo vmstate_info_qtailq;
> >> #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
> >>     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
> > 
> > A comment here stating they're for Linux kernel types would be nice.
> 
> I didn’t want to state this because in theory these types can be used not in kernel context…
> I thought commit message is sufficient. I think comments in code should be made to clarify
> things. But to justify existence I think commit message should be used.
> But if you insist, I have no strong objection of adding such comment.

It's only a 'would be nice' - it's just I don't want people trying to
use them for other places (I'm not sure what happens if you pass a
uint8_t to VMSTATE_U8 ???

> > 
> >> +#define VMSTATE_U8_V(_f, _s, _v)                                   \
> >> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint8, __u8)
> >> +#define VMSTATE_U16_V(_f, _s, _v)                                  \
> >> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, __u16)
> >> +#define VMSTATE_U32_V(_f, _s, _v)                                  \
> >> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, __u32)
> >> +#define VMSTATE_U64_V(_f, _s, _v)                                  \
> >> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, __u64)
> >> +
> > 
> > Have you checked that builds OK on a non-Linux system?
> 
> Hmm that’s a good point. No. :P

Worth a check if you can find one lying around :-)

Dave

> -Liran
> 
> > 
> > Dave
> > 
> >> #define VMSTATE_BOOL(_f, _s)                                          \
> >>     VMSTATE_BOOL_V(_f, _s, 0)
> >> 
> >> @@ -818,6 +827,15 @@ extern const VMStateInfo vmstate_info_qtailq;
> >> #define VMSTATE_UINT64(_f, _s)                                        \
> >>     VMSTATE_UINT64_V(_f, _s, 0)
> >> 
> >> +#define VMSTATE_U8(_f, _s)                                         \
> >> +    VMSTATE_U8_V(_f, _s, 0)
> >> +#define VMSTATE_U16(_f, _s)                                        \
> >> +    VMSTATE_U16_V(_f, _s, 0)
> >> +#define VMSTATE_U32(_f, _s)                                        \
> >> +    VMSTATE_U32_V(_f, _s, 0)
> >> +#define VMSTATE_U64(_f, _s)                                        \
> >> +    VMSTATE_U64_V(_f, _s, 0)
> >> +
> >> #define VMSTATE_UINT8_EQUAL(_f, _s, _err_hint)                        \
> >>     VMSTATE_SINGLE_FULL(_f, _s, 0, 0,                                 \
> >>                         vmstate_info_uint8_equal, uint8_t, _err_hint)
> >> -- 
> >> 2.20.1
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Paolo Bonzini June 18, 2019, 4:44 p.m. UTC | #4 
On 18/06/19 17:42, Dr. David Alan Gilbert wrote:
>>> Have you checked that builds OK on a non-Linux system?
>> Hmm that’s a good point. No. :P
> Worth a check if you can find one lying around :-)

It does not, but it's a macro so it's enough to enclose the uses in
#ifdef CONFIG_LINUX or CONFIG_KVM.

Paolo
diff mbox series

Patch

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9224370ed59a..a85424fb0483 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -797,6 +797,15 @@  extern const VMStateInfo vmstate_info_qtailq;
 #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
 
+#define VMSTATE_U8_V(_f, _s, _v)                                   \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint8, __u8)
+#define VMSTATE_U16_V(_f, _s, _v)                                  \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16, __u16)
+#define VMSTATE_U32_V(_f, _s, _v)                                  \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, __u32)
+#define VMSTATE_U64_V(_f, _s, _v)                                  \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, __u64)
+
 #define VMSTATE_BOOL(_f, _s)                                          \
     VMSTATE_BOOL_V(_f, _s, 0)
 
@@ -818,6 +827,15 @@  extern const VMStateInfo vmstate_info_qtailq;
 #define VMSTATE_UINT64(_f, _s)                                        \
     VMSTATE_UINT64_V(_f, _s, 0)
 
+#define VMSTATE_U8(_f, _s)                                         \
+    VMSTATE_U8_V(_f, _s, 0)
+#define VMSTATE_U16(_f, _s)                                        \
+    VMSTATE_U16_V(_f, _s, 0)
+#define VMSTATE_U32(_f, _s)                                        \
+    VMSTATE_U32_V(_f, _s, 0)
+#define VMSTATE_U64(_f, _s)                                        \
+    VMSTATE_U64_V(_f, _s, 0)
+
 #define VMSTATE_UINT8_EQUAL(_f, _s, _err_hint)                        \
     VMSTATE_SINGLE_FULL(_f, _s, 0, 0,                                 \
                         vmstate_info_uint8_equal, uint8_t, _err_hint)