[Qemu-devel] [PATCH 0/3] Fix/add vmstate handling in some I2C code

[Qemu-devel] [PATCH 0/3] Fix/add vmstate handling in some I2C code

[minyard]

These changes allow SMBus access while doing a state transfer.
Seems like a good idea to me in general.

I have these queued for the SMBus IPMI driver work, of course.

[Qemu-devel] [PATCH 1/3] i2c:pm_smbus: Fix state transfer

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Transfer the state information for the SMBus registers and
internal data so it will work on a VM transfer.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/piix4.c           |  3 ++-
 hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
 hw/i2c/smbus_ich9.c       |  5 +++--
 include/hw/i2c/pm_smbus.h |  2 ++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index e330f24c71..313305f5a0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
  */
 static const VMStateDescription vmstate_acpi = {
     .name = "piix4_pm",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .minimum_version_id_old = 1,
     .load_state_old = acpi_load_old,
@@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
         VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
         VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
+        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
         VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
         VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
         VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 685a2378ed..4d021bb017 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -402,6 +402,26 @@ static const MemoryRegionOps pm_smbus_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+const VMStateDescription pmsmb_vmstate = {
+    .name = "pmsmb",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(smb_stat, PMSMBus),
+        VMSTATE_UINT8(smb_ctl, PMSMBus),
+        VMSTATE_UINT8(smb_cmd, PMSMBus),
+        VMSTATE_UINT8(smb_addr, PMSMBus),
+        VMSTATE_UINT8(smb_data0, PMSMBus),
+        VMSTATE_UINT8(smb_data1, PMSMBus),
+        VMSTATE_UINT32(smb_index, PMSMBus),
+        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
+        VMSTATE_UINT8(smb_auxctl, PMSMBus),
+        VMSTATE_BOOL(i2c_enable, PMSMBus),
+        VMSTATE_BOOL(op_done, PMSMBus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
 {
     smb->op_done = true;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 2a8b49e02f..4202b71da5 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -47,10 +47,11 @@ typedef struct ICH9SMBState {
 
 static const VMStateDescription vmstate_ich9_smbus = {
     .name = "ich9_smb",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 060d3c6ac0..471345eaa2 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -35,4 +35,6 @@ typedef struct PMSMBus {
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
 
+extern const VMStateDescription pmsmb_vmstate;
+
 #endif /* PM_SMBUS_H */
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/3] i2c:pm_smbus: Fix state transfer

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Transfer the state information for the SMBus registers and
> internal data so it will work on a VM transfer.

Weren't the comments from August were that we'd prefer to see this in
a subsection so we didn't break migration compatibility.

Dave

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/acpi/piix4.c           |  3 ++-
>  hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
>  hw/i2c/smbus_ich9.c       |  5 +++--
>  include/hw/i2c/pm_smbus.h |  2 ++
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e330f24c71..313305f5a0 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
>   */
>  static const VMStateDescription vmstate_acpi = {
>      .name = "piix4_pm",
> -    .version_id = 3,
> +    .version_id = 4,
>      .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
>      .load_state_old = acpi_load_old,
> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
>          VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>          VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>          VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> +        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
>          VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>          VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>          VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 685a2378ed..4d021bb017 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -402,6 +402,26 @@ static const MemoryRegionOps pm_smbus_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +const VMStateDescription pmsmb_vmstate = {
> +    .name = "pmsmb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(smb_stat, PMSMBus),
> +        VMSTATE_UINT8(smb_ctl, PMSMBus),
> +        VMSTATE_UINT8(smb_cmd, PMSMBus),
> +        VMSTATE_UINT8(smb_addr, PMSMBus),
> +        VMSTATE_UINT8(smb_data0, PMSMBus),
> +        VMSTATE_UINT8(smb_data1, PMSMBus),
> +        VMSTATE_UINT32(smb_index, PMSMBus),
> +        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> +        VMSTATE_UINT8(smb_auxctl, PMSMBus),
> +        VMSTATE_BOOL(i2c_enable, PMSMBus),
> +        VMSTATE_BOOL(op_done, PMSMBus),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
>  {
>      smb->op_done = true;
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index 2a8b49e02f..4202b71da5 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -47,10 +47,11 @@ typedef struct ICH9SMBState {
>  
>  static const VMStateDescription vmstate_ich9_smbus = {
>      .name = "ich9_smb",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> +        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> +        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 060d3c6ac0..471345eaa2 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -35,4 +35,6 @@ typedef struct PMSMBus {
>  
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>  
> +extern const VMStateDescription pmsmb_vmstate;
> +
>  #endif /* PM_SMBUS_H */
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] i2c:pm_smbus: Fix state transfer

[Corey Minyard]

On 11/12/18 11:47 AM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Transfer the state information for the SMBus registers and
>> internal data so it will work on a VM transfer.
> Weren't the comments from August were that we'd prefer to see this in
> a subsection so we didn't break migration compatibility.


Yeah, I screwed up here, but I realized what happened.  I started working
this some more and discovered that the eeprom didn't work on q35 at all,
and while working on that I found some other issues, and then I had
another critical thing come up and this got lost.  I apologize, I should 
have
looked back.

I will go back and address your comments from the previous emails
once I get everything sorted out.

Thanks,

-corey


> Dave
>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   hw/acpi/piix4.c           |  3 ++-
>>   hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
>>   hw/i2c/smbus_ich9.c       |  5 +++--
>>   include/hw/i2c/pm_smbus.h |  2 ++
>>   4 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index e330f24c71..313305f5a0 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
>>    */
>>   static const VMStateDescription vmstate_acpi = {
>>       .name = "piix4_pm",
>> -    .version_id = 3,
>> +    .version_id = 4,
>>       .minimum_version_id = 3,
>>       .minimum_version_id_old = 1,
>>       .load_state_old = acpi_load_old,
>> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
>>           VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>>           VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>>           VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
>> +        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
>>           VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>>           VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>>           VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>> index 685a2378ed..4d021bb017 100644
>> --- a/hw/i2c/pm_smbus.c
>> +++ b/hw/i2c/pm_smbus.c
>> @@ -402,6 +402,26 @@ static const MemoryRegionOps pm_smbus_ops = {
>>       .endianness = DEVICE_LITTLE_ENDIAN,
>>   };
>>   
>> +const VMStateDescription pmsmb_vmstate = {
>> +    .name = "pmsmb",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(smb_stat, PMSMBus),
>> +        VMSTATE_UINT8(smb_ctl, PMSMBus),
>> +        VMSTATE_UINT8(smb_cmd, PMSMBus),
>> +        VMSTATE_UINT8(smb_addr, PMSMBus),
>> +        VMSTATE_UINT8(smb_data0, PMSMBus),
>> +        VMSTATE_UINT8(smb_data1, PMSMBus),
>> +        VMSTATE_UINT32(smb_index, PMSMBus),
>> +        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
>> +        VMSTATE_UINT8(smb_auxctl, PMSMBus),
>> +        VMSTATE_BOOL(i2c_enable, PMSMBus),
>> +        VMSTATE_BOOL(op_done, PMSMBus),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
>>   {
>>       smb->op_done = true;
>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>> index 2a8b49e02f..4202b71da5 100644
>> --- a/hw/i2c/smbus_ich9.c
>> +++ b/hw/i2c/smbus_ich9.c
>> @@ -47,10 +47,11 @@ typedef struct ICH9SMBState {
>>   
>>   static const VMStateDescription vmstate_ich9_smbus = {
>>       .name = "ich9_smb",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .minimum_version_id = 1,
>>       .fields = (VMStateField[]) {
>> -        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
>> +        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
>> +        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
>> index 060d3c6ac0..471345eaa2 100644
>> --- a/include/hw/i2c/pm_smbus.h
>> +++ b/include/hw/i2c/pm_smbus.h
>> @@ -35,4 +35,6 @@ typedef struct PMSMBus {
>>   
>>   void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>>   
>> +extern const VMStateDescription pmsmb_vmstate;
>> +
>>   #endif /* PM_SMBUS_H */
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/3] i2c: Add an SMBus vmstate structure

[minyard]

From: Corey Minyard <cminyard@mvista.com>

There is no vmstate handling for SMBus, so no device sitting on SMBus
can have a state transfer that works reliable.  So add it.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i2c/smbus.c         | 14 ++++++++++++++
 include/hw/i2c/smbus.h | 18 +++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 6ff77c582f..b0774d7683 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
     return 0;
 }
 
+const VMStateDescription vmstate_smbus_device = {
+    .name = TYPE_SMBUS_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
+        VMSTATE_INT32(mode, SMBusDevice),
+        VMSTATE_INT32(data_len, SMBusDevice),
+        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
+        VMSTATE_UINT8(command, SMBusDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
     I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index d8b1b9ee81..7b52020121 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
     uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
 } SMBusDeviceClass;
 
+#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
+
 struct SMBusDevice {
     /* The SMBus protocol is implemented on top of I2C.  */
     I2CSlave i2c;
 
     /* Remaining fields for internal use only.  */
-    int mode;
-    int data_len;
-    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
+    int32_t mode;
+    int32_t data_len;
+    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
     uint8_t command;
 };
 
@@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
 
+extern const VMStateDescription vmstate_smbus_device;
+
+#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(SMBusDevice),                               \
+    .vmsd       = &vmstate_smbus_device,                             \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
+}
+
 #endif
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/3] i2c: Add an SMBus vmstate structure

[Philippe Mathieu-Daudé]

Hi Corey,

On 7/11/18 16:54, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> There is no vmstate handling for SMBus, so no device sitting on SMBus
> can have a state transfer that works reliable.  So add it.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   hw/i2c/smbus.c         | 14 ++++++++++++++
>   include/hw/i2c/smbus.h | 18 +++++++++++++++---
>   2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
> index 6ff77c582f..b0774d7683 100644
> --- a/hw/i2c/smbus.c
> +++ b/hw/i2c/smbus.c
> @@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
>       return 0;
>   }
>   
> +const VMStateDescription vmstate_smbus_device = {
> +    .name = TYPE_SMBUS_DEVICE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
> +        VMSTATE_INT32(mode, SMBusDevice),
> +        VMSTATE_INT32(data_len, SMBusDevice),
> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
> +        VMSTATE_UINT8(command, SMBusDevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static void smbus_device_class_init(ObjectClass *klass, void *data)
>   {
>       I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
> index d8b1b9ee81..7b52020121 100644
> --- a/include/hw/i2c/smbus.h
> +++ b/include/hw/i2c/smbus.h
> @@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
>       uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
>   } SMBusDeviceClass;
>   
> +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
> +
>   struct SMBusDevice {
>       /* The SMBus protocol is implemented on top of I2C.  */
>       I2CSlave i2c;
>   
>       /* Remaining fields for internal use only.  */
> -    int mode;
> -    int data_len;
> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
> +    int32_t mode;
> +    int32_t data_len;
> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];

Those changes are not in your commit description.
Can you include them in a separate patch?
Thanks,
Phil.

>       uint8_t command;
>   };
>   
> @@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
>   void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>                          const uint8_t *eeprom_spd, int size);
>   
> +extern const VMStateDescription vmstate_smbus_device;
> +
> +#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(SMBusDevice),                               \
> +    .vmsd       = &vmstate_smbus_device,                             \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
> +}
> +
>   #endif
> 

Re: [Qemu-devel] [PATCH 2/3] i2c: Add an SMBus vmstate structure

[Peter Maydell]

On 8 November 2018 at 14:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Corey,
>
>
> On 7/11/18 16:54, minyard@acm.org wrote:
>>
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> There is no vmstate handling for SMBus, so no device sitting on SMBus
>> can have a state transfer that works reliable.  So add it.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   hw/i2c/smbus.c         | 14 ++++++++++++++
>>   include/hw/i2c/smbus.h | 18 +++++++++++++++---
>>   2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
>> index 6ff77c582f..b0774d7683 100644
>> --- a/hw/i2c/smbus.c
>> +++ b/hw/i2c/smbus.c
>> @@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr,
>> uint8_t command, uint8_t *data,
>>       return 0;
>>   }
>>   +const VMStateDescription vmstate_smbus_device = {
>> +    .name = TYPE_SMBUS_DEVICE,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
>> +        VMSTATE_INT32(mode, SMBusDevice),
>> +        VMSTATE_INT32(data_len, SMBusDevice),
>> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
>> +        VMSTATE_UINT8(command, SMBusDevice),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void smbus_device_class_init(ObjectClass *klass, void *data)
>>   {
>>       I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
>> index d8b1b9ee81..7b52020121 100644
>> --- a/include/hw/i2c/smbus.h
>> +++ b/include/hw/i2c/smbus.h
>> @@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
>>       uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
>>   } SMBusDeviceClass;
>>   +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
>> +
>>   struct SMBusDevice {
>>       /* The SMBus protocol is implemented on top of I2C.  */
>>       I2CSlave i2c;
>>         /* Remaining fields for internal use only.  */
>> -    int mode;
>> -    int data_len;
>> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
>> +    int32_t mode;
>> +    int32_t data_len;
>> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
>
>
> Those changes are not in your commit description.
> Can you include them in a separate patch?

These are just the standard "promote types in the state struct
to fixed-width ones required by the VMSTATE macros", aren't
they? I think they're fine as part of the vmstate conversion.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] i2c: Add an SMBus vmstate structure

[Philippe Mathieu-Daudé]

On 8/11/18 15:40, Peter Maydell wrote:
> On 8 November 2018 at 14:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Hi Corey,
>>
>>
>> On 7/11/18 16:54, minyard@acm.org wrote:
>>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> There is no vmstate handling for SMBus, so no device sitting on SMBus
>>> can have a state transfer that works reliable.  So add it.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>>    hw/i2c/smbus.c         | 14 ++++++++++++++
>>>    include/hw/i2c/smbus.h | 18 +++++++++++++++---
>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
>>> index 6ff77c582f..b0774d7683 100644
>>> --- a/hw/i2c/smbus.c
>>> +++ b/hw/i2c/smbus.c
>>> @@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr,
>>> uint8_t command, uint8_t *data,
>>>        return 0;
>>>    }
>>>    +const VMStateDescription vmstate_smbus_device = {
>>> +    .name = TYPE_SMBUS_DEVICE,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields      = (VMStateField[]) {
>>> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
>>> +        VMSTATE_INT32(mode, SMBusDevice),
>>> +        VMSTATE_INT32(data_len, SMBusDevice),
>>> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
>>> +        VMSTATE_UINT8(command, SMBusDevice),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>    static void smbus_device_class_init(ObjectClass *klass, void *data)
>>>    {
>>>        I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
>>> index d8b1b9ee81..7b52020121 100644
>>> --- a/include/hw/i2c/smbus.h
>>> +++ b/include/hw/i2c/smbus.h
>>> @@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
>>>        uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
>>>    } SMBusDeviceClass;
>>>    +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
>>> +
>>>    struct SMBusDevice {
>>>        /* The SMBus protocol is implemented on top of I2C.  */
>>>        I2CSlave i2c;
>>>          /* Remaining fields for internal use only.  */
>>> -    int mode;
>>> -    int data_len;
>>> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
>>> +    int32_t mode;
>>> +    int32_t data_len;
>>> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
>>
>>
>> Those changes are not in your commit description.
>> Can you include them in a separate patch?
> 
> These are just the standard "promote types in the state struct
> to fixed-width ones required by the VMSTATE macros", aren't
> they? I think they're fine as part of the vmstate conversion.

Fine then!

> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This was if the eeprom is accessed during a state transfer, the
transfer will be reliable.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3de35..d4430b0c5d 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -29,6 +29,8 @@
 
 //#define DEBUG
 
+#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
+
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     void *data;
@@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
     return eeprom_receive_byte(dev);
 }
 
+static const VMStateDescription vmstate_smbus_eeprom = {
+    .name = TYPE_SMBUS_EEPROM_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
@@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->write_data = eeprom_write_data;
     sc->read_data = eeprom_read_data;
     dc->props = smbus_eeprom_properties;
+    dc->vmsd = &vmstate_smbus_eeprom;
     /* Reason: pointer property "data" */
     dc->user_creatable = false;
 }
 
 static const TypeInfo smbus_eeprom_info = {
-    .name          = "smbus-eeprom",
+    .name          = TYPE_SMBUS_EEPROM_DEVICE,
     .parent        = TYPE_SMBUS_DEVICE,
     .instance_size = sizeof(SMBusEEPROMDevice),
     .class_init    = smbus_eeprom_class_initfn,
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 7 November 2018 at 15:54,  <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> This was if the eeprom is accessed during a state transfer, the
> transfer will be reliable.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..d4430b0c5d 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -29,6 +29,8 @@
>
>  //#define DEBUG
>
> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"

The part of this patch which is adding the usual QOM
macros is good, but we should provide also the
cast-macro (the one that's an OBJECT_CHECK(...)).
And this should be separate from adding the vmstate.

> +
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      void *data;
> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
>      return eeprom_receive_byte(dev);
>  }
>
> +static const VMStateDescription vmstate_smbus_eeprom = {
> +    .name = TYPE_SMBUS_EEPROM_DEVICE,

Generally we don't use the TYPE_FOO constant for the vmstate
name, because we can usually freely change the TYPE_FOO string without
breaking back-compat, but we can't change the vmstate name.
So we decouple them to make it more obvious when a change might
be changing the migration on-the-wire format.

> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

This doesn't do anything for migration of the actual data contents.
The current users of this device (hw/arm/aspeed.c and the
smbus_eeprom_init() function) doesn't do anything
to migrate the contents. For that matter, "user of the device
passes a pointer to a random lump of memory via a device property"
is a bit funky as an interface. The device should allocate its
own memory and migrate it, and the user should just pass the
initial required contents (default being "zero-initialize",
which is what everybody except the mips_fulong2e, mips_malta
and sam460ex want).

Does this also break migration from an old QEMU to a new one?
(For Aspeed that's probably ok, but we should flag it up in the
commit message if so. x86 uses need care...)

> +
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
>      SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      sc->write_data = eeprom_write_data;
>      sc->read_data = eeprom_read_data;
>      dc->props = smbus_eeprom_properties;
> +    dc->vmsd = &vmstate_smbus_eeprom;
>      /* Reason: pointer property "data" */
>      dc->user_creatable = false;
>  }
>
>  static const TypeInfo smbus_eeprom_info = {
> -    .name          = "smbus-eeprom",
> +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
>      .parent        = TYPE_SMBUS_DEVICE,
>      .instance_size = sizeof(SMBusEEPROMDevice),
>      .class_init    = smbus_eeprom_class_initfn,
> --
> 2.17.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Corey Minyard]

On 11/8/18 8:08 AM, Peter Maydell wrote:
> On 7 November 2018 at 15:54,  <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This was if the eeprom is accessed during a state transfer, the
>> transfer will be reliable.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index f18aa3de35..d4430b0c5d 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -29,6 +29,8 @@
>>
>>   //#define DEBUG
>>
>> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
> The part of this patch which is adding the usual QOM
> macros is good, but we should provide also the
> cast-macro (the one that's an OBJECT_CHECK(...)).
> And this should be separate from adding the vmstate.
>
>> +
>>   typedef struct SMBusEEPROMDevice {
>>       SMBusDevice smbusdev;
>>       void *data;
>> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
>>       return eeprom_receive_byte(dev);
>>   }
>>
>> +static const VMStateDescription vmstate_smbus_eeprom = {
>> +    .name = TYPE_SMBUS_EEPROM_DEVICE,
> Generally we don't use the TYPE_FOO constant for the vmstate
> name, because we can usually freely change the TYPE_FOO string without
> breaking back-compat, but we can't change the vmstate name.
> So we decouple them to make it more obvious when a change might
> be changing the migration on-the-wire format.
>

Ok, I've updated the code to use the standard type name and cast method
(in a separate patch) and I fixed the vmstate name.  It is better, you are
right.


>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
>> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> This doesn't do anything for migration of the actual data contents.
> The current users of this device (hw/arm/aspeed.c and the
> smbus_eeprom_init() function) doesn't do anything
> to migrate the contents. For that matter, "user of the device
> passes a pointer to a random lump of memory via a device property"
> is a bit funky as an interface. The device should allocate its
> own memory and migrate it, and the user should just pass the
> initial required contents (default being "zero-initialize",
> which is what everybody except the mips_fulong2e, mips_malta
> and sam460ex want).


I debated on this, and it depends on what the eeprom is used for.  If
it's a DRAM eeprom, it shouldn't need to be transferred.
If it's something where the host stores data for later use, you do.
Since it wasn't being cloned before, it probably won't matter now.

But even in the DRAM eeprom case, it shouldn't matter if it gets
transferred.  So it's probably safe to just transfer it.  Easy enough
to add.


>
> Does this also break migration from an old QEMU to a new one?
> (For Aspeed that's probably ok, but we should flag it up in the
> commit message if so. x86 uses need care...)
>
There is no transfer before, so I don't see why it would break anything.
Am I missing something?

-corey

>> +
>>   static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>   {
>>       SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
>> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>       sc->write_data = eeprom_write_data;
>>       sc->read_data = eeprom_read_data;
>>       dc->props = smbus_eeprom_properties;
>> +    dc->vmsd = &vmstate_smbus_eeprom;
>>       /* Reason: pointer property "data" */
>>       dc->user_creatable = false;
>>   }
>>
>>   static const TypeInfo smbus_eeprom_info = {
>> -    .name          = "smbus-eeprom",
>> +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
>>       .parent        = TYPE_SMBUS_DEVICE,
>>       .instance_size = sizeof(SMBusEEPROMDevice),
>>       .class_init    = smbus_eeprom_class_initfn,
>> --
>> 2.17.1
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote:
> On 11/8/18 8:08 AM, Peter Maydell wrote:
>> This doesn't do anything for migration of the actual data contents.
>> The current users of this device (hw/arm/aspeed.c and the
>> smbus_eeprom_init() function) doesn't do anything
>> to migrate the contents. For that matter, "user of the device
>> passes a pointer to a random lump of memory via a device property"
>> is a bit funky as an interface. The device should allocate its
>> own memory and migrate it, and the user should just pass the
>> initial required contents (default being "zero-initialize",
>> which is what everybody except the mips_fulong2e, mips_malta
>> and sam460ex want).

> I debated on this, and it depends on what the eeprom is used for.  If
> it's a DRAM eeprom, it shouldn't need to be transferred.

It's guest-visible data; the guest can write it and read it back.
So it needs to be migrated. Otherwise behaviour after migration
will not be the same as if the guest had never migrated.

>> Does this also break migration from an old QEMU to a new one?
>> (For Aspeed that's probably ok, but we should flag it up in the
>> commit message if so. x86 uses need care...)
>>
> There is no transfer before, so I don't see why it would break anything.
> Am I missing something?

I forget what the behaviour is where the source QEMU didn't
have a vmstate at all but the destination QEMU does expect
one. David can remind me...

thanks
- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Corey Minyard]

On 11/8/18 12:03 PM, Peter Maydell wrote:
> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote:
>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>> This doesn't do anything for migration of the actual data contents.
>>> The current users of this device (hw/arm/aspeed.c and the
>>> smbus_eeprom_init() function) doesn't do anything
>>> to migrate the contents. For that matter, "user of the device
>>> passes a pointer to a random lump of memory via a device property"
>>> is a bit funky as an interface. The device should allocate its
>>> own memory and migrate it, and the user should just pass the
>>> initial required contents (default being "zero-initialize",
>>> which is what everybody except the mips_fulong2e, mips_malta
>>> and sam460ex want).
>> I debated on this, and it depends on what the eeprom is used for.  If
>> it's a DRAM eeprom, it shouldn't need to be transferred.
> It's guest-visible data; the guest can write it and read it back.
> So it needs to be migrated. Otherwise behaviour after migration
> will not be the same as if the guest had never migrated.


I looked at adding it, but I ran into an issue.  The value is a

DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)

and that means the data has to be void *, but to transfer it it must be 
a uint8_t *.
The pointer property seems to be a cheap hack, I'm not sure what it will 
take
to fix it.

I was hoping this would be easy.  I guess transferring the eeprom is not
that important, the state of pm_smbus is a lot more critical.  But it would
be nice to fix it since I am messing with that code.

Thanks for your help.

-corey

>>> Does this also break migration from an old QEMU to a new one?
>>> (For Aspeed that's probably ok, but we should flag it up in the
>>> commit message if so. x86 uses need care...)
>>>
>> There is no transfer before, so I don't see why it would break anything.
>> Am I missing something?
> I forget what the behaviour is where the source QEMU didn't
> have a vmstate at all but the destination QEMU does expect
> one. David can remind me...
>
> thanks
> - PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 9 November 2018 at 14:56, Corey Minyard <minyard@acm.org> wrote:
> On 11/8/18 12:03 PM, Peter Maydell wrote:
>>
>> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote:
>>>
>>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>>>
>>>> This doesn't do anything for migration of the actual data contents.
>>>> The current users of this device (hw/arm/aspeed.c and the
>>>> smbus_eeprom_init() function) doesn't do anything
>>>> to migrate the contents. For that matter, "user of the device
>>>> passes a pointer to a random lump of memory via a device property"
>>>> is a bit funky as an interface. The device should allocate its
>>>> own memory and migrate it, and the user should just pass the
>>>> initial required contents (default being "zero-initialize",
>>>> which is what everybody except the mips_fulong2e, mips_malta
>>>> and sam460ex want).
>>>
>>> I debated on this, and it depends on what the eeprom is used for.  If
>>> it's a DRAM eeprom, it shouldn't need to be transferred.
>>
>> It's guest-visible data; the guest can write it and read it back.
>> So it needs to be migrated. Otherwise behaviour after migration
>> will not be the same as if the guest had never migrated.
>
>
>
> I looked at adding it, but I ran into an issue.  The value is a
>
> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)
>
> and that means the data has to be void *, but to transfer it it must be a
> uint8_t *.
> The pointer property seems to be a cheap hack, I'm not sure what it will
> take
> to fix it.

The data provided by the caller is only the initialization
data. So I think the device should create its own memory
to copy that init data into, and migrate that ram, not
wherever the initialization data lives. (Currently
this "copy the data into our own ram" happens in the
smbus_eeprom_init() wrapper, but we should do it in the
device realize function instead.)

Also there seem to be unresolved questions about what happens
on reset -- should the EEPROM revert back to the initial
contents? We don't do that at the moment, but this breaks
the usual QEMU pattern that reset is as if you'd just
completely restarted QEMU.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Corey Minyard]

On 11/9/18 9:02 AM, Peter Maydell wrote:
> On 9 November 2018 at 14:56, Corey Minyard <minyard@acm.org> wrote:
>> On 11/8/18 12:03 PM, Peter Maydell wrote:
>>> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote:
>>>> On 11/8/18 8:08 AM, Peter Maydell wrote:
>>>>> This doesn't do anything for migration of the actual data contents.
>>>>> The current users of this device (hw/arm/aspeed.c and the
>>>>> smbus_eeprom_init() function) doesn't do anything
>>>>> to migrate the contents. For that matter, "user of the device
>>>>> passes a pointer to a random lump of memory via a device property"
>>>>> is a bit funky as an interface. The device should allocate its
>>>>> own memory and migrate it, and the user should just pass the
>>>>> initial required contents (default being "zero-initialize",
>>>>> which is what everybody except the mips_fulong2e, mips_malta
>>>>> and sam460ex want).
>>>> I debated on this, and it depends on what the eeprom is used for.  If
>>>> it's a DRAM eeprom, it shouldn't need to be transferred.
>>> It's guest-visible data; the guest can write it and read it back.
>>> So it needs to be migrated. Otherwise behaviour after migration
>>> will not be the same as if the guest had never migrated.
>>
>>
>> I looked at adding it, but I ran into an issue.  The value is a
>>
>> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data)
>>
>> and that means the data has to be void *, but to transfer it it must be a
>> uint8_t *.
>> The pointer property seems to be a cheap hack, I'm not sure what it will
>> take
>> to fix it.
> The data provided by the caller is only the initialization
> data. So I think the device should create its own memory
> to copy that init data into, and migrate that ram, not
> wherever the initialization data lives. (Currently
> this "copy the data into our own ram" happens in the
> smbus_eeprom_init() wrapper, but we should do it in the
> device realize function instead.)

That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
I don't know the value of creating a properly like this, since the user
can't set it and can't see it.  Plus the comments in the code say that
it should be gotten rid of at some point.

But if I store off the initialization data and keep the actual data in
a separate memory created by the realize function, that should work
find.


>
> Also there seem to be unresolved questions about what happens
> on reset -- should the EEPROM revert back to the initial
> contents? We don't do that at the moment, but this breaks
> the usual QEMU pattern that reset is as if you'd just
> completely restarted QEMU.

I would consider this part of the hardware, like data on a disk drive,
so it seem better to leave it alone after a reset.  But it's not quite
the same.  So I'm not sure.

Thanks,

-corey


> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 9 November 2018 at 17:19, Corey Minyard <cminyard@mvista.com> wrote:
> On 11/9/18 9:02 AM, Peter Maydell wrote:
>> The data provided by the caller is only the initialization
>> data. So I think the device should create its own memory
>> to copy that init data into, and migrate that ram, not
>> wherever the initialization data lives. (Currently
>> this "copy the data into our own ram" happens in the
>> smbus_eeprom_init() wrapper, but we should do it in the
>> device realize function instead.)
>
>
> That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
> I don't know the value of creating a properly like this, since the user
> can't set it and can't see it.  Plus the comments in the code say that
> it should be gotten rid of at some point.
>
> But if I store off the initialization data and keep the actual data in
> a separate memory created by the realize function, that should work
> find.

Well, you still have to pass the init data to the device
somehow, so I think a pointer property is not a terrible
way to do that.

>> Also there seem to be unresolved questions about what happens
>> on reset -- should the EEPROM revert back to the initial
>> contents? We don't do that at the moment, but this breaks
>> the usual QEMU pattern that reset is as if you'd just
>> completely restarted QEMU.
>
>
> I would consider this part of the hardware, like data on a disk drive,
> so it seem better to leave it alone after a reset.  But it's not quite
> the same.  So I'm not sure.

That would require us to support backing it properly with a block
device, like the pflash flash devices, I think. (This would
be the long term way to be able to dump the pointer property,
in favour of a block backend property.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 9 November 2018 at 17:19, Corey Minyard <cminyard@mvista.com> wrote:
> > On 11/9/18 9:02 AM, Peter Maydell wrote:
> >> The data provided by the caller is only the initialization
> >> data. So I think the device should create its own memory
> >> to copy that init data into, and migrate that ram, not
> >> wherever the initialization data lives. (Currently
> >> this "copy the data into our own ram" happens in the
> >> smbus_eeprom_init() wrapper, but we should do it in the
> >> device realize function instead.)
> >
> >
> > That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"?
> > I don't know the value of creating a properly like this, since the user
> > can't set it and can't see it.  Plus the comments in the code say that
> > it should be gotten rid of at some point.
> >
> > But if I store off the initialization data and keep the actual data in
> > a separate memory created by the realize function, that should work
> > find.
> 
> Well, you still have to pass the init data to the device
> somehow, so I think a pointer property is not a terrible
> way to do that.
> 
> >> Also there seem to be unresolved questions about what happens
> >> on reset -- should the EEPROM revert back to the initial
> >> contents? We don't do that at the moment, but this breaks
> >> the usual QEMU pattern that reset is as if you'd just
> >> completely restarted QEMU.
> >
> >
> > I would consider this part of the hardware, like data on a disk drive,
> > so it seem better to leave it alone after a reset.  But it's not quite
> > the same.  So I'm not sure.
> 
> That would require us to support backing it properly with a block
> device, like the pflash flash devices, I think. (This would
> be the long term way to be able to dump the pointer property,
> in favour of a block backend property.)

There's a number of separate questions:

a) Can the guest write to the EEPROM or are we just treating it as a
ROM?
b) If a guest writes to the EEPROM and then resets does the data stay
there [I'd expect so, it's an EEPROM]
c) It the guest writes to the EEPROM and then quits qemu and restarts
does the data stay there?
d) If the guest is migrated does it keep the data it's written - I'd say
it must because otherwise it would get confused.

(c) is where it starts looking like a pflash - which does get a bit
messy.

Dave


> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Peter Maydell]

On 12 November 2018 at 17:38, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> That would require us to support backing it properly with a block
>> device, like the pflash flash devices, I think. (This would
>> be the long term way to be able to dump the pointer property,
>> in favour of a block backend property.)
>
> There's a number of separate questions:
>
> a) Can the guest write to the EEPROM or are we just treating it as a
> ROM?
> b) If a guest writes to the EEPROM and then resets does the data stay
> there [I'd expect so, it's an EEPROM]
> c) It the guest writes to the EEPROM and then quits qemu and restarts
> does the data stay there?
> d) If the guest is migrated does it keep the data it's written - I'd say
> it must because otherwise it would get confused.
>
> (c) is where it starts looking like a pflash - which does get a bit
> messy.

My view is that the answers to b and c should always be the same:
resetting QEMU with a system reset should look the same as if
you stopped QEMU and restarted it. (Our only kind of system
reset is an emulation of a hard powercycle.)

In this case, the answer to "a" is currently "yes, it can write to it",
though whether any guest takes advantage of that is a different
question.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote:
> > On 11/8/18 8:08 AM, Peter Maydell wrote:
> >> This doesn't do anything for migration of the actual data contents.
> >> The current users of this device (hw/arm/aspeed.c and the
> >> smbus_eeprom_init() function) doesn't do anything
> >> to migrate the contents. For that matter, "user of the device
> >> passes a pointer to a random lump of memory via a device property"
> >> is a bit funky as an interface. The device should allocate its
> >> own memory and migrate it, and the user should just pass the
> >> initial required contents (default being "zero-initialize",
> >> which is what everybody except the mips_fulong2e, mips_malta
> >> and sam460ex want).
> 
> > I debated on this, and it depends on what the eeprom is used for.  If
> > it's a DRAM eeprom, it shouldn't need to be transferred.
> 
> It's guest-visible data; the guest can write it and read it back.
> So it needs to be migrated. Otherwise behaviour after migration
> will not be the same as if the guest had never migrated.
> 
> >> Does this also break migration from an old QEMU to a new one?
> >> (For Aspeed that's probably ok, but we should flag it up in the
> >> commit message if so. x86 uses need care...)
> >>
> > There is no transfer before, so I don't see why it would break anything.
> > Am I missing something?
> 
> I forget what the behaviour is where the source QEMU didn't
> have a vmstate at all but the destination QEMU does expect
> one. David can remind me...

If it's a separate device then you tend to get away with it - there's no
check that all devices receive their state, so it should work.
This does break backwards migration though - migrating to an older
qemu with the existing machine type will complain it's received a 
device which it doesn't understand.
You should be able to add a .needed to the device
so it's only sent for new machine types.
(Which is what I said in August, when I also asked about the data)

Dave

> thanks
> - PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK