Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 105 insertions(+), 5 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 32223f6..b682a95 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -65,6 +65,7 @@
 
 #define MMIO_BASE_OFFSET 0x3e00000
 #define MMIO_SIZE 0x200000
+#define DC_PALETTE_ENTRIES (0x400 * 3)
 
 /* SM501 register definitions taken from "linux/include/linux/sm501-regs.h" */
 
@@ -491,7 +492,7 @@ typedef struct SM501State {
     uint32_t uart0_mcr;
     uint32_t uart0_scr;
 
-    uint8_t dc_palette[0x400 * 3];
+    uint8_t dc_palette[DC_PALETTE_ENTRIES];
 
     uint32_t dc_panel_control;
     uint32_t dc_panel_panning_control;
@@ -1537,16 +1538,32 @@ static const GraphicHwOps sm501_ops = {
     .gfx_update  = sm501_update_display,
 };
 
+static void sm501_reset(void *p)
+{
+    SM501State *s = p;
+
+    s->system_control = 0x00100000; /* 2D engine FIFO empty */
+    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
+    s->gpio_31_0_control = 0;
+    s->gpio_63_32_control = 0;
+    s->dram_control = 0;
+    s->arbitration_control = 0x05146732;
+    s->irq_mask = 0;
+    s->misc_timing = 0;
+    s->power_mode_control = 0;
+    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
+    s->dc_video_control = 0;
+    s->dc_crt_control = 0x00010000;
+    s->twoD_control = 0;
+}
+
 static void sm501_init(SM501State *s, DeviceState *dev,
                        uint32_t local_mem_bytes)
 {
     s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
     SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
                   s->local_mem_size_index);
-    s->system_control = 0x00100000; /* 2D engine FIFO empty */
-    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
-    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
-    s->dc_crt_control = 0x00010000;
+    qemu_register_reset(sm501_reset, s);
 
     /* local memory */
     memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",
@@ -1577,6 +1594,77 @@ static void sm501_init(SM501State *s, DeviceState *dev,
     s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
 }
 
+static const VMStateDescription vmstate_sm501_regs = {
+    .name = "sm501-regs",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(system_control, SM501State),
+        VMSTATE_UINT32(misc_control, SM501State),
+        VMSTATE_UINT32(gpio_31_0_control, SM501State),
+        VMSTATE_UINT32(gpio_63_32_control, SM501State),
+        VMSTATE_UINT32(dram_control, SM501State),
+        VMSTATE_UINT32(arbitration_control, SM501State),
+        VMSTATE_UINT32(irq_mask, SM501State),
+        VMSTATE_UINT32(misc_timing, SM501State),
+        VMSTATE_UINT32(power_mode_control, SM501State),
+        VMSTATE_UINT32(uart0_ier, SM501State),
+        VMSTATE_UINT32(uart0_lcr, SM501State),
+        VMSTATE_UINT32(uart0_mcr, SM501State),
+        VMSTATE_UINT32(uart0_scr, SM501State),
+        VMSTATE_UINT8_ARRAY(dc_palette, SM501State, DC_PALETTE_ENTRIES),
+        VMSTATE_UINT32(dc_panel_control, SM501State),
+        VMSTATE_UINT32(dc_panel_panning_control, SM501State),
+        VMSTATE_UINT32(dc_panel_fb_addr, SM501State),
+        VMSTATE_UINT32(dc_panel_fb_offset, SM501State),
+        VMSTATE_UINT32(dc_panel_fb_width, SM501State),
+        VMSTATE_UINT32(dc_panel_fb_height, SM501State),
+        VMSTATE_UINT32(dc_panel_tl_location, SM501State),
+        VMSTATE_UINT32(dc_panel_br_location, SM501State),
+        VMSTATE_UINT32(dc_panel_h_total, SM501State),
+        VMSTATE_UINT32(dc_panel_h_sync, SM501State),
+        VMSTATE_UINT32(dc_panel_v_total, SM501State),
+        VMSTATE_UINT32(dc_panel_v_sync, SM501State),
+        VMSTATE_UINT32(dc_panel_hwc_addr, SM501State),
+        VMSTATE_UINT32(dc_panel_hwc_location, SM501State),
+        VMSTATE_UINT32(dc_panel_hwc_color_1_2, SM501State),
+        VMSTATE_UINT32(dc_panel_hwc_color_3, SM501State),
+        VMSTATE_UINT32(dc_video_control, SM501State),
+        VMSTATE_UINT32(dc_crt_control, SM501State),
+        VMSTATE_UINT32(dc_crt_fb_addr, SM501State),
+        VMSTATE_UINT32(dc_crt_fb_offset, SM501State),
+        VMSTATE_UINT32(dc_crt_h_total, SM501State),
+        VMSTATE_UINT32(dc_crt_h_sync, SM501State),
+        VMSTATE_UINT32(dc_crt_v_total, SM501State),
+        VMSTATE_UINT32(dc_crt_v_sync, SM501State),
+        VMSTATE_UINT32(dc_crt_hwc_addr, SM501State),
+        VMSTATE_UINT32(dc_crt_hwc_location, SM501State),
+        VMSTATE_UINT32(dc_crt_hwc_color_1_2, SM501State),
+        VMSTATE_UINT32(dc_crt_hwc_color_3, SM501State),
+        VMSTATE_UINT32(twoD_source, SM501State),
+        VMSTATE_UINT32(twoD_destination, SM501State),
+        VMSTATE_UINT32(twoD_dimension, SM501State),
+        VMSTATE_UINT32(twoD_control, SM501State),
+        VMSTATE_UINT32(twoD_pitch, SM501State),
+        VMSTATE_UINT32(twoD_foreground, SM501State),
+        VMSTATE_UINT32(twoD_background, SM501State),
+        VMSTATE_UINT32(twoD_stretch, SM501State),
+        VMSTATE_UINT32(twoD_color_compare, SM501State),
+        VMSTATE_UINT32(twoD_color_compare_mask, SM501State),
+        VMSTATE_UINT32(twoD_mask, SM501State),
+        VMSTATE_UINT32(twoD_clip_tl, SM501State),
+        VMSTATE_UINT32(twoD_clip_br, SM501State),
+        VMSTATE_UINT32(twoD_mono_pattern_low, SM501State),
+        VMSTATE_UINT32(twoD_mono_pattern_high, SM501State),
+        VMSTATE_UINT32(twoD_window_width, SM501State),
+        VMSTATE_UINT32(twoD_source_base, SM501State),
+        VMSTATE_UINT32(twoD_destination_base, SM501State),
+        VMSTATE_UINT32(twoD_alpha, SM501State),
+        VMSTATE_UINT32(twoD_wrap, SM501State),
+        VMSTATE_END_OF_LIST()
+     }
+};
+
 #define TYPE_SYSBUS_SM501 "sysbus-sm501"
 #define SYSBUS_SM501(obj) \
     OBJECT_CHECK(SM501SysBusState, (obj), TYPE_SYSBUS_SM501)
@@ -1673,6 +1761,17 @@ static Property sm501_pci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription vmstate_sm501 = {
+    .name = "sm501",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
+        VMSTATE_STRUCT(state, SM501PCIState, 1, vmstate_sm501_regs, SM501State),
+        VMSTATE_END_OF_LIST()
+     }
+};
+
 static void sm501_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1686,6 +1785,7 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
     dc->desc = "SM501 Display Controller";
     dc->props = sm501_pci_properties;
     dc->hotpluggable = false;
+    dc->vmsd = &vmstate_sm501;
 }
 
 static const TypeInfo sm501_pci_info = {
-- 
2.7.4

On 25 February 2017 at 23:53, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/sm501.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 105 insertions(+), 5 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 32223f6..b682a95 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -65,6 +65,7 @@
>
>  #define MMIO_BASE_OFFSET 0x3e00000
>  #define MMIO_SIZE 0x200000
> +#define DC_PALETTE_ENTRIES (0x400 * 3)
>
>  /* SM501 register definitions taken from "linux/include/linux/sm501-regs.h" */
>
> @@ -491,7 +492,7 @@ typedef struct SM501State {
>      uint32_t uart0_mcr;
>      uint32_t uart0_scr;
>
> -    uint8_t dc_palette[0x400 * 3];
> +    uint8_t dc_palette[DC_PALETTE_ENTRIES];
>
>      uint32_t dc_panel_control;
>      uint32_t dc_panel_panning_control;
> @@ -1537,16 +1538,32 @@ static const GraphicHwOps sm501_ops = {
>      .gfx_update  = sm501_update_display,
>  };
>
> +static void sm501_reset(void *p)
> +{
> +    SM501State *s = p;
> +
> +    s->system_control = 0x00100000; /* 2D engine FIFO empty */
> +    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */

Don't forget to update this patch when you fix this reset value in patch 1.

> +    s->gpio_31_0_control = 0;
> +    s->gpio_63_32_control = 0;
> +    s->dram_control = 0;
> +    s->arbitration_control = 0x05146732;
> +    s->irq_mask = 0;
> +    s->misc_timing = 0;
> +    s->power_mode_control = 0;
> +    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
> +    s->dc_video_control = 0;
> +    s->dc_crt_control = 0x00010000;
> +    s->twoD_control = 0;
> +}
> +
>  static void sm501_init(SM501State *s, DeviceState *dev,
>                         uint32_t local_mem_bytes)
>  {
>      s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
>      SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
>                    s->local_mem_size_index);
> -    s->system_control = 0x00100000; /* 2D engine FIFO empty */
> -    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
> -    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
> -    s->dc_crt_control = 0x00010000;
> +    qemu_register_reset(sm501_reset, s);

Don't use qemu_register_reset(). Set the appropriate dc->reset
function pointers instead.

>
>      /* local memory */
>      memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",
> @@ -1577,6 +1594,77 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>      s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
>  }
>
> +static const VMStateDescription vmstate_sm501_regs = {
> +    .name = "sm501-regs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(system_control, SM501State),
> +        VMSTATE_UINT32(misc_control, SM501State),
> +        VMSTATE_UINT32(gpio_31_0_control, SM501State),
> +        VMSTATE_UINT32(gpio_63_32_control, SM501State),
> +        VMSTATE_UINT32(dram_control, SM501State),
> +        VMSTATE_UINT32(arbitration_control, SM501State),
> +        VMSTATE_UINT32(irq_mask, SM501State),
> +        VMSTATE_UINT32(misc_timing, SM501State),
> +        VMSTATE_UINT32(power_mode_control, SM501State),
> +        VMSTATE_UINT32(uart0_ier, SM501State),
> +        VMSTATE_UINT32(uart0_lcr, SM501State),
> +        VMSTATE_UINT32(uart0_mcr, SM501State),
> +        VMSTATE_UINT32(uart0_scr, SM501State),
> +        VMSTATE_UINT8_ARRAY(dc_palette, SM501State, DC_PALETTE_ENTRIES),
> +        VMSTATE_UINT32(dc_panel_control, SM501State),
> +        VMSTATE_UINT32(dc_panel_panning_control, SM501State),
> +        VMSTATE_UINT32(dc_panel_fb_addr, SM501State),
> +        VMSTATE_UINT32(dc_panel_fb_offset, SM501State),
> +        VMSTATE_UINT32(dc_panel_fb_width, SM501State),
> +        VMSTATE_UINT32(dc_panel_fb_height, SM501State),
> +        VMSTATE_UINT32(dc_panel_tl_location, SM501State),
> +        VMSTATE_UINT32(dc_panel_br_location, SM501State),
> +        VMSTATE_UINT32(dc_panel_h_total, SM501State),
> +        VMSTATE_UINT32(dc_panel_h_sync, SM501State),
> +        VMSTATE_UINT32(dc_panel_v_total, SM501State),
> +        VMSTATE_UINT32(dc_panel_v_sync, SM501State),
> +        VMSTATE_UINT32(dc_panel_hwc_addr, SM501State),
> +        VMSTATE_UINT32(dc_panel_hwc_location, SM501State),
> +        VMSTATE_UINT32(dc_panel_hwc_color_1_2, SM501State),
> +        VMSTATE_UINT32(dc_panel_hwc_color_3, SM501State),
> +        VMSTATE_UINT32(dc_video_control, SM501State),
> +        VMSTATE_UINT32(dc_crt_control, SM501State),
> +        VMSTATE_UINT32(dc_crt_fb_addr, SM501State),
> +        VMSTATE_UINT32(dc_crt_fb_offset, SM501State),
> +        VMSTATE_UINT32(dc_crt_h_total, SM501State),
> +        VMSTATE_UINT32(dc_crt_h_sync, SM501State),
> +        VMSTATE_UINT32(dc_crt_v_total, SM501State),
> +        VMSTATE_UINT32(dc_crt_v_sync, SM501State),
> +        VMSTATE_UINT32(dc_crt_hwc_addr, SM501State),
> +        VMSTATE_UINT32(dc_crt_hwc_location, SM501State),
> +        VMSTATE_UINT32(dc_crt_hwc_color_1_2, SM501State),
> +        VMSTATE_UINT32(dc_crt_hwc_color_3, SM501State),
> +        VMSTATE_UINT32(twoD_source, SM501State),
> +        VMSTATE_UINT32(twoD_destination, SM501State),
> +        VMSTATE_UINT32(twoD_dimension, SM501State),
> +        VMSTATE_UINT32(twoD_control, SM501State),
> +        VMSTATE_UINT32(twoD_pitch, SM501State),
> +        VMSTATE_UINT32(twoD_foreground, SM501State),
> +        VMSTATE_UINT32(twoD_background, SM501State),
> +        VMSTATE_UINT32(twoD_stretch, SM501State),
> +        VMSTATE_UINT32(twoD_color_compare, SM501State),
> +        VMSTATE_UINT32(twoD_color_compare_mask, SM501State),
> +        VMSTATE_UINT32(twoD_mask, SM501State),
> +        VMSTATE_UINT32(twoD_clip_tl, SM501State),
> +        VMSTATE_UINT32(twoD_clip_br, SM501State),
> +        VMSTATE_UINT32(twoD_mono_pattern_low, SM501State),
> +        VMSTATE_UINT32(twoD_mono_pattern_high, SM501State),
> +        VMSTATE_UINT32(twoD_window_width, SM501State),
> +        VMSTATE_UINT32(twoD_source_base, SM501State),
> +        VMSTATE_UINT32(twoD_destination_base, SM501State),
> +        VMSTATE_UINT32(twoD_alpha, SM501State),
> +        VMSTATE_UINT32(twoD_wrap, SM501State),
> +        VMSTATE_END_OF_LIST()
> +     }
> +};

local_mem_size_index is also guest updatable state (it's some
of the bits from the SM501_DRAM_CONTROL register), so we need
to migrate it as well.

> +
>  #define TYPE_SYSBUS_SM501 "sysbus-sm501"
>  #define SYSBUS_SM501(obj) \
>      OBJECT_CHECK(SM501SysBusState, (obj), TYPE_SYSBUS_SM501)
> @@ -1673,6 +1761,17 @@ static Property sm501_pci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static const VMStateDescription vmstate_sm501 = {
> +    .name = "sm501",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
> +        VMSTATE_STRUCT(state, SM501PCIState, 1, vmstate_sm501_regs, SM501State),
> +        VMSTATE_END_OF_LIST()
> +     }
> +};
> +
>  static void sm501_pci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1686,6 +1785,7 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>      dc->desc = "SM501 Display Controller";
>      dc->props = sm501_pci_properties;
>      dc->hotpluggable = false;
> +    dc->vmsd = &vmstate_sm501;
>  }
>
>  static const TypeInfo sm501_pci_info = {
> --
> 2.7.4

What about reset and vmstate for the sysbus device?

Consider putting reset and vmstate in separate patches.

thanks
-- PMM

On Thu, 2 Mar 2017, Peter Maydell wrote:
> On 25 February 2017 at 23:53, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/sm501.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 105 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 32223f6..b682a95 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -65,6 +65,7 @@
>>
>>  #define MMIO_BASE_OFFSET 0x3e00000
>>  #define MMIO_SIZE 0x200000
>> +#define DC_PALETTE_ENTRIES (0x400 * 3)
>>
>>  /* SM501 register definitions taken from "linux/include/linux/sm501-regs.h" */
>>
>> @@ -491,7 +492,7 @@ typedef struct SM501State {
>>      uint32_t uart0_mcr;
>>      uint32_t uart0_scr;
>>
>> -    uint8_t dc_palette[0x400 * 3];
>> +    uint8_t dc_palette[DC_PALETTE_ENTRIES];
>>
>>      uint32_t dc_panel_control;
>>      uint32_t dc_panel_panning_control;
>> @@ -1537,16 +1538,32 @@ static const GraphicHwOps sm501_ops = {
>>      .gfx_update  = sm501_update_display,
>>  };
>>
>> +static void sm501_reset(void *p)
>> +{
>> +    SM501State *s = p;
>> +
>> +    s->system_control = 0x00100000; /* 2D engine FIFO empty */
>> +    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
>
> Don't forget to update this patch when you fix this reset value in patch 1.
>
>> +    s->gpio_31_0_control = 0;
>> +    s->gpio_63_32_control = 0;
>> +    s->dram_control = 0;
>> +    s->arbitration_control = 0x05146732;
>> +    s->irq_mask = 0;
>> +    s->misc_timing = 0;
>> +    s->power_mode_control = 0;
>> +    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>> +    s->dc_video_control = 0;
>> +    s->dc_crt_control = 0x00010000;
>> +    s->twoD_control = 0;
>> +}
>> +
>>  static void sm501_init(SM501State *s, DeviceState *dev,
>>                         uint32_t local_mem_bytes)
>>  {
>>      s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
>>      SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
>>                    s->local_mem_size_index);
>> -    s->system_control = 0x00100000; /* 2D engine FIFO empty */
>> -    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
>> -    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>> -    s->dc_crt_control = 0x00010000;
>> +    qemu_register_reset(sm501_reset, s);
>
> Don't use qemu_register_reset(). Set the appropriate dc->reset
> function pointers instead.

Any reason for that? This way I could save two more boilerplate functions 
because I could define reset function once, otherwise I'd need two 
versions taking sysbus and pci states just to extract the SM501State 
function and call this function. Do you still think I should do that 
instead?

>>      /* local memory */
>>      memory_region_init_ram(&s->local_mem_region, OBJECT(dev), "sm501.local",
>> @@ -1577,6 +1594,77 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>>      s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
>>  }
>>
>> +static const VMStateDescription vmstate_sm501_regs = {
>> +    .name = "sm501-regs",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(system_control, SM501State),
>> +        VMSTATE_UINT32(misc_control, SM501State),
>> +        VMSTATE_UINT32(gpio_31_0_control, SM501State),
>> +        VMSTATE_UINT32(gpio_63_32_control, SM501State),
>> +        VMSTATE_UINT32(dram_control, SM501State),
>> +        VMSTATE_UINT32(arbitration_control, SM501State),
>> +        VMSTATE_UINT32(irq_mask, SM501State),
>> +        VMSTATE_UINT32(misc_timing, SM501State),
>> +        VMSTATE_UINT32(power_mode_control, SM501State),
>> +        VMSTATE_UINT32(uart0_ier, SM501State),
>> +        VMSTATE_UINT32(uart0_lcr, SM501State),
>> +        VMSTATE_UINT32(uart0_mcr, SM501State),
>> +        VMSTATE_UINT32(uart0_scr, SM501State),
>> +        VMSTATE_UINT8_ARRAY(dc_palette, SM501State, DC_PALETTE_ENTRIES),
>> +        VMSTATE_UINT32(dc_panel_control, SM501State),
>> +        VMSTATE_UINT32(dc_panel_panning_control, SM501State),
>> +        VMSTATE_UINT32(dc_panel_fb_addr, SM501State),
>> +        VMSTATE_UINT32(dc_panel_fb_offset, SM501State),
>> +        VMSTATE_UINT32(dc_panel_fb_width, SM501State),
>> +        VMSTATE_UINT32(dc_panel_fb_height, SM501State),
>> +        VMSTATE_UINT32(dc_panel_tl_location, SM501State),
>> +        VMSTATE_UINT32(dc_panel_br_location, SM501State),
>> +        VMSTATE_UINT32(dc_panel_h_total, SM501State),
>> +        VMSTATE_UINT32(dc_panel_h_sync, SM501State),
>> +        VMSTATE_UINT32(dc_panel_v_total, SM501State),
>> +        VMSTATE_UINT32(dc_panel_v_sync, SM501State),
>> +        VMSTATE_UINT32(dc_panel_hwc_addr, SM501State),
>> +        VMSTATE_UINT32(dc_panel_hwc_location, SM501State),
>> +        VMSTATE_UINT32(dc_panel_hwc_color_1_2, SM501State),
>> +        VMSTATE_UINT32(dc_panel_hwc_color_3, SM501State),
>> +        VMSTATE_UINT32(dc_video_control, SM501State),
>> +        VMSTATE_UINT32(dc_crt_control, SM501State),
>> +        VMSTATE_UINT32(dc_crt_fb_addr, SM501State),
>> +        VMSTATE_UINT32(dc_crt_fb_offset, SM501State),
>> +        VMSTATE_UINT32(dc_crt_h_total, SM501State),
>> +        VMSTATE_UINT32(dc_crt_h_sync, SM501State),
>> +        VMSTATE_UINT32(dc_crt_v_total, SM501State),
>> +        VMSTATE_UINT32(dc_crt_v_sync, SM501State),
>> +        VMSTATE_UINT32(dc_crt_hwc_addr, SM501State),
>> +        VMSTATE_UINT32(dc_crt_hwc_location, SM501State),
>> +        VMSTATE_UINT32(dc_crt_hwc_color_1_2, SM501State),
>> +        VMSTATE_UINT32(dc_crt_hwc_color_3, SM501State),
>> +        VMSTATE_UINT32(twoD_source, SM501State),
>> +        VMSTATE_UINT32(twoD_destination, SM501State),
>> +        VMSTATE_UINT32(twoD_dimension, SM501State),
>> +        VMSTATE_UINT32(twoD_control, SM501State),
>> +        VMSTATE_UINT32(twoD_pitch, SM501State),
>> +        VMSTATE_UINT32(twoD_foreground, SM501State),
>> +        VMSTATE_UINT32(twoD_background, SM501State),
>> +        VMSTATE_UINT32(twoD_stretch, SM501State),
>> +        VMSTATE_UINT32(twoD_color_compare, SM501State),
>> +        VMSTATE_UINT32(twoD_color_compare_mask, SM501State),
>> +        VMSTATE_UINT32(twoD_mask, SM501State),
>> +        VMSTATE_UINT32(twoD_clip_tl, SM501State),
>> +        VMSTATE_UINT32(twoD_clip_br, SM501State),
>> +        VMSTATE_UINT32(twoD_mono_pattern_low, SM501State),
>> +        VMSTATE_UINT32(twoD_mono_pattern_high, SM501State),
>> +        VMSTATE_UINT32(twoD_window_width, SM501State),
>> +        VMSTATE_UINT32(twoD_source_base, SM501State),
>> +        VMSTATE_UINT32(twoD_destination_base, SM501State),
>> +        VMSTATE_UINT32(twoD_alpha, SM501State),
>> +        VMSTATE_UINT32(twoD_wrap, SM501State),
>> +        VMSTATE_END_OF_LIST()
>> +     }
>> +};
>
> local_mem_size_index is also guest updatable state (it's some
> of the bits from the SM501_DRAM_CONTROL register), so we need
> to migrate it as well.

OK, thanks for catching this.

>> +
>>  #define TYPE_SYSBUS_SM501 "sysbus-sm501"
>>  #define SYSBUS_SM501(obj) \
>>      OBJECT_CHECK(SM501SysBusState, (obj), TYPE_SYSBUS_SM501)
>> @@ -1673,6 +1761,17 @@ static Property sm501_pci_properties[] = {
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> +static const VMStateDescription vmstate_sm501 = {
>> +    .name = "sm501",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
>> +        VMSTATE_STRUCT(state, SM501PCIState, 1, vmstate_sm501_regs, SM501State),
>> +        VMSTATE_END_OF_LIST()
>> +     }
>> +};
>> +
>>  static void sm501_pci_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -1686,6 +1785,7 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>>      dc->desc = "SM501 Display Controller";
>>      dc->props = sm501_pci_properties;
>>      dc->hotpluggable = false;
>> +    dc->vmsd = &vmstate_sm501;
>>  }
>>
>>  static const TypeInfo sm501_pci_info = {
>> --
>> 2.7.4
>
> What about reset and vmstate for the sysbus device?
>
> Consider putting reset and vmstate in separate patches.

So should it be 4 patches: reset for pci, reset for sysbus, vmstate for 
pci, vmstate for sysbus or 2 patches: reset for both, vmstate for both?

>
> thanks
> -- PMM
>
>

On 2 March 2017 at 20:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>
>> Don't use qemu_register_reset(). Set the appropriate dc->reset
>> function pointers instead.
>
>
> Any reason for that? This way I could save two more boilerplate functions
> because I could define reset function once, otherwise I'd need two versions
> taking sysbus and pci states just to extract the SM501State function and
> call this function. Do you still think I should do that instead?

qemu_register_reset is a pre-QOM method for doing reset;
the standard QOM way of saying "my device has some reset
behaviour" is to set its reset method pointer.
Code calling qemu_register_reset() is generally either (a) doing
something kind of weird or (b) old device code that hasn't yet
been converted to QOM.

> So should it be 4 patches: reset for pci, reset for sysbus, vmstate for pci,
> vmstate for sysbus or 2 patches: reset for both, vmstate for both?

I think I'd go with 2 patches, since the two are going to
share the bulk of the implementation in each case.

thanks
-- PMM

On Thu, 2 Mar 2017, Peter Maydell wrote:
> On 2 March 2017 at 20:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>>
>>> Don't use qemu_register_reset(). Set the appropriate dc->reset
>>> function pointers instead.
>>
>>
>> Any reason for that? This way I could save two more boilerplate functions
>> because I could define reset function once, otherwise I'd need two versions
>> taking sysbus and pci states just to extract the SM501State function and
>> call this function. Do you still think I should do that instead?
>
> qemu_register_reset is a pre-QOM method for doing reset;
> the standard QOM way of saying "my device has some reset
> behaviour" is to set its reset method pointer.
> Code calling qemu_register_reset() is generally either (a) doing
> something kind of weird or (b) old device code that hasn't yet
> been converted to QOM.

Hmm, does having a device with both sysbus and pci versions qualify as 
weird? Does adding a comment saying that this way we don't need two more 
reset functions just to call the registered one justifies using 
qemu_register_reset() or this function is deprecated and should not be 
used and should go with the separate reset functions instead?

>> So should it be 4 patches: reset for pci, reset for sysbus, vmstate for pci,
>> vmstate for sysbus or 2 patches: reset for both, vmstate for both?
>
> I think I'd go with 2 patches, since the two are going to
> share the bulk of the implementation in each case.

OK, thanks for the explanation in previous reply as well, I'll do this 
then.

On Thu, 2 Mar 2017, BALATON Zoltan wrote:
> On Thu, 2 Mar 2017, Peter Maydell wrote:
>> On 2 March 2017 at 20:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>>> 
>>>> Don't use qemu_register_reset(). Set the appropriate dc->reset
>>>> function pointers instead.
>>> 
>>> 
>>> Any reason for that? This way I could save two more boilerplate functions
>>> because I could define reset function once, otherwise I'd need two 
>>> versions
>>> taking sysbus and pci states just to extract the SM501State function and
>>> call this function. Do you still think I should do that instead?
>> 
>> qemu_register_reset is a pre-QOM method for doing reset;
>> the standard QOM way of saying "my device has some reset
>> behaviour" is to set its reset method pointer.
>> Code calling qemu_register_reset() is generally either (a) doing
>> something kind of weird or (b) old device code that hasn't yet
>> been converted to QOM.
>
> Hmm, does having a device with both sysbus and pci versions qualify as weird? 
> Does adding a comment saying that this way we don't need two more reset 
> functions just to call the registered one justifies using 
> qemu_register_reset() or this function is deprecated and should not be used 
> and should go with the separate reset functions instead?

On second thought I may go with the separate reset functions because then 
I can also update the bus type in the misc_control (not that anything I 
know depends on it but we can get it right).

>>> So should it be 4 patches: reset for pci, reset for sysbus, vmstate for 
>>> pci,
>>> vmstate for sysbus or 2 patches: reset for both, vmstate for both?
>> 
>> I think I'd go with 2 patches, since the two are going to
>> share the bulk of the implementation in each case.
>
> OK, thanks for the explanation in previous reply as well, I'll do this then.

So I may also add the reset function already in the QOMify and Add PCI 
version patches then only one more patch is needed for the vmstate.

On 2 March 2017 at 20:55, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>
>> On 2 March 2017 at 20:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>>>
>>>>
>>>> Don't use qemu_register_reset(). Set the appropriate dc->reset
>>>> function pointers instead.
>>>
>>>
>>>
>>> Any reason for that? This way I could save two more boilerplate functions
>>> because I could define reset function once, otherwise I'd need two
>>> versions
>>> taking sysbus and pci states just to extract the SM501State function and
>>> call this function. Do you still think I should do that instead?
>>
>>
>> qemu_register_reset is a pre-QOM method for doing reset;
>> the standard QOM way of saying "my device has some reset
>> behaviour" is to set its reset method pointer.
>> Code calling qemu_register_reset() is generally either (a) doing
>> something kind of weird or (b) old device code that hasn't yet
>> been converted to QOM.
>
>
> Hmm, does having a device with both sysbus and pci versions qualify as
> weird? Does adding a comment saying that this way we don't need two more
> reset functions just to call the registered one justifies using
> qemu_register_reset() or this function is deprecated and should not be used
> and should go with the separate reset functions instead?

Just use the QOM reset method pointers. Every QOM device should have
a reset method, and that should be how it gets
reset. (PCI is special anyway, it needs to get reset when
the guest does a PCI bus reset, and that results in your
device's reset method getting called. Functions registered
with qemu_register_reset() only get called for full system reset.)

thanks
-- PMM