[Qemu-devel] [PATCH v1 0/2] Add a generic loader

[Qemu-devel] [PATCH v1 0/2] Add a generic loader

[Alistair Francis]

This work is based on the original work by Li Guang with extra
features added by Peter C and myself.

The idea of this loader is to allow the user to load multiple images
or values into QEMU at startup.

Memory values can be loaded like this: -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4

Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0

This can be useful and we use it a lot in Xilinx to load multiple images
into a machine at creation (ATF, Kernel and DTB for example).

It can also be used to set registers.

The limiation for arch is based off settting the ELF_ARCH macro.

The reset patch is required otherwise the reset will never be registered
and the loader can't change the PC in the case of images.

Changes since RFC:
 - Add support for BE


Alistair Francis (2):
  qdev-monitor.c: Register reset function if the device has one
  generic-loader: Add a generic loader

 default-configs/arm-softmmu.mak  |   1 +
 hw/misc/Makefile.objs            |   2 +
 hw/misc/generic-loader.c         | 127 +++++++++++++++++++++++++++++++++++++++
 include/hw/misc/generic-loader.h |  50 +++++++++++++++
 qdev-monitor.c                   |   2 +
 5 files changed, 182 insertions(+)
 create mode 100644 hw/misc/generic-loader.c
 create mode 100644 include/hw/misc/generic-loader.h

-- 
2.5.0

[Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Alistair Francis]

If the device being added when running qdev_device_add() has
a reset function, register it so that it can be called.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 qdev-monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 81e3ff3..0a99d01 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 
     if (bus) {
         qdev_set_parent_bus(dev, bus);
+    } else if (dc->reset) {
+        qemu_register_reset((void (*)(void *))dc->reset, dev);
     }
 
     id = qemu_opts_id(opts);
-- 
2.5.0

[Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Alistair Francis]

Add a generic loader to QEMU which can be used to load images or set
memory values.

This only supports ARM architectures at the moment.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
Changes since RFC:
 - Add BE support

 default-configs/arm-softmmu.mak  |   1 +
 hw/misc/Makefile.objs            |   2 +
 hw/misc/generic-loader.c         | 127 +++++++++++++++++++++++++++++++++++++++
 include/hw/misc/generic-loader.h |  50 +++++++++++++++
 4 files changed, 180 insertions(+)
 create mode 100644 hw/misc/generic-loader.c
 create mode 100644 include/hw/misc/generic-loader.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index a9f82a1..b246b75 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -110,3 +110,4 @@ CONFIG_IOH3420=y
 CONFIG_I82801B11=y
 CONFIG_ACPI=y
 CONFIG_SMBIOS=y
+CONFIG_LOADER=y
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index ea6cd3c..9f05dcf 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -46,3 +46,5 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
 obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_EDU) += edu.o
 obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
+
+obj-$(CONFIG_LOADER) += generic-loader.o
diff --git a/hw/misc/generic-loader.c b/hw/misc/generic-loader.c
new file mode 100644
index 0000000..0c52c6a
--- /dev/null
+++ b/hw/misc/generic-loader.c
@@ -0,0 +1,127 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Written by Li Guang <lig.fnst@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+#include "hw/loader.h"
+#include "hw/misc/generic-loader.h"
+
+#define CPU_NONE 0xFF
+
+static void generic_loader_reset(DeviceState *dev)
+{
+    GenericLoaderState *s = GENERIC_LOADER(dev);
+
+    if (s->cpu) {
+        CPUClass *cc = CPU_GET_CLASS(s->cpu);
+        cpu_reset(s->cpu);
+        cc->set_pc(s->cpu, s->addr);
+    }
+
+    if (s->data_len) {
+        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data,
+                         s->data_len);
+    }
+}
+
+static void generic_loader_realize(DeviceState *dev, Error **errp)
+{
+    GenericLoaderState *s = GENERIC_LOADER(dev);
+    hwaddr entry;
+    int big_endian;
+    int size = 0;
+
+    if (s->cpu_nr != CPU_NONE) {
+        CPUState *cs = first_cpu;
+        int cpu_num = 0;
+
+        CPU_FOREACH(cs) {
+            if (cpu_num == s->cpu_nr) {
+                s->cpu = cs;
+                break;
+            } else if (!CPU_NEXT(cs)) {
+                error_setg(errp, "Specified boot CPU#%d is non existant",
+                           s->cpu_nr);
+                return;
+            } else {
+                cpu_num++;
+            }
+        }
+    }
+
+#ifdef TARGET_WORDS_BIGENDIAN
+    big_endian = 1;
+#else
+    big_endian = 0;
+#endif
+
+    if (s->file) {
+        if (!s->force_raw) {
+            size = load_elf(s->file, NULL, NULL, &entry, NULL, NULL,
+                            big_endian, ELF_ARCH, 0);
+
+            if (size < 0) {
+                size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL);
+            }
+        }
+
+        if (size < 0) {
+            size = load_image_targphys(s->file, s->addr, 0);
+        } else {
+            s->addr = entry;
+        }
+
+        if (size < 0) {
+            error_setg(errp, "Cannot load specified image %s", s->file);
+            return;
+        }
+    }
+}
+
+static Property generic_loader_props[] = {
+    DEFINE_PROP_UINT64("addr", GenericLoaderState, addr, 0),
+    DEFINE_PROP_UINT64("data", GenericLoaderState, data, 0),
+    DEFINE_PROP_UINT8("data-len", GenericLoaderState, data_len, 0),
+    DEFINE_PROP_UINT8("cpu", GenericLoaderState, cpu_nr, CPU_NONE),
+    DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
+    DEFINE_PROP_STRING("file", GenericLoaderState, file),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void generic_loader_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = generic_loader_reset;
+    dc->realize = generic_loader_realize;
+    dc->props = generic_loader_props;
+    dc->desc = "Generic Loader";
+}
+
+static TypeInfo generic_loader_info = {
+    .name = TYPE_GENERIC_LOADER,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(GenericLoaderState),
+    .class_init = generic_loader_class_init,
+};
+
+static void generic_loader_register_type(void)
+{
+    type_register_static(&generic_loader_info);
+}
+
+type_init(generic_loader_register_type)
diff --git a/include/hw/misc/generic-loader.h b/include/hw/misc/generic-loader.h
new file mode 100644
index 0000000..79b5536
--- /dev/null
+++ b/include/hw/misc/generic-loader.h
@@ -0,0 +1,50 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Written by Li Guang <lig.fnst@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#ifndef GENERIC_LOADER_H
+#define GENERIC_LOADER_H
+
+#include "elf.h"
+
+#if   defined(TARGET_AARCH64)
+    #define ELF_ARCH        EM_AARCH64
+#elif defined(TARGET_ARM)
+    #define ELF_ARCH        EM_ARM
+#endif
+
+typedef struct GenericLoaderState {
+    /* <private> */
+    DeviceState parent_obj;
+
+    /* <public> */
+    CPUState *cpu;
+
+    uint64_t addr;
+    uint64_t data;
+    uint8_t data_len;
+    uint8_t cpu_nr;
+
+    char *file;
+
+    bool force_raw;
+} GenericLoaderState;
+
+#define TYPE_GENERIC_LOADER "loader"
+#define GENERIC_LOADER(obj) OBJECT_CHECK(GenericLoaderState, (obj), \
+                                         TYPE_GENERIC_LOADER)
+
+#endif
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

On 02/17/2016 02:04 PM, Alistair Francis wrote:
> Add a generic loader to QEMU which can be used to load images or set
> memory values.
> 
> This only supports ARM architectures at the moment.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> Changes since RFC:
>  - Add BE support
> 

>  hw/misc/generic-loader.c         | 127 +++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/generic-loader.h |  50 +++++++++++++++
>  4 files changed, 180 insertions(+)
>  create mode 100644 hw/misc/generic-loader.c
>  create mode 100644 include/hw/misc/generic-loader.h

We really ought to improve checkpatch.pl to flag patches that add new
files not covered by MAINTAINERS.

> +++ b/hw/misc/generic-loader.c
> @@ -0,0 +1,127 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>

Want to claim 2016?

> 
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +#include "hw/loader.h"
> +#include "hw/misc/generic-loader.h"

New .c files should include "qemu/osdep.h" first, before anything else.

> +static void generic_loader_realize(DeviceState *dev, Error **errp)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(dev);
> +    hwaddr entry;
> +    int big_endian;
> +    int size = 0;
> +
> +    if (s->cpu_nr != CPU_NONE) {
> +        CPUState *cs = first_cpu;
> +        int cpu_num = 0;
> +
> +        CPU_FOREACH(cs) {
> +            if (cpu_num == s->cpu_nr) {
> +                s->cpu = cs;
> +                break;
> +            } else if (!CPU_NEXT(cs)) {
> +                error_setg(errp, "Specified boot CPU#%d is non existant",
> +                           s->cpu_nr);

s/non existant/nonexistent/


> +++ b/include/hw/misc/generic-loader.h
> @@ -0,0 +1,50 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>

2016

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Alistair Francis]

On Wed, Feb 17, 2016 at 1:41 PM, Eric Blake <eblake@redhat.com> wrote:
> On 02/17/2016 02:04 PM, Alistair Francis wrote:
>> Add a generic loader to QEMU which can be used to load images or set
>> memory values.
>>
>> This only supports ARM architectures at the moment.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> Changes since RFC:
>>  - Add BE support
>>
>
>>  hw/misc/generic-loader.c         | 127 +++++++++++++++++++++++++++++++++++++++
>>  include/hw/misc/generic-loader.h |  50 +++++++++++++++
>>  4 files changed, 180 insertions(+)
>>  create mode 100644 hw/misc/generic-loader.c
>>  create mode 100644 include/hw/misc/generic-loader.h
>
> We really ought to improve checkpatch.pl to flag patches that add new
> files not covered by MAINTAINERS.

Adding an entry for this.

>
>> +++ b/hw/misc/generic-loader.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * Generic Loader
>> + *
>> + * Copyright (C) 2014 Li Guang
>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
>
> Want to claim 2016?

Yep, I can do that. I'm never too sure when this can be changed or
not. Should I add a written by as well?

>
>>
>> +
>> +#include "hw/sysbus.h"
>> +#include "sysemu/dma.h"
>> +#include "hw/loader.h"
>> +#include "hw/misc/generic-loader.h"
>
> New .c files should include "qemu/osdep.h" first, before anything else.

Adding it

>
>> +static void generic_loader_realize(DeviceState *dev, Error **errp)
>> +{
>> +    GenericLoaderState *s = GENERIC_LOADER(dev);
>> +    hwaddr entry;
>> +    int big_endian;
>> +    int size = 0;
>> +
>> +    if (s->cpu_nr != CPU_NONE) {
>> +        CPUState *cs = first_cpu;
>> +        int cpu_num = 0;
>> +
>> +        CPU_FOREACH(cs) {
>> +            if (cpu_num == s->cpu_nr) {
>> +                s->cpu = cs;
>> +                break;
>> +            } else if (!CPU_NEXT(cs)) {
>> +                error_setg(errp, "Specified boot CPU#%d is non existant",
>> +                           s->cpu_nr);
>
> s/non existant/nonexistent/

Thanks, fixed

Thanks,

Alistair

>
>
>> +++ b/include/hw/misc/generic-loader.h
>> @@ -0,0 +1,50 @@
>> +/*
>> + * Generic Loader
>> + *
>> + * Copyright (C) 2014 Li Guang
>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
>
> 2016
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

On 02/17/2016 05:03 PM, Alistair Francis wrote:

>>> +++ b/hw/misc/generic-loader.c
>>> @@ -0,0 +1,127 @@
>>> +/*
>>> + * Generic Loader
>>> + *
>>> + * Copyright (C) 2014 Li Guang
>>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
>>
>> Want to claim 2016?
> 
> Yep, I can do that. I'm never too sure when this can be changed or
> not. Should I add a written by as well?

I'm not a lawyer, so my response may not be authoritative; in
particular, your employer may have specific rules that you must follow
for any code you submit that was written on your employer's time, and
that trumps anything I say here (that is, trust your lawyers more than
you trust me).

But in general, I tend to go by the simple rule of listing the first
year that any of the code was first developed (if you are copying
significant portions from some other file, then use the starting year
from that file, even if your file didn't exist back then), through the
current year, if my change is non-trivial (more than 10 lines, or
altering an interface), while ignoring the issue for trivial things
(such as fixing a typo, or doing a bulk search-and-replace across the
tree).  As for an authorship line, I tend to omit those (they quickly go
stale, and git history is sufficient for a much more accurate picture);
the copyright line is more important legally than any author line.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Alistair Francis]

On Wed, Feb 17, 2016 at 4:11 PM, Eric Blake <eblake@redhat.com> wrote:
> On 02/17/2016 05:03 PM, Alistair Francis wrote:
>
>>>> +++ b/hw/misc/generic-loader.c
>>>> @@ -0,0 +1,127 @@
>>>> +/*
>>>> + * Generic Loader
>>>> + *
>>>> + * Copyright (C) 2014 Li Guang
>>>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
>>>
>>> Want to claim 2016?
>>
>> Yep, I can do that. I'm never too sure when this can be changed or
>> not. Should I add a written by as well?
>
> I'm not a lawyer, so my response may not be authoritative; in
> particular, your employer may have specific rules that you must follow
> for any code you submit that was written on your employer's time, and
> that trumps anything I say here (that is, trust your lawyers more than
> you trust me).
>
> But in general, I tend to go by the simple rule of listing the first
> year that any of the code was first developed (if you are copying
> significant portions from some other file, then use the starting year
> from that file, even if your file didn't exist back then), through the
> current year, if my change is non-trivial (more than 10 lines, or
> altering an interface), while ignoring the issue for trivial things
> (such as fixing a typo, or doing a bulk search-and-replace across the
> tree).  As for an authorship line, I tend to omit those (they quickly go
> stale, and git history is sufficient for a much more accurate picture);
> the copyright line is more important legally than any author line.

Ok, I have added a Xilinx copyright line and not bothered with a
written by line.

Thanks for your help.

Thanks,

Alistair

>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Markus Armbruster]

Alistair Francis <alistair.francis@xilinx.com> writes:

> If the device being added when running qdev_device_add() has
> a reset function, register it so that it can be called.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  qdev-monitor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 81e3ff3..0a99d01 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  
>      if (bus) {
>          qdev_set_parent_bus(dev, bus);
> +    } else if (dc->reset) {
> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>      }
>  
>      id = qemu_opts_id(opts);

This looks wrong to me.

You stuff all the device reset methods into the global reset_handlers
list, where they get called in some semi-random order.  This breaks when
there are reset order dependencies between devices, e.g. between a
device and the bus it plugs into.

Propagating the reset signal to all the devices is a qdev problem.
Copying Andreas for further insight.

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Hollis Blanchard]

On 02/17/2016 01:04 PM, Alistair Francis wrote:
> +static void generic_loader_reset(DeviceState *dev)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(dev);
> +
> +    if (s->cpu) {
> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
> +        cpu_reset(s->cpu);
> +        cc->set_pc(s->cpu, s->addr);
> +    }
> +
> +    if (s->data_len) {
> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data,
> +                         s->data_len);
> +    }
> +}

What happens if I accidentally make "data-len" bigger than 
sizeof(s->data)? I think some bounds checking is needed?

Hollis Blanchard
Mentor Graphics Emulation Division

Re: [Qemu-devel] [PATCH v1 0/2] Add a generic loader

[Hollis Blanchard]

This cover letter has some useful explanation. Perhaps some of the 
contents should go into a docs/ file so other people can enjoy it too? :-)

I understand the part about loading multiple files, but why would I want 
to load a kernel with this loader, instead of the -kernel option?

Hollis Blanchard
Mentor Graphics Emulation Division

On 02/17/2016 01:04 PM, Alistair Francis wrote:
> This work is based on the original work by Li Guang with extra
> features added by Peter C and myself.
>
> The idea of this loader is to allow the user to load multiple images
> or values into QEMU at startup.
>
> Memory values can be loaded like this: -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>
> Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0
>
> This can be useful and we use it a lot in Xilinx to load multiple images
> into a machine at creation (ATF, Kernel and DTB for example).
>
> It can also be used to set registers.
>
> The limiation for arch is based off settting the ELF_ARCH macro.
>
> The reset patch is required otherwise the reset will never be registered
> and the loader can't change the PC in the case of images.
>
> Changes since RFC:
>   - Add support for BE
>
>
> Alistair Francis (2):
>    qdev-monitor.c: Register reset function if the device has one
>    generic-loader: Add a generic loader
>
>   default-configs/arm-softmmu.mak  |   1 +
>   hw/misc/Makefile.objs            |   2 +
>   hw/misc/generic-loader.c         | 127 +++++++++++++++++++++++++++++++++++++++
>   include/hw/misc/generic-loader.h |  50 +++++++++++++++
>   qdev-monitor.c                   |   2 +
>   5 files changed, 182 insertions(+)
>   create mode 100644 hw/misc/generic-loader.c
>   create mode 100644 include/hw/misc/generic-loader.h
>

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Alistair Francis]

On Thu, Feb 18, 2016 at 1:56 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> If the device being added when running qdev_device_add() has
>> a reset function, register it so that it can be called.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81e3ff3..0a99d01 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (dc->reset) {
>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>      }
>>
>>      id = qemu_opts_id(opts);
>
> This looks wrong to me.
>
> You stuff all the device reset methods into the global reset_handlers
> list, where they get called in some semi-random order.  This breaks when
> there are reset order dependencies between devices, e.g. between a
> device and the bus it plugs into.

So I'm not a expert on this, but from what I see this will also be
added near the end of the list (before the rom_resets though) and
doesn't seem to be a problem. Do you have any other ideas how the
reset function could be registered?

Thanks,

Alistair

>
> Propagating the reset signal to all the devices is a qdev problem.
> Copying Andreas for further insight.
>

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Alistair Francis]

On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard
<hollis_blanchard@mentor.com> wrote:
> On 02/17/2016 01:04 PM, Alistair Francis wrote:
>>
>> +static void generic_loader_reset(DeviceState *dev)
>> +{
>> +    GenericLoaderState *s = GENERIC_LOADER(dev);
>> +
>> +    if (s->cpu) {
>> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
>> +        cpu_reset(s->cpu);
>> +        cc->set_pc(s->cpu, s->addr);
>> +    }
>> +
>> +    if (s->data_len) {
>> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr,
>> &s->data,
>> +                         s->data_len);
>> +    }
>> +}
>
>
> What happens if I accidentally make "data-len" bigger than sizeof(s->data)?
> I think some bounds checking is needed?

Good point! I'll add an assert as it isn't a recoverable error.

Thanks,

Alistair

>
> Hollis Blanchard
> Mentor Graphics Emulation Division
>

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Hollis Blanchard]

On 02/18/2016 10:49 AM, Alistair Francis wrote:
> On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard
> <hollis_blanchard@mentor.com> wrote:
>> On 02/17/2016 01:04 PM, Alistair Francis wrote:
>>> +static void generic_loader_reset(DeviceState *dev)
>>> +{
>>> +    GenericLoaderState *s = GENERIC_LOADER(dev);
>>> +
>>> +    if (s->cpu) {
>>> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
>>> +        cpu_reset(s->cpu);
>>> +        cc->set_pc(s->cpu, s->addr);
>>> +    }
>>> +
>>> +    if (s->data_len) {
>>> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr,
>>> &s->data,
>>> +                         s->data_len);
>>> +    }
>>> +}
>>
>> What happens if I accidentally make "data-len" bigger than sizeof(s->data)?
>> I think some bounds checking is needed?
> Good point! I'll add an assert as it isn't a recoverable error.

Perhaps a more user-friendly error message would be, well, more 
user-friendly. :-) That could be done when reading the "data-len" 
property, in addition to an assert when using s->data_len.

Hollis Blanchard
Mentor Graphics Emulation Division

Re: [Qemu-devel] [PATCH v1 0/2] Add a generic loader

[Alistair Francis]

On Thu, Feb 18, 2016 at 10:27 AM, Hollis Blanchard
<hollis_blanchard@mentor.com> wrote:
> This cover letter has some useful explanation. Perhaps some of the contents
> should go into a docs/ file so other people can enjoy it too? :-)

Ok, I have created a docs file based on the cover letter

>
> I understand the part about loading multiple files, but why would I want to
> load a kernel with this loader, instead of the -kernel option?

If you just want to load a kernel, you should use the -kernel option.
The advantage with this is when you need multiple images. For example:
 - Load ATF and let it run
 - Hands off to a simple loader that sets some registers and drops EL
 - Start Linux

To be able to perform the steps above all three images and the DTB
need to already be loaded. This allows us to boot Linux on ATF without
u-boot. That is just one example, there are others. Mostly it comes
down to using ATF and something.

We also use the register write to avoid having to run first stage boot
loaders, as the kernel expects this registers to be set but we don't
want to run the whole boot loader.

It also allows images to be loaded later in the boot process from the
monitor. We use this for some other testing as well.

Thanks,

Alistair

>
> Hollis Blanchard
> Mentor Graphics Emulation Division
>
>
> On 02/17/2016 01:04 PM, Alistair Francis wrote:
>>
>> This work is based on the original work by Li Guang with extra
>> features added by Peter C and myself.
>>
>> The idea of this loader is to allow the user to load multiple images
>> or values into QEMU at startup.
>>
>> Memory values can be loaded like this: -device
>> loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
>>
>> Images can be loaded like this: -device
>> loader,file=./images/u-boot.elf,cpu=0
>>
>> This can be useful and we use it a lot in Xilinx to load multiple images
>> into a machine at creation (ATF, Kernel and DTB for example).
>>
>> It can also be used to set registers.
>>
>> The limiation for arch is based off settting the ELF_ARCH macro.
>>
>> The reset patch is required otherwise the reset will never be registered
>> and the loader can't change the PC in the case of images.
>>
>> Changes since RFC:
>>   - Add support for BE
>>
>>
>> Alistair Francis (2):
>>    qdev-monitor.c: Register reset function if the device has one
>>    generic-loader: Add a generic loader
>>
>>   default-configs/arm-softmmu.mak  |   1 +
>>   hw/misc/Makefile.objs            |   2 +
>>   hw/misc/generic-loader.c         | 127
>> +++++++++++++++++++++++++++++++++++++++
>>   include/hw/misc/generic-loader.h |  50 +++++++++++++++
>>   qdev-monitor.c                   |   2 +
>>   5 files changed, 182 insertions(+)
>>   create mode 100644 hw/misc/generic-loader.c
>>   create mode 100644 include/hw/misc/generic-loader.h
>>
>
>

Re: [Qemu-devel] [PATCH v1 2/2] generic-loader: Add a generic loader

[Alistair Francis]

On Thu, Feb 18, 2016 at 10:58 AM, Hollis Blanchard
<hollis_blanchard@mentor.com> wrote:
> On 02/18/2016 10:49 AM, Alistair Francis wrote:
>>
>> On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard
>> <hollis_blanchard@mentor.com> wrote:
>>>
>>> On 02/17/2016 01:04 PM, Alistair Francis wrote:
>>>>
>>>> +static void generic_loader_reset(DeviceState *dev)
>>>> +{
>>>> +    GenericLoaderState *s = GENERIC_LOADER(dev);
>>>> +
>>>> +    if (s->cpu) {
>>>> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
>>>> +        cpu_reset(s->cpu);
>>>> +        cc->set_pc(s->cpu, s->addr);
>>>> +    }
>>>> +
>>>> +    if (s->data_len) {
>>>> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr,
>>>> &s->data,
>>>> +                         s->data_len);
>>>> +    }
>>>> +}
>>>
>>>
>>> What happens if I accidentally make "data-len" bigger than
>>> sizeof(s->data)?
>>> I think some bounds checking is needed?
>>
>> Good point! I'll add an assert as it isn't a recoverable error.
>
>
> Perhaps a more user-friendly error message would be, well, more
> user-friendly. :-) That could be done when reading the "data-len" property,
> in addition to an assert when using s->data_len.

Fair enough, there is now a more appropriate check in the realise function.

Thanks,

Alistair

>
>
> Hollis Blanchard
> Mentor Graphics Emulation Division
>

Re: [Qemu-devel] [PATCH v1 0/2] Add a generic loader

[Peter Maydell]

On 18 February 2016 at 19:02, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Feb 18, 2016 at 10:27 AM, Hollis Blanchard
> <hollis_blanchard@mentor.com> wrote:
>> I understand the part about loading multiple files, but why would I want to
>> load a kernel with this loader, instead of the -kernel option?
>
> If you just want to load a kernel, you should use the -kernel option.
> The advantage with this is when you need multiple images. For example:
>  - Load ATF and let it run
>  - Hands off to a simple loader that sets some registers and drops EL
>  - Start Linux
>
> To be able to perform the steps above all three images and the DTB
> need to already be loaded. This allows us to boot Linux on ATF without
> u-boot. That is just one example, there are others. Mostly it comes
> down to using ATF and something.

FWIW, the ATF stack we have for the 'virt' board uses the semihosting
API to load the later stage blobs. (That's not saying we don't need
this series.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Paolo Bonzini]

On 18/02/2016 10:56, Markus Armbruster wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
> 
>> If the device being added when running qdev_device_add() has
>> a reset function, register it so that it can be called.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81e3ff3..0a99d01 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>  
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (dc->reset) {
>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>      }
>>  
>>      id = qemu_opts_id(opts);
> 
> This looks wrong to me.
> 
> You stuff all the device reset methods into the global reset_handlers
> list, where they get called in some semi-random order.  This breaks when
> there are reset order dependencies between devices, e.g. between a
> device and the bus it plugs into.

There is no bus here, see the "if" above the one that's being added.

However, what devices have done so far is to register/unregister the
reset in the realize/unrealize methods, and I suggest doing the same.

If you really want to add the magic qemu_register_reset, you should at
least do one of the following:

* add a matching unregister (no idea where)

* assert that the device is not hot-unpluggable, otherwise hot-unplug
would leave a dangling pointer.

Paolo

> Propagating the reset signal to all the devices is a qdev problem.
> Copying Andreas for further insight.

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Peter Crosthwaite]

On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/02/2016 10:56, Markus Armbruster wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> If the device being added when running qdev_device_add() has
>>> a reset function, register it so that it can be called.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  qdev-monitor.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 81e3ff3..0a99d01 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>
>>>      if (bus) {
>>>          qdev_set_parent_bus(dev, bus);
>>> +    } else if (dc->reset) {
>>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>>      }
>>>
>>>      id = qemu_opts_id(opts);
>>
>> This looks wrong to me.
>>
>> You stuff all the device reset methods into the global reset_handlers
>> list, where they get called in some semi-random order.  This breaks when
>> there are reset order dependencies between devices, e.g. between a
>> device and the bus it plugs into.
>
> There is no bus here, see the "if" above the one that's being added.
>
> However, what devices have done so far is to register/unregister the
> reset in the realize/unrealize methods, and I suggest doing the same.
>

Does this assume the device itself knows whether it is bus-connected
or not? This way has the advantage of catchall-ing devices that have
no bus connected and the device may or may not know whether it is
bus-connected (nor should it need to know). Probably doesn't have in
tree precedent yet, but I thought we wanted to move away from
qdev/qbus managing the device-tree. So ideally, the new else should
become unconditional long term once we debusify (and properly QOMify)
the reset tree (and the if goes away).

> If you really want to add the magic qemu_register_reset, you should at
> least do one of the following:
>
> * add a matching unregister (no idea where)
>

You could add a boolean flag to DeviceState that is set by this
registration. It can then be checked at unrealize to remove reset
handler.

Regards,
Peter

> * assert that the device is not hot-unpluggable, otherwise hot-unplug
> would leave a dangling pointer.
>
> Paolo
>
>> Propagating the reset signal to all the devices is a qdev problem.
>> Copying Andreas for further insight.

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Alistair Francis]

On Thu, Feb 18, 2016 at 3:07 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 18/02/2016 10:56, Markus Armbruster wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>>> If the device being added when running qdev_device_add() has
>>>> a reset function, register it so that it can be called.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>>  qdev-monitor.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>>> index 81e3ff3..0a99d01 100644
>>>> --- a/qdev-monitor.c
>>>> +++ b/qdev-monitor.c
>>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>>
>>>>      if (bus) {
>>>>          qdev_set_parent_bus(dev, bus);
>>>> +    } else if (dc->reset) {
>>>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>>>      }
>>>>
>>>>      id = qemu_opts_id(opts);
>>>
>>> This looks wrong to me.
>>>
>>> You stuff all the device reset methods into the global reset_handlers
>>> list, where they get called in some semi-random order.  This breaks when
>>> there are reset order dependencies between devices, e.g. between a
>>> device and the bus it plugs into.
>>
>> There is no bus here, see the "if" above the one that's being added.
>>
>> However, what devices have done so far is to register/unregister the
>> reset in the realize/unrealize methods, and I suggest doing the same.

Ok, I am happy to do it that way. It just seemed dodgy to me
registering the reset in the realise.

This also seemed like a feature worth having, as I thought this would
come up again in the future.

>>
>
> Does this assume the device itself knows whether it is bus-connected
> or not? This way has the advantage of catchall-ing devices that have
> no bus connected and the device may or may not know whether it is
> bus-connected (nor should it need to know). Probably doesn't have in
> tree precedent yet, but I thought we wanted to move away from
> qdev/qbus managing the device-tree. So ideally, the new else should
> become unconditional long term once we debusify (and properly QOMify)
> the reset tree (and the if goes away).

That was my general thinking as well.

Thanks,

Alistair

>
>> If you really want to add the magic qemu_register_reset, you should at
>> least do one of the following:
>>
>> * add a matching unregister (no idea where)
>>
>
> You could add a boolean flag to DeviceState that is set by this
> registration. It can then be checked at unrealize to remove reset
> handler.
>
> Regards,
> Peter
>
>> * assert that the device is not hot-unpluggable, otherwise hot-unplug
>> would leave a dangling pointer.
>>
>> Paolo
>>
>>> Propagating the reset signal to all the devices is a qdev problem.
>>> Copying Andreas for further insight.
>

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Paolo Bonzini]

On 19/02/2016 00:07, Peter Crosthwaite wrote:
> On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 18/02/2016 10:56, Markus Armbruster wrote:
>>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>>
>>>> If the device being added when running qdev_device_add() has
>>>> a reset function, register it so that it can be called.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>>  qdev-monitor.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>>> index 81e3ff3..0a99d01 100644
>>>> --- a/qdev-monitor.c
>>>> +++ b/qdev-monitor.c
>>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>>
>>>>      if (bus) {
>>>>          qdev_set_parent_bus(dev, bus);
>>>> +    } else if (dc->reset) {
>>>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>>>      }
>>>>
>>>>      id = qemu_opts_id(opts);
>>>
>>> This looks wrong to me.
>>>
>>> You stuff all the device reset methods into the global reset_handlers
>>> list, where they get called in some semi-random order.  This breaks when
>>> there are reset order dependencies between devices, e.g. between a
>>> device and the bus it plugs into.
>>
>> There is no bus here, see the "if" above the one that's being added.
>>
>> However, what devices have done so far is to register/unregister the
>> reset in the realize/unrealize methods, and I suggest doing the same.
> 
> Does this assume the device itself knows whether it is bus-connected
> or not? This way has the advantage of catchall-ing devices that have
> no bus connected and the device may or may not know whether it is
> bus-connected (nor should it need to know).

A device almost definitely needs to know if it is bus connected.  More
likely than not, a busless device inherits from DeviceState while a
device with a bus inherits from SCSIDevice, PCIDevice, I2CSlave, etc.

> Probably doesn't have in
> tree precedent yet, but I thought we wanted to move away from
> qdev/qbus managing the device-tree. So ideally, the new else should
> become unconditional long term once we debusify (and properly QOMify)
> the reset tree (and the if goes away).

Any abstraction we have in QEMU should have at least a parallel (though
it need not be the same) in real hardware.  Reset signals _do_ propagate
along buses, or at least along some buses, so "debusifying" reset seems
like a counterproductive goal to me.

For busless devices, I thought the idea was just to have the QOM parent
(container) propagate the realize/reset/unrealize signals in the right
order.  Unfortunately reality is not quite as simple and indeed here
however you have a busless device that:

- doesn't have a container (the container is just /machine/unattached or
similar).

- triggers a reset for something else that is not contained in it (the
CPU) and even some DMA.

>> If you really want to add the magic qemu_register_reset, you should at
>> least do one of the following:
>>
>> * add a matching unregister (no idea where)
> 
> You could add a boolean flag to DeviceState that is set by this
> registration. It can then be checked at unrealize to remove reset
> handler.

Yeah, I guess that would work.  A few changes:

- you register the callback unconditionally for all busless devices,
using qdev_reset_all_fn instead of directly dc->reset

- you do it after the "realized" property has been set successfully.
Otherwise, a failed -device or device_add will also leave a dangling
callback.

- add a comment that this is because the callback is registered because
this is a busless _and_ container-less device

Paolo

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/02/2016 10:56, Markus Armbruster wrote:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>> 
>>> If the device being added when running qdev_device_add() has
>>> a reset function, register it so that it can be called.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  qdev-monitor.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 81e3ff3..0a99d01 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>  
>>>      if (bus) {
>>>          qdev_set_parent_bus(dev, bus);
>>> +    } else if (dc->reset) {
>>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>>      }
>>>  
>>>      id = qemu_opts_id(opts);
>> 
>> This looks wrong to me.
>> 
>> You stuff all the device reset methods into the global reset_handlers
>> list, where they get called in some semi-random order.  This breaks when
>> there are reset order dependencies between devices, e.g. between a
>> device and the bus it plugs into.
>
> There is no bus here, see the "if" above the one that's being added.
>
> However, what devices have done so far is to register/unregister the
> reset in the realize/unrealize methods, and I suggest doing the same.

Shows that our support for bus-less devices is still in its infancy.

For me, a device has a number of external connectors that need to be
wired up.  A qdev bus is merely a standardized package of such
connectors.  For historical reasons, buses are all qdev has, with a
special sysbus for everything that cannot be shoehorned into a single
bus.  To work with individual connectors, you have to drop into C
(ignoring the weird "platform bus" sysbus thing Alex added).  Support
for doing that at configuration rather than code level would be nice.

[...]

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Peter Maydell]

On 19 February 2016 at 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Any abstraction we have in QEMU should have at least a parallel (though
> it need not be the same) in real hardware.  Reset signals _do_ propagate
> along buses, or at least along some buses, so "debusifying" reset seems
> like a counterproductive goal to me.

Reset for some buses propagates along buses, but not in all
cases. In any case our current "reset" semantics are "power on
reset", not any kind of driven-by-hardware-reset-lines reset.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Andreas Färber]

Am 18.02.2016 um 10:56 schrieb Markus Armbruster:
> Alistair Francis <alistair.francis@xilinx.com> writes:
> 
>> If the device being added when running qdev_device_add() has
>> a reset function, register it so that it can be called.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81e3ff3..0a99d01 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>  
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (dc->reset) {
>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>      }
>>  
>>      id = qemu_opts_id(opts);
> 
> This looks wrong to me.
> 
> You stuff all the device reset methods into the global reset_handlers
> list, where they get called in some semi-random order.  This breaks when
> there are reset order dependencies between devices, e.g. between a
> device and the bus it plugs into.
> 
> Propagating the reset signal to all the devices is a qdev problem.
> Copying Andreas for further insight.

We had a similar discussion for s390x, and I had started a big reset
refactoring, but we agreed to go for a stop-gap solution for 2.5. The
recently posted hw/core/bus.c refactoring originated from that branch.

The overall idea was that for buses reset propagates along buses (so
yes, NACK to this patch), but where no bus exists it shall propagate to
QOM children, too.

So Alistair, if you have a device that needs a reset while not sitting
on a bus, please register the register hook in your device's realize
hook for now.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Paolo Bonzini]

On 19/02/2016 11:55, Peter Maydell wrote:
>> Any abstraction we have in QEMU should have at least a parallel (though
>> > it need not be the same) in real hardware.  Reset signals _do_ propagate
>> > along buses, or at least along some buses, so "debusifying" reset seems
>> > like a counterproductive goal to me.
> Reset for some buses propagates along buses, but not in all
> cases.

Agreed.  But still this doesn't change the fact that "debusifying" reset
is a non-goal.

> In any case our current "reset" semantics are "power on
> reset", not any kind of driven-by-hardware-reset-lines reset.

It depends.  There are several cases (PCI, SCSI) where qdev_reset_all is
used from within the code in order to provide hardware reset semantics.

Paolo

Re: [Qemu-devel] [PATCH v1 1/2] qdev-monitor.c: Register reset function if the device has one

[Alistair Francis]

On Fri, Feb 19, 2016 at 9:15 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 18.02.2016 um 10:56 schrieb Markus Armbruster:
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> If the device being added when running qdev_device_add() has
>>> a reset function, register it so that it can be called.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  qdev-monitor.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 81e3ff3..0a99d01 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>>
>>>      if (bus) {
>>>          qdev_set_parent_bus(dev, bus);
>>> +    } else if (dc->reset) {
>>> +        qemu_register_reset((void (*)(void *))dc->reset, dev);
>>>      }
>>>
>>>      id = qemu_opts_id(opts);
>>
>> This looks wrong to me.
>>
>> You stuff all the device reset methods into the global reset_handlers
>> list, where they get called in some semi-random order.  This breaks when
>> there are reset order dependencies between devices, e.g. between a
>> device and the bus it plugs into.
>>
>> Propagating the reset signal to all the devices is a qdev problem.
>> Copying Andreas for further insight.
>
> We had a similar discussion for s390x, and I had started a big reset
> refactoring, but we agreed to go for a stop-gap solution for 2.5. The
> recently posted hw/core/bus.c refactoring originated from that branch.
>
> The overall idea was that for buses reset propagates along buses (so
> yes, NACK to this patch), but where no bus exists it shall propagate to
> QOM children, too.
>
> So Alistair, if you have a device that needs a reset while not sitting
> on a bus, please register the register hook in your device's realize
> hook for now.

Ok, that is fair. I have moved the reset register/unregister to the
realise/unrealise functions in V2.

Thanks,

Alistair

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