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

2016-09-29 Thread Alistair Francis
On Thu, Sep 29, 2016 at 2:32 AM, Markus Armbruster  wrote:
> Alistair Francis  writes:
>
>> Add a generic loader to QEMU which can be used to load images or set
>> memory values.
>>
>> Internally inside QEMU this is a device. It is a strange device that
>> provides no hardware interface but allows QEMU to monkey patch memory
>> specified when it is created. To be able to do this it has a reset
>> callback that does the memory operations.
>>
>> This device allows the user to monkey patch memory. To be able to do
>> this it needs a backend to manage the datas, the same as other
>> memory-related devices. In this case as the backend is so trivial we
>> have merged it with the frontend instead of creating and maintaining a
>> seperate backend.
>
> Works for me.

Great!

>
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Peter Maydell 
> [...]
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> new file mode 100644
>> index 000..fc2fea7
>> --- /dev/null
>> +++ b/hw/core/generic-loader.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * Generic Loader
>> + *
>> + * Copyright (C) 2014 Li Guang
>> + * Copyright (C) 2016 Xilinx Inc.
>> + * Written by Li Guang 
>> + *
>> + * 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.
>> + */
>
> The text you added to the commit message would make a lovely comment
> here.  Please add it.

Adding it in.

>
>> +
>> +#include "qemu/osdep.h"
>> +#include "qom/cpu.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/dma.h"
>> +#include "hw/loader.h"
>> +#include "qapi/error.h"
>> +#include "hw/core/generic-loader.h"
> [...]
>
> Thank you very much for processing my much-too-late design review
> graciously.

No worries

>
> Acked-by: Markus Armbruster 

Thanks!

Alistair

>



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

2016-09-29 Thread Markus Armbruster
Alistair Francis  writes:

> Add a generic loader to QEMU which can be used to load images or set
> memory values.
>
> Internally inside QEMU this is a device. It is a strange device that
> provides no hardware interface but allows QEMU to monkey patch memory
> specified when it is created. To be able to do this it has a reset
> callback that does the memory operations.
>
> This device allows the user to monkey patch memory. To be able to do
> this it needs a backend to manage the datas, the same as other
> memory-related devices. In this case as the backend is so trivial we
> have merged it with the frontend instead of creating and maintaining a
> seperate backend.

Works for me.

> Signed-off-by: Alistair Francis 
> Reviewed-by: Peter Maydell 
[...]
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> new file mode 100644
> index 000..fc2fea7
> --- /dev/null
> +++ b/hw/core/generic-loader.c
> @@ -0,0 +1,197 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Copyright (C) 2016 Xilinx Inc.
> + * Written by Li Guang 
> + *
> + * 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.
> + */

The text you added to the commit message would make a lovely comment
here.  Please add it.

> +
> +#include "qemu/osdep.h"
> +#include "qom/cpu.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +#include "hw/loader.h"
> +#include "qapi/error.h"
> +#include "hw/core/generic-loader.h"
[...]

Thank you very much for processing my much-too-late design review
graciously.

Acked-by: Markus Armbruster 



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

2016-09-28 Thread Alistair Francis
Add a generic loader to QEMU which can be used to load images or set
memory values.

Internally inside QEMU this is a device. It is a strange device that
provides no hardware interface but allows QEMU to monkey patch memory
specified when it is created. To be able to do this it has a reset
callback that does the memory operations.

This device allows the user to monkey patch memory. To be able to do
this it needs a backend to manage the datas, the same as other
memory-related devices. In this case as the backend is so trivial we
have merged it with the frontend instead of creating and maintaining a
seperate backend.

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Maydell 
---
V12:
 - Update commit message to justify using -device
V11:
 - Small corrections
 - Don't check for !data as writing a value of 0 is valid.
V10:
 - Split out the PC setting and data loading
V9:
 - Fix error messages
 - Updated some incorrect logic
 - Add address space loading support for all image types
 - Explain why the reset is manually registered
V8:
 - Code corrections
 - Rebase
V7:
 - Rebase
V6:
 - Add error checking
V5:
 - Rebase
V4:
 - Allow the loader to work with every architecture
 - Move the file to hw/core
 - Increase the maximum number of CPUs
 - Make the CPU operations conditional
 - Convert the cpu option to cpu-num
 - Require the user to specify endianess
V3:
 - Pass the ram_size to load_image_targphys()
V2:
 - Add maintainers entry
 - Perform bounds checking
 - Register and unregister the reset in the realise/unrealise
Changes since RFC:
 - Add BE support

 MAINTAINERS  |   6 ++
 hw/core/Makefile.objs|   2 +
 hw/core/generic-loader.c | 197 +++
 include/hw/core/generic-loader.h |  46 +
 4 files changed, 251 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f3c1f7f..9ecaaa5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1013,6 +1013,12 @@ M: Dmitry Fleytman 
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis 
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 --
 Audio
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index cfd4840..939c94e 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_SOFTMMU) += register.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
new file mode 100644
index 000..fc2fea7
--- /dev/null
+++ b/hw/core/generic-loader.c
@@ -0,0 +1,197 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Copyright (C) 2016 Xilinx Inc.
+ * Written by Li Guang 
+ *
+ * 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 "qemu/osdep.h"
+#include "qom/cpu.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+#include "hw/loader.h"
+#include "qapi/error.h"
+#include "hw/core/generic-loader.h"
+
+#define CPU_NONE 0x
+
+static void generic_loader_reset(void *opaque)
+{
+GenericLoaderState *s = GENERIC_LOADER(opaque);
+
+if (s->set_pc) {
+CPUClass *cc = CPU_GET_CLASS(s->cpu);
+cpu_reset(s->cpu);
+if (cc) {
+cc->set_pc(s->cpu, s->addr);
+}
+}
+
+if (s->data_len) {
+assert(s->data_len < sizeof(s->data));
+dma_memory_write(s->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;
+
+s->set_pc = false;
+
+/* Perform some error checking on the user's options */
+if (s->data || s->data_len  || s->data_be) {
+/* User is loading memory values */
+if (s->file) {
+error_setg(errp, "Specifying a file is not supported when loading "
+   "memory values");
+return;
+} else if (s->force_raw) {
+error_setg(errp, "Specifying force-raw is not supported when "
+   "loading memory values");
+return;
+} else if (!s->data_len) {
+/* We cant' check for !data here as a value of 0 is still valid. */
+error_setg(errp, "Both