[Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Andreas Färber]

Hello Markus et al.,

This series attempts to fix the -device pc87312 issues you reported.

I can't add alias properties for devices that don't get created before realize.
Therefore this involves moving code for various ISA devices, to enable us
to initialize the objects early for alias properties and realizing them
as part of the composite device once the configuration is known, also fixing
error propagation while at it. Probably needs a further iteration.

Yes, it's terribly invasive, that's why the code was as it is.
But the code movements are quite trivial, as long as no in-air conflicts occur.

A follow-up would be to respin my old ISA enabled/disabled series to allow
inactive ISADevices sitting on an ISABus.

Regards,
Andreas

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Hervé Poussineau <hpoussin@reactos.org>
Cc: qemu-ppc@nongnu.org
Cc: qemu-block@nongnu.org
Cc: John Snow <jsnow@redhat.com>

Andreas Färber (8):
  parallel: Factor out header for ISAParallelState struct
  pc87312: Create isa-parallel in-place and add alias par0-chardev
    property
  serial: Move ISASerialState to header
  pc87312: Create UARTs in-place and add alias properties
  fdb: Move FDCtrlISABus to header
  pc87312: Create FDC in-place
  ide: Move ISAIDEState to header
  pc87312: Create IDE in-place

 hw/block/fdc.c             |  87 -----------------------
 hw/char/parallel.c         |  30 +-------
 hw/char/serial-isa.c       |  12 ----
 hw/ide/internal.h          | 155 ----------------------------------------
 hw/ide/isa.c               |  13 ----
 hw/isa/pc87312.c           | 107 ++++++++++++++++------------
 hw/ppc/prep.c              |  33 +++++++++
 include/hw/block/fdc.h     |  88 +++++++++++++++++++++++
 include/hw/char/parallel.h |  62 ++++++++++++++++
 include/hw/char/serial.h   |  14 ++++
 include/hw/ide.h           | 173 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/isa/pc87312.h   |  23 +++---
 12 files changed, 442 insertions(+), 355 deletions(-)
 create mode 100644 include/hw/char/parallel.h

-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 1/8] parallel: Factor out header for ISAParallelState struct

[Andreas Färber]

To be used for embedding the device.

Add gtk-doc private/public markers for parent field.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/char/parallel.c         | 30 +---------------------
 include/hw/char/parallel.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 29 deletions(-)
 create mode 100644 include/hw/char/parallel.h

diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 4079554..fcb3764 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -24,7 +24,7 @@
  */
 #include "hw/hw.h"
 #include "sysemu/char.h"
-#include "hw/isa/isa.h"
+#include "hw/char/parallel.h"
 #include "hw/i386/pc.h"
 #include "sysemu/sysemu.h"
 
@@ -64,34 +64,6 @@
 
 #define PARA_CTR_SIGNAL (PARA_CTR_SELECT|PARA_CTR_INIT|PARA_CTR_AUTOLF|PARA_CTR_STROBE)
 
-typedef struct ParallelState {
-    MemoryRegion iomem;
-    uint8_t dataw;
-    uint8_t datar;
-    uint8_t status;
-    uint8_t control;
-    qemu_irq irq;
-    int irq_pending;
-    CharDriverState *chr;
-    int hw_driver;
-    int epp_timeout;
-    uint32_t last_read_offset; /* For debugging */
-    /* Memory-mapped interface */
-    int it_shift;
-} ParallelState;
-
-#define TYPE_ISA_PARALLEL "isa-parallel"
-#define ISA_PARALLEL(obj) \
-    OBJECT_CHECK(ISAParallelState, (obj), TYPE_ISA_PARALLEL)
-
-typedef struct ISAParallelState {
-    ISADevice parent_obj;
-
-    uint32_t index;
-    uint32_t iobase;
-    uint32_t isairq;
-    ParallelState state;
-} ISAParallelState;
 
 static void parallel_update_irq(ParallelState *s)
 {
diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
new file mode 100644
index 0000000..b20e403
--- /dev/null
+++ b/include/hw/char/parallel.h
@@ -0,0 +1,62 @@
+/*
+ * QEMU Parallel PORT emulation
+ *
+ * Copyright (c) 2003-2005 Fabrice Bellard
+ * Copyright (c) 2007 Marko Kohtala
+ * Copyright (c) 2015 SUSE Linux GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef HW_CHAR_PARALLEL_H
+#define HW_CHAR_PARALLEL_H
+
+#include "hw/isa/isa.h"
+
+typedef struct ParallelState {
+    MemoryRegion iomem;
+    uint8_t dataw;
+    uint8_t datar;
+    uint8_t status;
+    uint8_t control;
+    qemu_irq irq;
+    int irq_pending;
+    CharDriverState *chr;
+    int hw_driver;
+    int epp_timeout;
+    uint32_t last_read_offset; /* For debugging */
+    /* Memory-mapped interface */
+    int it_shift;
+} ParallelState;
+
+#define TYPE_ISA_PARALLEL "isa-parallel"
+#define ISA_PARALLEL(obj) \
+    OBJECT_CHECK(ISAParallelState, (obj), TYPE_ISA_PARALLEL)
+
+typedef struct ISAParallelState {
+    /*< private >*/
+    ISADevice parent_obj;
+    /*< public >*/
+
+    uint32_t index;
+    uint32_t iobase;
+    uint32_t isairq;
+    ParallelState state;
+} ISAParallelState;
+
+#endif
-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 2/8] pc87312: Create isa-parallel in-place and add alias par0-chardev property

[Andreas Färber]

Move the parallel_hds[] code to the PReP machine.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/isa/pc87312.c         | 25 +++++++++++++++----------
 hw/ppc/prep.c            |  9 +++++++++
 include/hw/isa/pc87312.h |  5 ++---
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
index 40a1106..ded6c01 100644
--- a/hw/isa/pc87312.c
+++ b/hw/isa/pc87312.c
@@ -268,6 +268,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     ISABus *bus;
     CharDriverState *chr;
     DriveInfo *drive;
+    Error *local_err = NULL;
     char name[5];
     int i;
 
@@ -278,20 +279,19 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     pc87312_hard_reset(s);
 
     if (is_parallel_enabled(s)) {
-        chr = parallel_hds[0];
-        if (chr == NULL) {
-            chr = qemu_chr_new("par0", "null", NULL);
-        }
-        isa = isa_create(bus, "isa-parallel");
-        d = DEVICE(isa);
-        qdev_prop_set_uint32(d, "index", 0);
+        d = DEVICE(&s->parallel);
+        qdev_set_parent_bus(d, BUS(bus));
         qdev_prop_set_uint32(d, "iobase", get_parallel_iobase(s));
         qdev_prop_set_uint32(d, "irq", get_parallel_irq(s));
-        qdev_prop_set_chr(d, "chardev", chr);
-        qdev_init_nofail(d);
-        s->parallel.dev = isa;
         trace_pc87312_info_parallel(get_parallel_iobase(s),
                                     get_parallel_irq(s));
+        object_property_set_bool(OBJECT(&s->parallel), true, "realized",
+                                 &local_err);
+        object_unref(OBJECT(&s->parallel));
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     for (i = 0; i < 2; i++) {
@@ -351,6 +351,11 @@ static void pc87312_initfn(Object *obj)
     PC87312State *s = PC87312(obj);
 
     memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312", 2);
+
+    object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PARALLEL);
+    object_property_add_alias(obj, "par0-chardev",
+                              OBJECT(&s->parallel), "chardev", &error_abort);
+    qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0);
 }
 
 static const VMStateDescription vmstate_pc87312 = {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 7f52662..1dfa051 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -40,6 +40,7 @@
 #include "hw/isa/pc87312.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/char.h"
 #include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 #include "elf.h"
@@ -528,6 +529,7 @@ static void ppc_prep_init(MachineState *machine)
     PCIDevice *pci;
     ISABus *isa_bus;
     ISADevice *isa;
+    CharDriverState *chr;
     qemu_irq *cpu_exit_irq;
     int ppc_boot_device;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -639,6 +641,13 @@ static void ppc_prep_init(MachineState *machine)
     /* Super I/O (parallel + serial ports) */
     isa = isa_create(isa_bus, TYPE_PC87312);
     dev = DEVICE(isa);
+    chr = parallel_hds[0];
+    if (chr == NULL) {
+        chr = qemu_chr_new("par0", "null", NULL);
+    }
+    if (chr != NULL) {
+        qdev_prop_set_chr(dev, "par0-chardev", chr);
+    }
     qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
     qdev_init_nofail(dev);
 
diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
index bf74470..b473b3b 100644
--- a/include/hw/isa/pc87312.h
+++ b/include/hw/isa/pc87312.h
@@ -26,6 +26,7 @@
 #define QEMU_PC87312_H
 
 #include "hw/isa/isa.h"
+#include "hw/char/parallel.h"
 
 
 #define TYPE_PC87312 "pc87312"
@@ -37,9 +38,7 @@ typedef struct PC87312State {
     uint32_t iobase;
     uint8_t config; /* initial configuration */
 
-    struct {
-        ISADevice *dev;
-    } parallel;
+    ISAParallelState parallel;
 
     struct {
         ISADevice *dev;
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC for-2.3? 2/8] pc87312: Create isa-parallel in-place and add alias par0-chardev property

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Move the parallel_hds[] code to the PReP machine.

Could use a brief explanation *why* we move it.

> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/isa/pc87312.c         | 25 +++++++++++++++----------
>  hw/ppc/prep.c            |  9 +++++++++
>  include/hw/isa/pc87312.h |  5 ++---
>  3 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
> index 40a1106..ded6c01 100644
> --- a/hw/isa/pc87312.c
> +++ b/hw/isa/pc87312.c
> @@ -268,6 +268,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>      ISABus *bus;
>      CharDriverState *chr;
>      DriveInfo *drive;
> +    Error *local_err = NULL;
>      char name[5];
>      int i;
>  
> @@ -278,20 +279,19 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>      pc87312_hard_reset(s);
>  
>      if (is_parallel_enabled(s)) {
> -        chr = parallel_hds[0];
> -        if (chr == NULL) {
> -            chr = qemu_chr_new("par0", "null", NULL);
> -        }
> -        isa = isa_create(bus, "isa-parallel");
> -        d = DEVICE(isa);
> -        qdev_prop_set_uint32(d, "index", 0);
> +        d = DEVICE(&s->parallel);
> +        qdev_set_parent_bus(d, BUS(bus));
>          qdev_prop_set_uint32(d, "iobase", get_parallel_iobase(s));
>          qdev_prop_set_uint32(d, "irq", get_parallel_irq(s));
> -        qdev_prop_set_chr(d, "chardev", chr);
> -        qdev_init_nofail(d);
> -        s->parallel.dev = isa;
>          trace_pc87312_info_parallel(get_parallel_iobase(s),
>                                      get_parallel_irq(s));
> +        object_property_set_bool(OBJECT(&s->parallel), true, "realized",
> +                                 &local_err);
> +        object_unref(OBJECT(&s->parallel));

Ignorant question: why unref here?

> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>  
>      for (i = 0; i < 2; i++) {
> @@ -351,6 +351,11 @@ static void pc87312_initfn(Object *obj)
>      PC87312State *s = PC87312(obj);
>  
>      memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312", 2);
> +
> +    object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PARALLEL);
> +    object_property_add_alias(obj, "par0-chardev",
> +                              OBJECT(&s->parallel), "chardev", &error_abort);
> +    qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0);
>  }
>  
>  static const VMStateDescription vmstate_pc87312 = {
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 7f52662..1dfa051 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -40,6 +40,7 @@
>  #include "hw/isa/pc87312.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/char.h"
>  #include "sysemu/qtest.h"
>  #include "exec/address-spaces.h"
>  #include "elf.h"
> @@ -528,6 +529,7 @@ static void ppc_prep_init(MachineState *machine)
>      PCIDevice *pci;
>      ISABus *isa_bus;
>      ISADevice *isa;
> +    CharDriverState *chr;
>      qemu_irq *cpu_exit_irq;
>      int ppc_boot_device;
>      DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> @@ -639,6 +641,13 @@ static void ppc_prep_init(MachineState *machine)
>      /* Super I/O (parallel + serial ports) */
>      isa = isa_create(isa_bus, TYPE_PC87312);
>      dev = DEVICE(isa);
> +    chr = parallel_hds[0];
> +    if (chr == NULL) {
> +        chr = qemu_chr_new("par0", "null", NULL);
> +    }
> +    if (chr != NULL) {
> +        qdev_prop_set_chr(dev, "par0-chardev", chr);
> +    }
>      qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
>      qdev_init_nofail(dev);
>  

Unlike the old code, this checks for qemu_chr_new() failure.  Can this
happen?

If it does, property "par0-chardev" remains null.
parallel_isa_realizefn() will then fail.  Worth a comment, I think.

I don't like boards creating backends with IDs on their own as long as
we don't have a ID namespace reserved for the system.  However, this is
preexisting, so let's not worry about it now.

> diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
> index bf74470..b473b3b 100644
> --- a/include/hw/isa/pc87312.h
> +++ b/include/hw/isa/pc87312.h
> @@ -26,6 +26,7 @@
>  #define QEMU_PC87312_H
>  
>  #include "hw/isa/isa.h"
> +#include "hw/char/parallel.h"
>  
>  
>  #define TYPE_PC87312 "pc87312"
> @@ -37,9 +38,7 @@ typedef struct PC87312State {
>      uint32_t iobase;
>      uint8_t config; /* initial configuration */
>  
> -    struct {
> -        ISADevice *dev;
> -    } parallel;
> +    ISAParallelState parallel;
>  
>      struct {
>          ISADevice *dev;

Re: [Qemu-devel] [PATCH RFC for-2.3? 2/8] pc87312: Create isa-parallel in-place and add alias par0-chardev property

[Andreas Färber]

Am 30.03.2015 um 16:12 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Move the parallel_hds[] code to the PReP machine.
> 
> Could use a brief explanation *why* we move it.

Yeah, the cover letter did say it probably needs a further iteration - I
had bad mobile Internet connection and was in a hurry.

>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  hw/isa/pc87312.c         | 25 +++++++++++++++----------
>>  hw/ppc/prep.c            |  9 +++++++++
>>  include/hw/isa/pc87312.h |  5 ++---
>>  3 files changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
>> index 40a1106..ded6c01 100644
>> --- a/hw/isa/pc87312.c
>> +++ b/hw/isa/pc87312.c
>> @@ -268,6 +268,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>>      ISABus *bus;
>>      CharDriverState *chr;
>>      DriveInfo *drive;
>> +    Error *local_err = NULL;
>>      char name[5];
>>      int i;
>>  
>> @@ -278,20 +279,19 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>>      pc87312_hard_reset(s);
>>  
>>      if (is_parallel_enabled(s)) {
>> -        chr = parallel_hds[0];
>> -        if (chr == NULL) {
>> -            chr = qemu_chr_new("par0", "null", NULL);
>> -        }
>> -        isa = isa_create(bus, "isa-parallel");
>> -        d = DEVICE(isa);
>> -        qdev_prop_set_uint32(d, "index", 0);
>> +        d = DEVICE(&s->parallel);
>> +        qdev_set_parent_bus(d, BUS(bus));
>>          qdev_prop_set_uint32(d, "iobase", get_parallel_iobase(s));
>>          qdev_prop_set_uint32(d, "irq", get_parallel_irq(s));
>> -        qdev_prop_set_chr(d, "chardev", chr);
>> -        qdev_init_nofail(d);
>> -        s->parallel.dev = isa;
>>          trace_pc87312_info_parallel(get_parallel_iobase(s),
>>                                      get_parallel_irq(s));
>> +        object_property_set_bool(OBJECT(&s->parallel), true, "realized",
>> +                                 &local_err);
>> +        object_unref(OBJECT(&s->parallel));
> 
> Ignorant question: why unref here?

In pair with qdev_set_parent_bus(), inlined from qdev_create().

A discussion of the exact placement recently took place in the context
of pc_new_cpu() and ICC, resulting in: "[PATCH for-next] pc: Ensure
non-zero CPU ref count after attaching to ICC bus"
Basically, we want to defer the unref until the last local usage to
avoid concurrent qom-set finalizing the object.

>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>>      }
>>  
>>      for (i = 0; i < 2; i++) {
>> @@ -351,6 +351,11 @@ static void pc87312_initfn(Object *obj)
>>      PC87312State *s = PC87312(obj);
>>  
>>      memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312", 2);
>> +
>> +    object_initialize(&s->parallel, sizeof(s->parallel), TYPE_ISA_PARALLEL);
>> +    object_property_add_alias(obj, "par0-chardev",
>> +                              OBJECT(&s->parallel), "chardev", &error_abort);
>> +    qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0);
>>  }
>>  
>>  static const VMStateDescription vmstate_pc87312 = {
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index 7f52662..1dfa051 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -40,6 +40,7 @@
>>  #include "hw/isa/pc87312.h"
>>  #include "sysemu/block-backend.h"
>>  #include "sysemu/arch_init.h"
>> +#include "sysemu/char.h"
>>  #include "sysemu/qtest.h"
>>  #include "exec/address-spaces.h"
>>  #include "elf.h"
>> @@ -528,6 +529,7 @@ static void ppc_prep_init(MachineState *machine)
>>      PCIDevice *pci;
>>      ISABus *isa_bus;
>>      ISADevice *isa;
>> +    CharDriverState *chr;
>>      qemu_irq *cpu_exit_irq;
>>      int ppc_boot_device;
>>      DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>> @@ -639,6 +641,13 @@ static void ppc_prep_init(MachineState *machine)
>>      /* Super I/O (parallel + serial ports) */
>>      isa = isa_create(isa_bus, TYPE_PC87312);
>>      dev = DEVICE(isa);
>> +    chr = parallel_hds[0];
>> +    if (chr == NULL) {
>> +        chr = qemu_chr_new("par0", "null", NULL);
>> +    }
>> +    if (chr != NULL) {
>> +        qdev_prop_set_chr(dev, "par0-chardev", chr);
>> +    }
>>      qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
>>      qdev_init_nofail(dev);
>>  
> 
> Unlike the old code, this checks for qemu_chr_new() failure.  Can this
> happen?

Probably a result of moving the code piecewise. Not sure about the API,
to me it looks like it can return NULL and can emit an Error via
error_report_err() but doesn't pass it out to the caller...

> If it does, property "par0-chardev" remains null.
> parallel_isa_realizefn() will then fail.  Worth a comment, I think.
> 
> I don't like boards creating backends with IDs on their own as long as
> we don't have a ID namespace reserved for the system.  However, this is
> preexisting, so let's not worry about it now.

I think the problem was that PC handles this by creating devices only if
it has a default or user-supplied backend. Here, we need all aggregate
devices independent of what the user supplies, so to avoid
segfaults/errors we create dummy ones.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCH RFC for-2.3? 3/8] serial: Move ISASerialState to header

[Andreas Färber]

To be used for embedding the device.

Add gtk-doc private/public markers for parent field.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/char/serial-isa.c     | 12 ------------
 include/hw/char/serial.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index f3db024..ae2235b 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -24,18 +24,6 @@
  */
 
 #include "hw/char/serial.h"
-#include "hw/isa/isa.h"
-
-#define ISA_SERIAL(obj) OBJECT_CHECK(ISASerialState, (obj), TYPE_ISA_SERIAL)
-
-typedef struct ISASerialState {
-    ISADevice parent_obj;
-
-    uint32_t index;
-    uint32_t iobase;
-    uint32_t isairq;
-    SerialState state;
-} ISASerialState;
 
 static const int isa_serial_io[MAX_SERIAL_PORTS] = {
     0x3f8, 0x2f8, 0x3e8, 0x2e8
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 15beb6b..77cb95f 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -26,6 +26,7 @@
 #define HW_SERIAL_H 1
 
 #include "hw/hw.h"
+#include "hw/isa/isa.h"
 #include "sysemu/sysemu.h"
 #include "exec/memory.h"
 #include "qemu/fifo8.h"
@@ -92,6 +93,19 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 
 /* serial-isa.c */
 #define TYPE_ISA_SERIAL "isa-serial"
+#define ISA_SERIAL(obj) OBJECT_CHECK(ISASerialState, (obj), TYPE_ISA_SERIAL)
+
+typedef struct ISASerialState {
+    /*< private >*/
+    ISADevice parent_obj;
+    /*< public >*/
+
+    uint32_t index;
+    uint32_t iobase;
+    uint32_t isairq;
+    SerialState state;
+} ISASerialState;
+
 void serial_hds_isa_init(ISABus *bus, int n);
 
 #endif
-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 4/8] pc87312: Create UARTs in-place and add alias properties

[Andreas Färber]

Move serial_hds[] code into PReP machine.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/isa/pc87312.c         | 33 ++++++++++++++++++++-------------
 hw/ppc/prep.c            | 13 +++++++++++++
 include/hw/isa/pc87312.h |  6 ++----
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
index ded6c01..207eaa8 100644
--- a/hw/isa/pc87312.c
+++ b/hw/isa/pc87312.c
@@ -266,10 +266,8 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     DeviceState *d;
     ISADevice *isa;
     ISABus *bus;
-    CharDriverState *chr;
     DriveInfo *drive;
     Error *local_err = NULL;
-    char name[5];
     int i;
 
     s = PC87312(dev);
@@ -296,21 +294,19 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
 
     for (i = 0; i < 2; i++) {
         if (is_uart_enabled(s, i)) {
-            chr = serial_hds[i];
-            if (chr == NULL) {
-                snprintf(name, sizeof(name), "ser%d", i);
-                chr = qemu_chr_new(name, "null", NULL);
-            }
-            isa = isa_create(bus, "isa-serial");
-            d = DEVICE(isa);
-            qdev_prop_set_uint32(d, "index", i);
+            d = DEVICE(&s->uart[i]);
+            qdev_set_parent_bus(d, BUS(bus));
             qdev_prop_set_uint32(d, "iobase", get_uart_iobase(s, i));
             qdev_prop_set_uint32(d, "irq", get_uart_irq(s, i));
-            qdev_prop_set_chr(d, "chardev", chr);
-            qdev_init_nofail(d);
-            s->uart[i].dev = isa;
             trace_pc87312_info_serial(i, get_uart_iobase(s, i),
                                       get_uart_irq(s, i));
+            object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
+                                     &local_err);
+            object_unref(OBJECT(&s->uart[i]));
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
         }
     }
 
@@ -349,6 +345,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
 static void pc87312_initfn(Object *obj)
 {
     PC87312State *s = PC87312(obj);
+    int i;
 
     memory_region_init_io(&s->io, obj, &pc87312_io_ops, s, "pc87312", 2);
 
@@ -356,6 +353,16 @@ static void pc87312_initfn(Object *obj)
     object_property_add_alias(obj, "par0-chardev",
                               OBJECT(&s->parallel), "chardev", &error_abort);
     qdev_prop_set_uint32(DEVICE(&s->parallel), "index", 0);
+
+    for (i = 0; i < 2; i++) {
+        char *propname = g_strdup_printf("ser%d-chardev", i);
+        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_ISA_SERIAL);
+        qdev_prop_set_uint32(DEVICE(&s->uart[i]), "index", i);
+        object_property_add_alias(obj, propname,
+                                  OBJECT(&s->uart[i]), "chardev",
+                                  &error_abort);
+        g_free(propname);
+    }
 }
 
 static const VMStateDescription vmstate_pc87312 = {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 1dfa051..eb29d3c 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -648,6 +648,19 @@ static void ppc_prep_init(MachineState *machine)
     if (chr != NULL) {
         qdev_prop_set_chr(dev, "par0-chardev", chr);
     }
+    for (i = 0; i < 2; i++) {
+        chr = serial_hds[i];
+        if (chr == NULL) {
+            char *name = g_strdup_printf("ser%d", i);
+            chr = qemu_chr_new(name, "null", NULL);
+            g_free(name);
+        }
+        if (chr != NULL) {
+            char *name = g_strdup_printf("ser%d-chardev", i);
+            qdev_prop_set_chr(dev, name, chr);
+            g_free(name);
+        }
+    }
     qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
     qdev_init_nofail(dev);
 
diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
index b473b3b..3256e0f 100644
--- a/include/hw/isa/pc87312.h
+++ b/include/hw/isa/pc87312.h
@@ -27,6 +27,7 @@
 
 #include "hw/isa/isa.h"
 #include "hw/char/parallel.h"
+#include "hw/char/serial.h"
 
 
 #define TYPE_PC87312 "pc87312"
@@ -39,10 +40,7 @@ typedef struct PC87312State {
     uint8_t config; /* initial configuration */
 
     ISAParallelState parallel;
-
-    struct {
-        ISADevice *dev;
-    } uart[2];
+    ISASerialState uart[2];
 
     struct {
         ISADevice *dev;
-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 5/8] fdb: Move FDCtrlISABus to header

[Andreas Färber]

To be used for embedding the device.

Add gtk-doc private/public markers for parent field.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/block/fdc.c         | 87 -------------------------------------------------
 include/hw/block/fdc.h | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2bf87c9..da521f1 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -31,7 +31,6 @@
 #include "hw/block/fdc.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
-#include "hw/isa/isa.h"
 #include "hw/sysbus.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
@@ -167,33 +166,7 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
 #define FD_SECTOR_SC           2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
-typedef enum FDiskFlags {
-    FDISK_DBL_SIDES  = 0x01,
-} FDiskFlags;
-
-typedef struct FDrive {
-    FDCtrl *fdctrl;
-    BlockBackend *blk;
-    /* Drive status */
-    FDriveType drive;
-    uint8_t perpendicular;    /* 2.88 MB access mode    */
-    /* Position */
-    uint8_t head;
-    uint8_t track;
-    uint8_t sect;
-    /* Media */
-    FDiskFlags flags;
-    uint8_t last_sect;        /* Nb sector per track    */
-    uint8_t max_track;        /* Nb of tracks           */
-    uint16_t bps;             /* Bytes per sector       */
-    uint8_t ro;               /* Is read-only           */
-    uint8_t media_changed;    /* Is media changed       */
-    uint8_t media_rate;       /* Data rate of medium    */
-} FDrive;
-
 static void fd_init(FDrive *drv)
 {
     /* Drive */
@@ -498,53 +471,6 @@ enum {
 #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
 #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
 
-struct FDCtrl {
-    MemoryRegion iomem;
-    qemu_irq irq;
-    /* Controller state */
-    QEMUTimer *result_timer;
-    int dma_chann;
-    /* Controller's identification */
-    uint8_t version;
-    /* HW */
-    uint8_t sra;
-    uint8_t srb;
-    uint8_t dor;
-    uint8_t dor_vmstate; /* only used as temp during vmstate */
-    uint8_t tdr;
-    uint8_t dsr;
-    uint8_t msr;
-    uint8_t cur_drv;
-    uint8_t status0;
-    uint8_t status1;
-    uint8_t status2;
-    /* Command FIFO */
-    uint8_t *fifo;
-    int32_t fifo_size;
-    uint32_t data_pos;
-    uint32_t data_len;
-    uint8_t data_state;
-    uint8_t data_dir;
-    uint8_t eot; /* last wanted sector */
-    /* States kept only to be returned back */
-    /* precompensation */
-    uint8_t precomp_trk;
-    uint8_t config;
-    uint8_t lock;
-    /* Power down config (also with status regB access mode */
-    uint8_t pwrd;
-    /* Floppy drives */
-    uint8_t num_floppies;
-    /* Sun4m quirks? */
-    int sun4m;
-    FDrive drives[MAX_FD];
-    int reset_sensei;
-    uint32_t check_media_rate;
-    /* Timers state */
-    uint8_t timer0;
-    uint8_t timer1;
-};
-
 #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
 #define SYSBUS_FDC(obj) OBJECT_CHECK(FDCtrlSysBus, (obj), TYPE_SYSBUS_FDC)
 
@@ -556,19 +482,6 @@ typedef struct FDCtrlSysBus {
     struct FDCtrl state;
 } FDCtrlSysBus;
 
-#define ISA_FDC(obj) OBJECT_CHECK(FDCtrlISABus, (obj), TYPE_ISA_FDC)
-
-typedef struct FDCtrlISABus {
-    ISADevice parent_obj;
-
-    uint32_t iobase;
-    uint32_t irq;
-    uint32_t dma;
-    struct FDCtrl state;
-    int32_t bootindexA;
-    int32_t bootindexB;
-} FDCtrlISABus;
-
 static uint32_t fdctrl_read (void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index d48b2f8..86d852d 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -2,6 +2,7 @@
 #define HW_FDC_H
 
 #include "qemu-common.h"
+#include "hw/isa/isa.h"
 
 /* fdc.c */
 #define MAX_FD 2
@@ -13,7 +14,94 @@ typedef enum FDriveType {
     FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
 } FDriveType;
 
+typedef enum FDiskFlags {
+    FDISK_DBL_SIDES  = 0x01,
+} FDiskFlags;
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FDrive {
+    FDCtrl *fdctrl;
+    BlockBackend *blk;
+    /* Drive status */
+    FDriveType drive;
+    uint8_t perpendicular;    /* 2.88 MB access mode    */
+    /* Position */
+    uint8_t head;
+    uint8_t track;
+    uint8_t sect;
+    /* Media */
+    FDiskFlags flags;
+    uint8_t last_sect;        /* Nb sector per track    */
+    uint8_t max_track;        /* Nb of tracks           */
+    uint16_t bps;             /* Bytes per sector       */
+    uint8_t ro;               /* Is read-only           */
+    uint8_t media_changed;    /* Is media changed       */
+    uint8_t media_rate;       /* Data rate of medium    */
+} FDrive;
+
+struct FDCtrl {
+    MemoryRegion iomem;
+    qemu_irq irq;
+    /* Controller state */
+    QEMUTimer *result_timer;
+    int dma_chann;
+    /* Controller's identification */
+    uint8_t version;
+    /* HW */
+    uint8_t sra;
+    uint8_t srb;
+    uint8_t dor;
+    uint8_t dor_vmstate; /* only used as temp during vmstate */
+    uint8_t tdr;
+    uint8_t dsr;
+    uint8_t msr;
+    uint8_t cur_drv;
+    uint8_t status0;
+    uint8_t status1;
+    uint8_t status2;
+    /* Command FIFO */
+    uint8_t *fifo;
+    int32_t fifo_size;
+    uint32_t data_pos;
+    uint32_t data_len;
+    uint8_t data_state;
+    uint8_t data_dir;
+    uint8_t eot; /* last wanted sector */
+    /* States kept only to be returned back */
+    /* precompensation */
+    uint8_t precomp_trk;
+    uint8_t config;
+    uint8_t lock;
+    /* Power down config (also with status regB access mode */
+    uint8_t pwrd;
+    /* Floppy drives */
+    uint8_t num_floppies;
+    /* Sun4m quirks? */
+    int sun4m;
+    FDrive drives[MAX_FD];
+    int reset_sensei;
+    uint32_t check_media_rate;
+    /* Timers state */
+    uint8_t timer0;
+    uint8_t timer1;
+};
+
 #define TYPE_ISA_FDC "isa-fdc"
+#define ISA_FDC(obj) OBJECT_CHECK(FDCtrlISABus, (obj), TYPE_ISA_FDC)
+
+typedef struct FDCtrlISABus {
+    /*< private >*/
+    ISADevice parent_obj;
+    /*< public >*/
+
+    uint32_t iobase;
+    uint32_t irq;
+    uint32_t dma;
+    struct FDCtrl state;
+    int32_t bootindexA;
+    int32_t bootindexB;
+} FDCtrlISABus;
 
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC for-2.3? 5/8] fdb: Move FDCtrlISABus to header

[John Snow]

You probably meant 'fdc' !

On 03/29/2015 01:53 PM, Andreas Färber wrote:
> To be used for embedding the device.
>
> Add gtk-doc private/public markers for parent field.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>   hw/block/fdc.c         | 87 -------------------------------------------------
>   include/hw/block/fdc.h | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 88 insertions(+), 87 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2bf87c9..da521f1 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -31,7 +31,6 @@
>   #include "hw/block/fdc.h"
>   #include "qemu/error-report.h"
>   #include "qemu/timer.h"
> -#include "hw/isa/isa.h"
>   #include "hw/sysbus.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/blockdev.h"
> @@ -167,33 +166,7 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>   #define FD_SECTOR_SC           2   /* Sector size code */
>   #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
>
> -typedef struct FDCtrl FDCtrl;
> -
>   /* Floppy disk drive emulation */
> -typedef enum FDiskFlags {
> -    FDISK_DBL_SIDES  = 0x01,
> -} FDiskFlags;
> -
> -typedef struct FDrive {
> -    FDCtrl *fdctrl;
> -    BlockBackend *blk;
> -    /* Drive status */
> -    FDriveType drive;
> -    uint8_t perpendicular;    /* 2.88 MB access mode    */
> -    /* Position */
> -    uint8_t head;
> -    uint8_t track;
> -    uint8_t sect;
> -    /* Media */
> -    FDiskFlags flags;
> -    uint8_t last_sect;        /* Nb sector per track    */
> -    uint8_t max_track;        /* Nb of tracks           */
> -    uint16_t bps;             /* Bytes per sector       */
> -    uint8_t ro;               /* Is read-only           */
> -    uint8_t media_changed;    /* Is media changed       */
> -    uint8_t media_rate;       /* Data rate of medium    */
> -} FDrive;
> -
>   static void fd_init(FDrive *drv)
>   {
>       /* Drive */
> @@ -498,53 +471,6 @@ enum {
>   #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
>   #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>
> -struct FDCtrl {
> -    MemoryRegion iomem;
> -    qemu_irq irq;
> -    /* Controller state */
> -    QEMUTimer *result_timer;
> -    int dma_chann;
> -    /* Controller's identification */
> -    uint8_t version;
> -    /* HW */
> -    uint8_t sra;
> -    uint8_t srb;
> -    uint8_t dor;
> -    uint8_t dor_vmstate; /* only used as temp during vmstate */
> -    uint8_t tdr;
> -    uint8_t dsr;
> -    uint8_t msr;
> -    uint8_t cur_drv;
> -    uint8_t status0;
> -    uint8_t status1;
> -    uint8_t status2;
> -    /* Command FIFO */
> -    uint8_t *fifo;
> -    int32_t fifo_size;
> -    uint32_t data_pos;
> -    uint32_t data_len;
> -    uint8_t data_state;
> -    uint8_t data_dir;
> -    uint8_t eot; /* last wanted sector */
> -    /* States kept only to be returned back */
> -    /* precompensation */
> -    uint8_t precomp_trk;
> -    uint8_t config;
> -    uint8_t lock;
> -    /* Power down config (also with status regB access mode */
> -    uint8_t pwrd;
> -    /* Floppy drives */
> -    uint8_t num_floppies;
> -    /* Sun4m quirks? */
> -    int sun4m;
> -    FDrive drives[MAX_FD];
> -    int reset_sensei;
> -    uint32_t check_media_rate;
> -    /* Timers state */
> -    uint8_t timer0;
> -    uint8_t timer1;
> -};
> -
>   #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>   #define SYSBUS_FDC(obj) OBJECT_CHECK(FDCtrlSysBus, (obj), TYPE_SYSBUS_FDC)
>
> @@ -556,19 +482,6 @@ typedef struct FDCtrlSysBus {
>       struct FDCtrl state;
>   } FDCtrlSysBus;
>
> -#define ISA_FDC(obj) OBJECT_CHECK(FDCtrlISABus, (obj), TYPE_ISA_FDC)
> -
> -typedef struct FDCtrlISABus {
> -    ISADevice parent_obj;
> -
> -    uint32_t iobase;
> -    uint32_t irq;
> -    uint32_t dma;
> -    struct FDCtrl state;
> -    int32_t bootindexA;
> -    int32_t bootindexB;
> -} FDCtrlISABus;
> -
>   static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>   {
>       FDCtrl *fdctrl = opaque;
> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
> index d48b2f8..86d852d 100644
> --- a/include/hw/block/fdc.h
> +++ b/include/hw/block/fdc.h
> @@ -2,6 +2,7 @@
>   #define HW_FDC_H
>
>   #include "qemu-common.h"
> +#include "hw/isa/isa.h"
>
>   /* fdc.c */
>   #define MAX_FD 2
> @@ -13,7 +14,94 @@ typedef enum FDriveType {
>       FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
>   } FDriveType;
>
> +typedef enum FDiskFlags {
> +    FDISK_DBL_SIDES  = 0x01,
> +} FDiskFlags;
> +
> +typedef struct FDCtrl FDCtrl;
> +
> +typedef struct FDrive {
> +    FDCtrl *fdctrl;
> +    BlockBackend *blk;
> +    /* Drive status */
> +    FDriveType drive;
> +    uint8_t perpendicular;    /* 2.88 MB access mode    */
> +    /* Position */
> +    uint8_t head;
> +    uint8_t track;
> +    uint8_t sect;
> +    /* Media */
> +    FDiskFlags flags;
> +    uint8_t last_sect;        /* Nb sector per track    */
> +    uint8_t max_track;        /* Nb of tracks           */
> +    uint16_t bps;             /* Bytes per sector       */
> +    uint8_t ro;               /* Is read-only           */
> +    uint8_t media_changed;    /* Is media changed       */
> +    uint8_t media_rate;       /* Data rate of medium    */
> +} FDrive;
> +
> +struct FDCtrl {
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +    /* Controller state */
> +    QEMUTimer *result_timer;
> +    int dma_chann;
> +    /* Controller's identification */
> +    uint8_t version;
> +    /* HW */
> +    uint8_t sra;
> +    uint8_t srb;
> +    uint8_t dor;
> +    uint8_t dor_vmstate; /* only used as temp during vmstate */
> +    uint8_t tdr;
> +    uint8_t dsr;
> +    uint8_t msr;
> +    uint8_t cur_drv;
> +    uint8_t status0;
> +    uint8_t status1;
> +    uint8_t status2;
> +    /* Command FIFO */
> +    uint8_t *fifo;
> +    int32_t fifo_size;
> +    uint32_t data_pos;
> +    uint32_t data_len;
> +    uint8_t data_state;
> +    uint8_t data_dir;
> +    uint8_t eot; /* last wanted sector */
> +    /* States kept only to be returned back */
> +    /* precompensation */
> +    uint8_t precomp_trk;
> +    uint8_t config;
> +    uint8_t lock;
> +    /* Power down config (also with status regB access mode */
> +    uint8_t pwrd;
> +    /* Floppy drives */
> +    uint8_t num_floppies;
> +    /* Sun4m quirks? */
> +    int sun4m;
> +    FDrive drives[MAX_FD];
> +    int reset_sensei;
> +    uint32_t check_media_rate;
> +    /* Timers state */
> +    uint8_t timer0;
> +    uint8_t timer1;
> +};
> +
>   #define TYPE_ISA_FDC "isa-fdc"
> +#define ISA_FDC(obj) OBJECT_CHECK(FDCtrlISABus, (obj), TYPE_ISA_FDC)
> +
> +typedef struct FDCtrlISABus {
> +    /*< private >*/
> +    ISADevice parent_obj;
> +    /*< public >*/
> +
> +    uint32_t iobase;
> +    uint32_t irq;
> +    uint32_t dma;
> +    struct FDCtrl state;
> +    int32_t bootindexA;
> +    int32_t bootindexB;
> +} FDCtrlISABus;
>
>   ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
>   void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>

Both this and the IDE patch look programmatically sound to me, but I 
missed the discussion on why it's needed.

Re: [Qemu-devel] [PATCH RFC for-2.3? 5/8] fdb: Move FDCtrlISABus to header

[Andreas Färber]

Am 31.03.2015 um 03:38 schrieb John Snow:
> You probably meant 'fdc' !
> 
> On 03/29/2015 01:53 PM, Andreas Färber wrote:
>> To be used for embedding the device.
>>
>> Add gtk-doc private/public markers for parent field.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>   hw/block/fdc.c         | 87
>> -------------------------------------------------
>>   include/hw/block/fdc.h | 88
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 88 insertions(+), 87 deletions(-)

John, I've rebased this onto the sun4m fdc change. Are you planning to
take this through your IDE tree or should this go through qom-next?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC for-2.3? 5/8] fdb: Move FDCtrlISABus to header

[John Snow]

On 05/19/2015 09:02 AM, Andreas Färber wrote:
> Am 31.03.2015 um 03:38 schrieb John Snow:
>> You probably meant 'fdc' !
>>
>> On 03/29/2015 01:53 PM, Andreas Färber wrote:
>>> To be used for embedding the device.
>>>
>>> Add gtk-doc private/public markers for parent field.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>   hw/block/fdc.c         | 87
>>> -------------------------------------------------
>>>   include/hw/block/fdc.h | 88
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 88 insertions(+), 87 deletions(-)
> 
> John, I've rebased this onto the sun4m fdc change. Are you planning to
> take this through your IDE tree or should this go through qom-next?
> 
> Regards,
> Andreas
> 

I don't need to pick a single patch out of a series because you touch
the FDC stuff, just keep it in-line with the rest of the patches.

I haven't approved anything post-CVE-2015-3456 yet, so as long as you
send the pullreq soon we won't hurt anything*

*Except Kevin's series that he just posted this morning, but I'll deal
with that.

--js

[Qemu-devel] [PATCH RFC for-2.3? 6/8] pc87312: Create FDC in-place

[Andreas Färber]

Move drive_get() code to PReP machine.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/isa/pc87312.c         | 32 ++++++++++++++++----------------
 hw/ppc/prep.c            | 11 +++++++++++
 include/hw/isa/pc87312.h |  6 ++----
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
index 207eaa8..d35eb0e 100644
--- a/hw/isa/pc87312.c
+++ b/hw/isa/pc87312.c
@@ -266,7 +266,6 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     DeviceState *d;
     ISADevice *isa;
     ISABus *bus;
-    DriveInfo *drive;
     Error *local_err = NULL;
     int i;
 
@@ -311,23 +310,17 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     }
 
     if (is_fdc_enabled(s)) {
-        isa = isa_create(bus, "isa-fdc");
-        d = DEVICE(isa);
+        d = DEVICE(&s->fdc);
+        qdev_set_parent_bus(d, BUS(bus));
         qdev_prop_set_uint32(d, "iobase", get_fdc_iobase(s));
-        qdev_prop_set_uint32(d, "irq", 6);
-        drive = drive_get(IF_FLOPPY, 0, 0);
-        if (drive != NULL) {
-            qdev_prop_set_drive_nofail(d, "driveA",
-                                       blk_by_legacy_dinfo(drive));
-        }
-        drive = drive_get(IF_FLOPPY, 0, 1);
-        if (drive != NULL) {
-            qdev_prop_set_drive_nofail(d, "driveB",
-                                       blk_by_legacy_dinfo(drive));
-        }
-        qdev_init_nofail(d);
-        s->fdc.dev = isa;
         trace_pc87312_info_floppy(get_fdc_iobase(s));
+        object_property_set_bool(OBJECT(&s->fdc), true, "realized",
+                                 &local_err);
+        object_unref(OBJECT(&s->fdc));
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (is_ide_enabled(s)) {
@@ -363,6 +356,13 @@ static void pc87312_initfn(Object *obj)
                                   &error_abort);
         g_free(propname);
     }
+
+    object_initialize(&s->fdc, sizeof(s->fdc), TYPE_ISA_FDC);
+    object_property_add_alias(obj, "fdc-driveA",
+                              OBJECT(&s->fdc), "driveA", &error_abort);
+    object_property_add_alias(obj, "fdc-driveB",
+                              OBJECT(&s->fdc), "driveB", &error_abort);
+    qdev_prop_set_uint32(DEVICE(&s->fdc), "irq", 6);
 }
 
 static const VMStateDescription vmstate_pc87312 = {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index eb29d3c..272a284 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -530,6 +530,7 @@ static void ppc_prep_init(MachineState *machine)
     ISABus *isa_bus;
     ISADevice *isa;
     CharDriverState *chr;
+    DriveInfo *drive;
     qemu_irq *cpu_exit_irq;
     int ppc_boot_device;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -661,6 +662,16 @@ static void ppc_prep_init(MachineState *machine)
             g_free(name);
         }
     }
+    drive = drive_get(IF_FLOPPY, 0, 0);
+    if (drive != NULL) {
+        qdev_prop_set_drive_nofail(dev, "fdc-driveA",
+                                   blk_by_legacy_dinfo(drive));
+    }
+    drive = drive_get(IF_FLOPPY, 0, 1);
+    if (drive != NULL) {
+        qdev_prop_set_drive_nofail(dev, "fdc-driveB",
+                                   blk_by_legacy_dinfo(drive));
+    }
     qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
     qdev_init_nofail(dev);
 
diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
index 3256e0f..c49a06d 100644
--- a/include/hw/isa/pc87312.h
+++ b/include/hw/isa/pc87312.h
@@ -28,6 +28,7 @@
 #include "hw/isa/isa.h"
 #include "hw/char/parallel.h"
 #include "hw/char/serial.h"
+#include "hw/block/fdc.h"
 
 
 #define TYPE_PC87312 "pc87312"
@@ -41,10 +42,7 @@ typedef struct PC87312State {
 
     ISAParallelState parallel;
     ISASerialState uart[2];
-
-    struct {
-        ISADevice *dev;
-    } fdc;
+    FDCtrlISABus fdc;
 
     struct {
         ISADevice *dev;
-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 7/8] ide: Move ISAIDEState to header

[Andreas Färber]

To be used for embedding the device.

Add gtk-doc private/public markers for parent fields.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ide/internal.h | 155 ------------------------------------------------
 hw/ide/isa.c      |  13 ----
 include/hw/ide.h  | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+), 168 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 965cc55..d22f66d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -19,12 +19,6 @@
 //#define DEBUG_AIO
 #define USE_DMA_CDROM
 
-typedef struct IDEBus IDEBus;
-typedef struct IDEDevice IDEDevice;
-typedef struct IDEState IDEState;
-typedef struct IDEDMA IDEDMA;
-typedef struct IDEDMAOps IDEDMAOps;
-
 #define TYPE_IDE_BUS "IDE"
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
@@ -317,158 +311,9 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define SMART_DISABLE         0xd9
 #define SMART_STATUS          0xda
 
-typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
-
-typedef void EndTransferFunc(IDEState *);
-
-typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *);
-typedef void DMAVoidFunc(IDEDMA *);
-typedef int DMAIntFunc(IDEDMA *, int);
-typedef int32_t DMAInt32Func(IDEDMA *, int);
-typedef void DMAu32Func(IDEDMA *, uint32_t);
-typedef void DMAStopFunc(IDEDMA *, bool);
-typedef void DMARestartFunc(void *, int, RunState);
-
-struct unreported_events {
-    bool eject_request;
-    bool new_media;
-};
-
-enum ide_dma_cmd {
-    IDE_DMA_READ,
-    IDE_DMA_WRITE,
-    IDE_DMA_TRIM,
-};
-
 #define ide_cmd_is_read(s) \
 	((s)->dma_cmd == IDE_DMA_READ)
 
-/* NOTE: IDEState represents in fact one drive */
-struct IDEState {
-    IDEBus *bus;
-    uint8_t unit;
-    /* ide config */
-    IDEDriveKind drive_kind;
-    int cylinders, heads, sectors, chs_trans;
-    int64_t nb_sectors;
-    int mult_sectors;
-    int identify_set;
-    uint8_t identify_data[512];
-    int drive_serial;
-    char drive_serial_str[21];
-    char drive_model_str[41];
-    uint64_t wwn;
-    /* ide regs */
-    uint8_t feature;
-    uint8_t error;
-    uint32_t nsector;
-    uint8_t sector;
-    uint8_t lcyl;
-    uint8_t hcyl;
-    /* other part of tf for lba48 support */
-    uint8_t hob_feature;
-    uint8_t hob_nsector;
-    uint8_t hob_sector;
-    uint8_t hob_lcyl;
-    uint8_t hob_hcyl;
-
-    uint8_t select;
-    uint8_t status;
-
-    /* set for lba48 access */
-    uint8_t lba48;
-    BlockBackend *blk;
-    char version[9];
-    /* ATAPI specific */
-    struct unreported_events events;
-    uint8_t sense_key;
-    uint8_t asc;
-    bool tray_open;
-    bool tray_locked;
-    uint8_t cdrom_changed;
-    int packet_transfer_size;
-    int elementary_transfer_size;
-    int32_t io_buffer_index;
-    int lba;
-    int cd_sector_size;
-    int atapi_dma; /* true if dma is requested for the packet cmd */
-    BlockAcctCookie acct;
-    BlockAIOCB *pio_aiocb;
-    struct iovec iov;
-    QEMUIOVector qiov;
-    /* ATA DMA state */
-    int32_t io_buffer_offset;
-    int32_t io_buffer_size;
-    QEMUSGList sg;
-    /* PIO transfer handling */
-    int req_nb_sectors; /* number of sectors per interrupt */
-    EndTransferFunc *end_transfer_func;
-    uint8_t *data_ptr;
-    uint8_t *data_end;
-    uint8_t *io_buffer;
-    /* PIO save/restore */
-    int32_t io_buffer_total_len;
-    int32_t cur_io_buffer_offset;
-    int32_t cur_io_buffer_len;
-    uint8_t end_transfer_fn_idx;
-    QEMUTimer *sector_write_timer; /* only used for win2k install hack */
-    uint32_t irq_count; /* counts IRQs when using win2k install hack */
-    /* CF-ATA extended error */
-    uint8_t ext_error;
-    /* CF-ATA metadata storage */
-    uint32_t mdata_size;
-    uint8_t *mdata_storage;
-    int media_changed;
-    enum ide_dma_cmd dma_cmd;
-    /* SMART */
-    uint8_t smart_enabled;
-    uint8_t smart_autosave;
-    int smart_errors;
-    uint8_t smart_selftest_count;
-    uint8_t *smart_selftest_data;
-    /* AHCI */
-    int ncq_queues;
-};
-
-struct IDEDMAOps {
-    DMAStartFunc *start_dma;
-    DMAVoidFunc *start_transfer;
-    DMAInt32Func *prepare_buf;
-    DMAu32Func *commit_buf;
-    DMAIntFunc *rw_buf;
-    DMAVoidFunc *restart_dma;
-    DMAStopFunc *set_inactive;
-    DMAVoidFunc *cmd_done;
-    DMAVoidFunc *reset;
-};
-
-struct IDEDMA {
-    const struct IDEDMAOps *ops;
-    struct iovec iov;
-    QEMUIOVector qiov;
-    BlockAIOCB *aiocb;
-};
-
-struct IDEBus {
-    BusState qbus;
-    IDEDevice *master;
-    IDEDevice *slave;
-    IDEState ifs[2];
-    QEMUBH *bh;
-
-    int bus_id;
-    int max_units;
-    IDEDMA *dma;
-    uint8_t unit;
-    uint8_t cmd;
-    qemu_irq irq;
-
-    int error_status;
-    uint8_t retry_unit;
-    int64_t retry_sector_num;
-    uint32_t retry_nsector;
-};
-
 #define TYPE_IDE_DEVICE "ide-device"
 #define IDE_DEVICE(obj) \
      OBJECT_CHECK(IDEDevice, (obj), TYPE_IDE_DEVICE)
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 9f80503..05d1fd5 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -33,19 +33,6 @@
 /***********************************************************/
 /* ISA IDE definitions */
 
-#define TYPE_ISA_IDE "isa-ide"
-#define ISA_IDE(obj) OBJECT_CHECK(ISAIDEState, (obj), TYPE_ISA_IDE)
-
-typedef struct ISAIDEState {
-    ISADevice parent_obj;
-
-    IDEBus    bus;
-    uint32_t  iobase;
-    uint32_t  iobase2;
-    uint32_t  isairq;
-    qemu_irq  irq;
-} ISAIDEState;
-
 static void isa_ide_reset(DeviceState *d)
 {
     ISAIDEState *s = ISA_IDE(d);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index bc8bd32..a864388 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -7,6 +7,164 @@
 
 #define MAX_IDE_DEVS	2
 
+typedef struct IDEBus IDEBus;
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;
+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+
+typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
+
+typedef void EndTransferFunc(IDEState *);
+
+typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *);
+typedef void DMAVoidFunc(IDEDMA *);
+typedef int DMAIntFunc(IDEDMA *, int);
+typedef int32_t DMAInt32Func(IDEDMA *, int);
+typedef void DMAu32Func(IDEDMA *, uint32_t);
+typedef void DMAStopFunc(IDEDMA *, bool);
+typedef void DMARestartFunc(void *, int, RunState);
+
+struct unreported_events {
+    bool eject_request;
+    bool new_media;
+};
+
+enum ide_dma_cmd {
+    IDE_DMA_READ,
+    IDE_DMA_WRITE,
+    IDE_DMA_TRIM,
+};
+
+/* NOTE: IDEState represents in fact one drive */
+struct IDEState {
+    IDEBus *bus;
+    uint8_t unit;
+    /* ide config */
+    IDEDriveKind drive_kind;
+    int cylinders, heads, sectors, chs_trans;
+    int64_t nb_sectors;
+    int mult_sectors;
+    int identify_set;
+    uint8_t identify_data[512];
+    int drive_serial;
+    char drive_serial_str[21];
+    char drive_model_str[41];
+    uint64_t wwn;
+    /* ide regs */
+    uint8_t feature;
+    uint8_t error;
+    uint32_t nsector;
+    uint8_t sector;
+    uint8_t lcyl;
+    uint8_t hcyl;
+    /* other part of tf for lba48 support */
+    uint8_t hob_feature;
+    uint8_t hob_nsector;
+    uint8_t hob_sector;
+    uint8_t hob_lcyl;
+    uint8_t hob_hcyl;
+
+    uint8_t select;
+    uint8_t status;
+
+    /* set for lba48 access */
+    uint8_t lba48;
+    BlockBackend *blk;
+    char version[9];
+    /* ATAPI specific */
+    struct unreported_events events;
+    uint8_t sense_key;
+    uint8_t asc;
+    bool tray_open;
+    bool tray_locked;
+    uint8_t cdrom_changed;
+    int packet_transfer_size;
+    int elementary_transfer_size;
+    int32_t io_buffer_index;
+    int lba;
+    int cd_sector_size;
+    int atapi_dma; /* true if dma is requested for the packet cmd */
+    BlockAcctCookie acct;
+    BlockAIOCB *pio_aiocb;
+    struct iovec iov;
+    QEMUIOVector qiov;
+    /* ATA DMA state */
+    int32_t io_buffer_offset;
+    int32_t io_buffer_size;
+    QEMUSGList sg;
+    /* PIO transfer handling */
+    int req_nb_sectors; /* number of sectors per interrupt */
+    EndTransferFunc *end_transfer_func;
+    uint8_t *data_ptr;
+    uint8_t *data_end;
+    uint8_t *io_buffer;
+    /* PIO save/restore */
+    int32_t io_buffer_total_len;
+    int32_t cur_io_buffer_offset;
+    int32_t cur_io_buffer_len;
+    uint8_t end_transfer_fn_idx;
+    QEMUTimer *sector_write_timer; /* only used for win2k install hack */
+    uint32_t irq_count; /* counts IRQs when using win2k install hack */
+    /* CF-ATA extended error */
+    uint8_t ext_error;
+    /* CF-ATA metadata storage */
+    uint32_t mdata_size;
+    uint8_t *mdata_storage;
+    int media_changed;
+    enum ide_dma_cmd dma_cmd;
+    /* SMART */
+    uint8_t smart_enabled;
+    uint8_t smart_autosave;
+    int smart_errors;
+    uint8_t smart_selftest_count;
+    uint8_t *smart_selftest_data;
+    /* AHCI */
+    int ncq_queues;
+};
+
+struct IDEDMAOps {
+    DMAStartFunc *start_dma;
+    DMAVoidFunc *start_transfer;
+    DMAInt32Func *prepare_buf;
+    DMAu32Func *commit_buf;
+    DMAIntFunc *rw_buf;
+    DMAVoidFunc *restart_dma;
+    DMAStopFunc *set_inactive;
+    DMAVoidFunc *cmd_done;
+    DMAVoidFunc *reset;
+};
+
+struct IDEDMA {
+    const struct IDEDMAOps *ops;
+    struct iovec iov;
+    QEMUIOVector qiov;
+    BlockAIOCB *aiocb;
+};
+
+struct IDEBus {
+    /*< private >*/
+    BusState qbus;
+    /*< public >*/
+
+    IDEDevice *master;
+    IDEDevice *slave;
+    IDEState ifs[2];
+    QEMUBH *bh;
+
+    int bus_id;
+    int max_units;
+    IDEDMA *dma;
+    uint8_t unit;
+    uint8_t cmd;
+    qemu_irq irq;
+
+    int error_status;
+    uint8_t retry_unit;
+    int64_t retry_sector_num;
+    uint32_t retry_nsector;
+};
+
 /* ide-isa.c */
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
@@ -28,6 +186,21 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 
 /* ide/core.c */
+#define TYPE_ISA_IDE "isa-ide"
+#define ISA_IDE(obj) OBJECT_CHECK(ISAIDEState, (obj), TYPE_ISA_IDE)
+
+typedef struct ISAIDEState {
+    /*< private >*/
+    ISADevice parent_obj;
+    /*< public >*/
+
+    IDEBus    bus;
+    uint32_t  iobase;
+    uint32_t  iobase2;
+    uint32_t  isairq;
+    qemu_irq  irq;
+} ISAIDEState;
+
 void ide_drive_get(DriveInfo **hd, int max_bus);
 
 #endif /* HW_IDE_H */
-- 
2.1.4

[Qemu-devel] [PATCH RFC for-2.3? 8/8] pc87312: Create IDE in-place

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/isa/pc87312.c         | 17 ++++++++++++-----
 include/hw/isa/pc87312.h |  6 ++----
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
index d35eb0e..37e2df4 100644
--- a/hw/isa/pc87312.c
+++ b/hw/isa/pc87312.c
@@ -324,14 +324,18 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
     }
 
     if (is_ide_enabled(s)) {
-        isa = isa_create(bus, "isa-ide");
-        d = DEVICE(isa);
+        d = DEVICE(&s->ide);
+        qdev_set_parent_bus(d, BUS(bus));
         qdev_prop_set_uint32(d, "iobase", get_ide_iobase(s));
         qdev_prop_set_uint32(d, "iobase2", get_ide_iobase(s) + 0x206);
-        qdev_prop_set_uint32(d, "irq", 14);
-        qdev_init_nofail(d);
-        s->ide.dev = isa;
         trace_pc87312_info_ide(get_ide_iobase(s));
+        object_property_set_bool(OBJECT(&s->ide), true, "realized",
+                                 &local_err);
+        object_unref(OBJECT(&s->fdc));
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 }
 
@@ -363,6 +367,9 @@ static void pc87312_initfn(Object *obj)
     object_property_add_alias(obj, "fdc-driveB",
                               OBJECT(&s->fdc), "driveB", &error_abort);
     qdev_prop_set_uint32(DEVICE(&s->fdc), "irq", 6);
+
+    object_initialize(&s->ide, sizeof(s->ide), TYPE_ISA_IDE);
+    qdev_prop_set_uint32(DEVICE(&s->ide), "irq", 14);
 }
 
 static const VMStateDescription vmstate_pc87312 = {
diff --git a/include/hw/isa/pc87312.h b/include/hw/isa/pc87312.h
index c49a06d..ffc2b96 100644
--- a/include/hw/isa/pc87312.h
+++ b/include/hw/isa/pc87312.h
@@ -29,6 +29,7 @@
 #include "hw/char/parallel.h"
 #include "hw/char/serial.h"
 #include "hw/block/fdc.h"
+#include "hw/ide.h"
 
 
 #define TYPE_PC87312 "pc87312"
@@ -43,10 +44,7 @@ typedef struct PC87312State {
     ISAParallelState parallel;
     ISASerialState uart[2];
     FDCtrlISABus fdc;
-
-    struct {
-        ISADevice *dev;
-    } ide;
+    ISAIDEState ide;
 
     MemoryRegion io;
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Hello Markus et al.,
>
> This series attempts to fix the -device pc87312 issues you reported.
>
> I can't add alias properties for devices that don't get created before realize.
> Therefore this involves moving code for various ISA devices, to enable us
> to initialize the objects early for alias properties and realizing them
> as part of the composite device once the configuration is known, also fixing
> error propagation while at it. Probably needs a further iteration.
>
> Yes, it's terribly invasive, that's why the code was as it is.
> But the code movements are quite trivial, as long as no in-air conflicts occur.

Pity we have to move the state structs to the header, but that what we
have to do to make them embeddable.  And I understand embedding is how
we do sub-devices ("part of" instead of "plugged into").

Your changes are quite regular.  They don't look scary to me, but that
could be just ignorance.  They're non-trivial enough though to make wary
of merging them for 2.3 this late.

> A follow-up would be to respin my old ISA enabled/disabled series to allow
> inactive ISADevices sitting on an ISABus.

I don't remember this series :)

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Paolo Bonzini]

On 30/03/2015 16:25, Markus Armbruster wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Hello Markus et al.,
>>
>> This series attempts to fix the -device pc87312 issues you reported.
>>
>> I can't add alias properties for devices that don't get created before realize.
>> Therefore this involves moving code for various ISA devices, to enable us
>> to initialize the objects early for alias properties and realizing them
>> as part of the composite device once the configuration is known, also fixing
>> error propagation while at it. Probably needs a further iteration.
>>
>> Yes, it's terribly invasive, that's why the code was as it is.
>> But the code movements are quite trivial, as long as no in-air conflicts occur.
> 
> Pity we have to move the state structs to the header, but that what we
> have to do to make them embeddable.  And I understand embedding is how
> we do sub-devices ("part of" instead of "plugged into").
> 
> Your changes are quite regular.  They don't look scary to me, but that
> could be just ignorance.  They're non-trivial enough though to make wary
> of merging them for 2.3 this late.

The question is really: what is gained from this series as of 2.3?  Is
anything actually using "-device pc87312"?  It was broken before IIUC,
so it can remain broken for one more version.  Markus's patches would
just add one more "git revert" to this series, basically.

Paolo

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Andreas Färber]

Am 30.03.2015 um 18:12 schrieb Paolo Bonzini:
> On 30/03/2015 16:25, Markus Armbruster wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>>
>>> Hello Markus et al.,
>>>
>>> This series attempts to fix the -device pc87312 issues you reported.
>>>
>>> I can't add alias properties for devices that don't get created before realize.
>>> Therefore this involves moving code for various ISA devices, to enable us
>>> to initialize the objects early for alias properties and realizing them
>>> as part of the composite device once the configuration is known, also fixing
>>> error propagation while at it. Probably needs a further iteration.
>>>
>>> Yes, it's terribly invasive, that's why the code was as it is.
>>> But the code movements are quite trivial, as long as no in-air conflicts occur.
>>
>> Pity we have to move the state structs to the header, but that what we
>> have to do to make them embeddable.  And I understand embedding is how
>> we do sub-devices ("part of" instead of "plugged into").
>>
>> Your changes are quite regular.  They don't look scary to me, but that
>> could be just ignorance.  They're non-trivial enough though to make wary
>> of merging them for 2.3 this late.
> 
> The question is really: what is gained from this series as of 2.3?  Is
> anything actually using "-device pc87312"?  It was broken before IIUC,
> so it can remain broken for one more version.  Markus's patches would
> just add one more "git revert" to this series, basically.

Well, my intent was to avoid adding FIXMEs by fixing it properly right
away. But I don't see an urgent need as long as it's start-up errors and
not a device_add-induced crash (not sure here, would need to re-read
Markus' thread). Agree that it's getting late.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 30.03.2015 um 18:12 schrieb Paolo Bonzini:
>> On 30/03/2015 16:25, Markus Armbruster wrote:
>>> Andreas Färber <afaerber@suse.de> writes:
>>>
>>>> Hello Markus et al.,
>>>>
>>>> This series attempts to fix the -device pc87312 issues you reported.
>>>>
>>>> I can't add alias properties for devices that don't get created
>>>> before realize.
>>>> Therefore this involves moving code for various ISA devices, to enable us
>>>> to initialize the objects early for alias properties and realizing them
>>>> as part of the composite device once the configuration is known, also fixing
>>>> error propagation while at it. Probably needs a further iteration.
>>>>
>>>> Yes, it's terribly invasive, that's why the code was as it is.
>>>> But the code movements are quite trivial, as long as no in-air
>>>> conflicts occur.
>>>
>>> Pity we have to move the state structs to the header, but that what we
>>> have to do to make them embeddable.  And I understand embedding is how
>>> we do sub-devices ("part of" instead of "plugged into").
>>>
>>> Your changes are quite regular.  They don't look scary to me, but that
>>> could be just ignorance.  They're non-trivial enough though to make wary
>>> of merging them for 2.3 this late.
>> 
>> The question is really: what is gained from this series as of 2.3?  Is
>> anything actually using "-device pc87312"?  It was broken before IIUC,
>> so it can remain broken for one more version.  Markus's patches would
>> just add one more "git revert" to this series, basically.
>
> Well, my intent was to avoid adding FIXMEs by fixing it properly right
> away.

Appreciated!

>       But I don't see an urgent need as long as it's start-up errors and
> not a device_add-induced crash (not sure here, would need to re-read
> Markus' thread). Agree that it's getting late.

I'm not aware of a device_add crash with this device.

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/03/2015 16:25, Markus Armbruster wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Hello Markus et al.,
>>>
>>> This series attempts to fix the -device pc87312 issues you reported.
>>>
>>> I can't add alias properties for devices that don't get created
>>> before realize.
>>> Therefore this involves moving code for various ISA devices, to enable us
>>> to initialize the objects early for alias properties and realizing them
>>> as part of the composite device once the configuration is known, also fixing
>>> error propagation while at it. Probably needs a further iteration.
>>>
>>> Yes, it's terribly invasive, that's why the code was as it is.
>>> But the code movements are quite trivial, as long as no in-air
>>> conflicts occur.
>> 
>> Pity we have to move the state structs to the header, but that what we
>> have to do to make them embeddable.  And I understand embedding is how
>> we do sub-devices ("part of" instead of "plugged into").
>> 
>> Your changes are quite regular.  They don't look scary to me, but that
>> could be just ignorance.  They're non-trivial enough though to make wary
>> of merging them for 2.3 this late.
>
> The question is really: what is gained from this series as of 2.3?  Is
> anything actually using "-device pc87312"?  It was broken before IIUC,
> so it can remain broken for one more version.  Markus's patches would
> just add one more "git revert" to this series, basically.

My series adds four FIXME comments in two commits.  Both commits add
more of them elsewhere.

If my series goes in first, then rebasing this series on top should drop
the four FIXMEs again.

If this series goes first, then I have to rebase minr to drop the four
FIXMEs and replace an example in a commit message.

Re: [Qemu-devel] [PATCH RFC for-2.3? 0/8] prep: Fix pc87312 for -device usage

[Andreas Färber]

David,

Am 29.03.2015 um 19:53 schrieb Andreas Färber:
> Hello Markus et al.,
> 
> This series attempts to fix the -device pc87312 issues you reported.
> 
> I can't add alias properties for devices that don't get created before realize.
> Therefore this involves moving code for various ISA devices, to enable us
> to initialize the objects early for alias properties and realizing them
> as part of the composite device once the configuration is known, also fixing
> error propagation while at it. Probably needs a further iteration.
> 
> Yes, it's terribly invasive, that's why the code was as it is.
> But the code movements are quite trivial, as long as no in-air conflicts occur.

Could you take a look whether or where these proposed ISA code movements
conflict with your ISA config series? Might we need to introduce
separate -isa headers here?

Thanks,
Andreas

> 
> A follow-up would be to respin my old ISA enabled/disabled series to allow
> inactive ISADevices sitting on an ISABus.
> 
> Regards,
> Andreas
> 
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: John Snow <jsnow@redhat.com>
> 
> Andreas Färber (8):
>   parallel: Factor out header for ISAParallelState struct
>   pc87312: Create isa-parallel in-place and add alias par0-chardev
>     property
>   serial: Move ISASerialState to header
>   pc87312: Create UARTs in-place and add alias properties
>   fdb: Move FDCtrlISABus to header
>   pc87312: Create FDC in-place
>   ide: Move ISAIDEState to header
>   pc87312: Create IDE in-place
> 
>  hw/block/fdc.c             |  87 -----------------------
>  hw/char/parallel.c         |  30 +-------
>  hw/char/serial-isa.c       |  12 ----
>  hw/ide/internal.h          | 155 ----------------------------------------
>  hw/ide/isa.c               |  13 ----
>  hw/isa/pc87312.c           | 107 ++++++++++++++++------------
>  hw/ppc/prep.c              |  33 +++++++++
>  include/hw/block/fdc.h     |  88 +++++++++++++++++++++++
>  include/hw/char/parallel.h |  62 ++++++++++++++++
>  include/hw/char/serial.h   |  14 ++++
>  include/hw/ide.h           | 173 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/isa/pc87312.h   |  23 +++---
>  12 files changed, 442 insertions(+), 355 deletions(-)
>  create mode 100644 include/hw/char/parallel.h
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)