Re: [Qemu-devel] [PATCH 1/6] hw/char: QOM'ify pl011 model

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 25 May 2016 at 11:58, xiaoqiang zhao <address@hidden> wrote:
>> * drop qemu_char_get_next_serial and use chardev prop
>> * add pl011_create wrapper function to create pl011 uart device
>> * change affected board code to use the new way
>>
>> Signed-off-by: xiaoqiang zhao <address@hidden>
>> ---
>>  hw/arm/bcm2835_peripherals.c | 16 +++-----------
>>  hw/arm/highbank.c            |  3 ++-
>>  hw/arm/integratorcp.c        |  5 +++--
>>  hw/arm/realview.c            |  9 ++++----
>>  hw/arm/stellaris.c           |  6 +++--
>>  hw/arm/versatilepb.c         |  9 ++++----
>>  hw/arm/vexpress.c            |  9 ++++----
>>  hw/arm/virt.c                |  1 +
>>  hw/char/pl011.c              | 11 +++++-----
>>  include/hw/char/pl011.h      | 52 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  10 files changed, 86 insertions(+), 35 deletions(-)
>>  create mode 100644 include/hw/char/pl011.h
>>
>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>> index 234d518..46c320f 100644
>> --- a/hw/arm/bcm2835_peripherals.c
>> +++ b/hw/arm/bcm2835_peripherals.c
>> @@ -14,6 +14,7 @@
>>  #include "hw/misc/bcm2835_mbox_defs.h"
>>  #include "hw/arm/raspi_platform.h"
>>  #include "sysemu/char.h"
>> +#include "sysemu/sysemu.h"
>>
>>  /* Peripheral base address on the VC (GPU) system bus */
>>  #define BCM2835_VC_PERI_BASE 0x7e000000
>> @@ -106,7 +107,6 @@ static void bcm2835_peripherals_realize(DeviceState 
>> *dev, Error **errp)
>>      MemoryRegion *ram;
>>      Error *err = NULL;
>>      uint32_t ram_size, vcram_size;
>> -    CharDriverState *chr;
>>      int n;
>>
>>      obj = object_property_get_link(OBJECT(dev), "ram", &err);
>> @@ -147,6 +147,7 @@ static void bcm2835_peripherals_realize(DeviceState 
>> *dev, Error **errp)
>>      sysbus_pass_irq(SYS_BUS_DEVICE(s), SYS_BUS_DEVICE(&s->ic));
>>
>>      /* UART0 */
>> +    qdev_prop_set_chr(DEVICE(&s->uart0), "chardev", serial_hds[0]);
>>      object_property_set_bool(OBJECT(s->uart0), true, "realized", &err);
>>      if (err) {
>>          error_propagate(errp, err);
>> @@ -158,17 +159,8 @@ static void bcm2835_peripherals_realize(DeviceState 
>> *dev, Error **errp)
>>      sysbus_connect_irq(s->uart0, 0,
>>          qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
>>                                 INTERRUPT_UART));
>> -
>>      /* AUX / UART1 */
>> -    /* TODO: don't call qemu_char_get_next_serial() here, instead set
>> -     * chardev properties for each uart at the board level, once pl011
>> -     * (uart0) has been updated to avoid qemu_char_get_next_serial()
>> -     */
>
> This comment says this should be fixed by having board-level
> properties; you've removed it but this patch isn't adding
> the properties to this (SoC-level) device. I think the board
> level should be looking at serial_hds[], not this code.

Device models should not fish backends out of global state.  Whether
they fish directly or via some helper like qemu_char_get_next_serial()
doesn't matter.  The ones that still do need to set
cannot_instantiate_with_device_add_yet with a suitable comment.

>> @@ -310,8 +312,7 @@ static void pl011_class_init(ObjectClass *oc, void *data)
>>
>>      dc->realize = pl011_realize;
>>      dc->vmsd = &vmstate_pl011;
>> -    /* Reason: realize() method uses qemu_char_get_next_serial() */
>> -    dc->cannot_instantiate_with_device_add_yet = true;
>
> Why does instantiating with device_add work now? There's
> still no way to wire up interrupt lines or map mmio regions.
> (This has never made much sense to me -- Markus?)

Uh, which part does "this" refer to?

I systematically reviewed devices for my "Clean up and fix no_user"
series (commit f976b09..7ea5e78), and wrote down my findings in
"Reason:" comments next to cannot_instantiate_with_device_add_yet
assignments.  Any such assignment must have such a comment.

Testing can catch cases where we missed *all* reasons.  Example: my "Fix
device introspection regressions" series (commit b37686f..33fe968).  It
can't catch cases where we missed *some* reasons.

Note that cannot_instantiate_with_device_add_yet can get set by
(possibly abstract) parent devices as well.  If a parent device sets it,
its children should nevertheless set it *again* if they have additional
reasons.  I believe this is such a case.  I'm not 100% sure, because I
haven't been 100% sure about anything related to sysbus devices ever
since Alex's commit 33cd52b "sysbus: Make devices spawnable via -device"
dropped cannot_instantiate_with_device_add_yet from
sysbus_device_class_init(), quoted below.  As you see, that assignment
covered "no way to wire up interrupt lines or map mmio regions."


diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 19437e6..7bfe381 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -283,13 +283,6 @@ static void sysbus_device_class_init(ObjectClass *klass, vo
id *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = sysbus_device_init;
     k->bus_type = TYPE_SYSTEM_BUS;
-    /*
-     * device_add plugs devices into suitable bus.  For "real" buses,
-     * that actually connects the device.  For sysbus, the connections
-     * need to be made separately, and device_add can't do that.  The
-     * device would be left unconnected, and could not possibly work.
-     */
-    k->cannot_instantiate_with_device_add_yet = true;
 }