[Qemu-devel] [PATCH v3 1/2] pvpanic: initialization cleanup

[Qemu-devel] [PATCH v3 1/2] pvpanic: initialization cleanup

[Michael S. Tsirkin]

Avoid use of static variables: PC systems
initialize pvpanic device through pvpanic_init,
so we can simply create the fw_cfg file at that point.
This also makes it possible to skip device
creation completely if fw_cfg is not there, e.g. for xen -
so the ports it reserves are not discoverable by guests.

Also, make pvpanic_init void since callers ignore return
status anyway.

Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Paul Durrant <Paul.Durrant@citrix.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Chanes from v2:
    skip device creation completely if !fw_cfg
    make pvpanic_init void
Changes from v1:
    don't assert if !fw_cfg


 hw/misc/pvpanic.c    | 30 ++++++++++++++++--------------
 include/hw/i386/pc.h |  2 +-
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 060099b..83ed226 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -97,26 +97,28 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
 {
     ISADevice *d = ISA_DEVICE(dev);
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
-    static bool port_configured;
-    FWCfgState *fw_cfg;
 
     isa_register_ioport(d, &s->io, s->ioport);
+}
 
-    if (!port_configured) {
-        fw_cfg = fw_cfg_find();
-        if (fw_cfg) {
-            fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
-                            g_memdup(&s->ioport, sizeof(s->ioport)),
-                            sizeof(s->ioport));
-            port_configured = true;
-        }
-    }
+static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
+{
+    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
+
+    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
+                    g_memdup(&s->ioport, sizeof(s->ioport)),
+                    sizeof(s->ioport));
 }
 
-int pvpanic_init(ISABus *bus)
+void pvpanic_init(ISABus *bus)
 {
-    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
-    return 0;
+    ISADevice *dev;
+    FWCfgState *fw_cfg = fw_cfg_find();
+    if (!fw_cfg) {
+        return;
+    }
+    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
+    pvpanic_fw_cfg(dev, fw_cfg);
 }
 
 static Property pvpanic_isa_properties[] = {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ba9ba1a..458eded 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -196,7 +196,7 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 void pc_system_firmware_init(MemoryRegion *rom_memory);
 
 /* pvpanic.c */
-int pvpanic_init(ISABus *bus);
+void pvpanic_init(ISABus *bus);
 
 /* e820 types */
 #define E820_RAM        1
-- 
MST

[Qemu-devel] [PATCH v3 2/2] pvpanic: fix fwcfg for big endian hosts

[Michael S. Tsirkin]

Convert port number to little endian when
exposing it in fw cfg.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/misc/pvpanic.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 83ed226..792d8e4 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -104,10 +104,11 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
 static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
 {
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
+    uint16_t *pvpanic_port = g_malloc(sizeof(*pvpanic_port));
+    *pvpanic_port = cpu_to_le16(s->ioport);
 
-    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
-                    g_memdup(&s->ioport, sizeof(s->ioport)),
-                    sizeof(s->ioport));
+    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", pvpanic_port,
+                    sizeof(*pvpanic_port));
 }
 
 void pvpanic_init(ISABus *bus)
-- 
MST

Re: [Qemu-devel] [PATCH v3 1/2] pvpanic: initialization cleanup

[Laszlo Ersek]

On 06/19/13 17:02, Michael S. Tsirkin wrote:
> Avoid use of static variables: PC systems
> initialize pvpanic device through pvpanic_init,
> so we can simply create the fw_cfg file at that point.
> This also makes it possible to skip device
> creation completely if fw_cfg is not there, e.g. for xen -
> so the ports it reserves are not discoverable by guests.
> 
> Also, make pvpanic_init void since callers ignore return
> status anyway.
> 
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Chanes from v2:
>     skip device creation completely if !fw_cfg
>     make pvpanic_init void
> Changes from v1:
>     don't assert if !fw_cfg
> 
> 
>  hw/misc/pvpanic.c    | 30 ++++++++++++++++--------------
>  include/hw/i386/pc.h |  2 +-
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 060099b..83ed226 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -97,26 +97,28 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
>  {
>      ISADevice *d = ISA_DEVICE(dev);
>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> -    static bool port_configured;
> -    FWCfgState *fw_cfg;
>  
>      isa_register_ioport(d, &s->io, s->ioport);
> +}
>  
> -    if (!port_configured) {
> -        fw_cfg = fw_cfg_find();
> -        if (fw_cfg) {
> -            fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> -                            g_memdup(&s->ioport, sizeof(s->ioport)),
> -                            sizeof(s->ioport));
> -            port_configured = true;
> -        }
> -    }
> +static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
> +{
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> +
> +    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> +                    g_memdup(&s->ioport, sizeof(s->ioport)),
> +                    sizeof(s->ioport));
>  }
>  
> -int pvpanic_init(ISABus *bus)
> +void pvpanic_init(ISABus *bus)
>  {
> -    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
> -    return 0;
> +    ISADevice *dev;
> +    FWCfgState *fw_cfg = fw_cfg_find();
> +    if (!fw_cfg) {
> +        return;
> +    }
> +    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
> +    pvpanic_fw_cfg(dev, fw_cfg);
>  }
>  
>  static Property pvpanic_isa_properties[] = {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ba9ba1a..458eded 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -196,7 +196,7 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  void pc_system_firmware_init(MemoryRegion *rom_memory);
>  
>  /* pvpanic.c */
> -int pvpanic_init(ISABus *bus);
> +void pvpanic_init(ISABus *bus);
>  
>  /* e820 types */
>  #define E820_RAM        1
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/2] pvpanic: initialization cleanup

[Stefano Stabellini]

On Wed, 19 Jun 2013, Michael S. Tsirkin wrote:
> -int pvpanic_init(ISABus *bus)
> +void pvpanic_init(ISABus *bus)
>  {
> -    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
> -    return 0;
> +    ISADevice *dev;
> +    FWCfgState *fw_cfg = fw_cfg_find();
> +    if (!fw_cfg) {
> +        return;
> +    }
> +    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
> +    pvpanic_fw_cfg(dev, fw_cfg);
>  }

looks OK to me