[Qemu-devel] [PATCH] hw/arm/virt: Fix non-secure flash mode

[David Engraf]

Using the whole 128 MiB flash in non-secure mode is not working because
virt_flash_fdt() expects the same address for secure_sysmem and sysmem.
This is not correctly handled by caller because it forwards NULL for
secure_sysmem in non-secure flash mode.

Fixed by using sysmem when secure_sysmem is NULL.

Signed-off-by: David Engraf 
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0b5138cb22..d9496c9363 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1674,7 +1674,7 @@ static void machvirt_init(MachineState *machine)
 >device_memory->mr);
 }
 
-virt_flash_fdt(vms, sysmem, secure_sysmem);
+virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem);
 
 create_gic(vms, pic);
 
-- 
2.17.1

[Qemu-devel] [PATCH] PPC e500: Fix gap between u-boot and kernel

[David Engraf]

This patch moves the gap between u-boot and kernel at the correct location.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 hw/ppc/e500.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 43c15d18c4..bdef2bddc6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1009,6 +1009,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 
 cur_base = loadaddr + payload_size;
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
 
 /* Load bare kernel only if no bios/u-boot has been provided */
 if (machine->kernel_filename && !kernel_as_payload) {
@@ -1025,11 +1029,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 cur_base += kernel_size;
 }
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 /* Load initrd. */
 if (machine->initrd_filename) {
 initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-- 
2.14.1

[Qemu-devel] [PATCH v4] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image.

When here do not use bios_name/size for the kernel and use a more generic
name called payload_name/size.

New in v3: dtb must be stored between kernel and initrd because Linux can
   handle the dtb only within the first 64MB. Add a comment to
   clarify the behavior.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
---
 hw/ppc/e500.c | 116 +++---
 1 file changed, 70 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ef541a00be..43c15d18c4 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -792,8 +792,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 int initrd_size = 0;
 hwaddr cur_base = 0;
 char *filename;
+const char *payload_name;
+bool kernel_as_payload;
 hwaddr bios_entry = 0;
-target_long bios_size;
+target_long payload_size;
 struct boot_info *boot_info;
 int dt_size;
 int i;
@@ -921,11 +923,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 /* Register spinning region */
 sysbus_create_simple("e500-spin", params->spin_base, NULL);
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 if (params->has_mpc8xxx_gpio) {
 qemu_irq poweroff_irq;
 
@@ -960,8 +957,61 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 sysbus_mmio_get_region(s, 0));
 }
 
-/* Load kernel. */
-if (machine->kernel_filename) {
+/*
+ * Smart firmware defaults ahead!
+ *
+ * We follow the following table to select which payload we execute.
+ *
+ *  -kernel | -bios | payload
+ * -+---+-
+ * N|   Y   | u-boot
+ * N|   N   | u-boot
+ * Y|   Y   | u-boot
+ * Y|   N   | kernel
+ *
+ * This ensures backwards compatibility with how we used to expose
+ * -kernel to users but allows them to run through u-boot as well.
+ */
+kernel_as_payload = false;
+if (bios_name == NULL) {
+if (machine->kernel_filename) {
+payload_name = machine->kernel_filename;
+kernel_as_payload = true;
+} else {
+payload_name = "u-boot.e500";
+}
+} else {
+payload_name = bios_name;
+}
+
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+
+payload_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+1, PPC_ELF_MACHINE, 0, 0);
+if (payload_size < 0) {
+/*
+ * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
+ * ePAPR compliant kernel
+ */
+payload_size = load_uimage(filename, _entry, , NULL,
+   NULL, NULL);
+if (payload_size < 0) {
+error_report("qemu: could not load firmware '%s'", filename);
+exit(1);
+}
+}
+
+g_free(filename);
+
+if (kernel_as_payload) {
+kernel_base = loadaddr;
+kernel_size = payload_size;
+}
+
+cur_base = loadaddr + payload_size;
+
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename && !kernel_as_payload) {
 kernel_base = cur_base;
 kernel_size = load_image_targphys(machine->kernel_filename,
   cur_base,
@@ -975,6 +1025,11 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 cur_base += kernel_size;
 }
 
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
+
 /* Load initrd. */
 if (machine->initrd_filename) {
 initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
@@ -991,47 +1046,16 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 
 /*
- * Smart firmware defaults ahead!
- *
- * We follow the following table to select which payload we execute.
- *
- *  -kernel | -bios | payload
- * -+---+-
- * N|   Y   | u-boot
- * N|   N   | u-boot
- * Y|   Y   | u-boot
- * Y|   N   | kernel
- *
- 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

Am 02.03.2018 um 10:11 schrieb Mark Cave-Ayland:

On 02/03/18 08:53, David Engraf wrote:


Am 02.03.2018 um 02:45 schrieb David Gibson:

On Thu, Feb 15, 2018 at 10:36:00AM +0100, David Engraf wrote:
This patch fixes an incorrect behavior when the -kernel argument has 
been
specified without -bios. In this case the kernel was loaded twice. 
At address

32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device 
tree and

the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image.

When here do not use bios_name/size for the kernel and use a more 
generic

name called payload_name/size.

New in v3: dtb must be stored between kernel and initrd because 
Linux can

    handle the dtb only within the first 64MB. Add a comment to
    clarify the behavior.

Signed-off-by: David Engraf <david.eng...@sysgo.com>


Sorry I've taken so long to reply to this.  It looks fine to me,
however, other changes mean it longer quite applies to the
ppc-for-2.12 tree.  Can you fix that up and repost please.


What other changes do you mean? In v3 I just restored the old behavior 
by putting the dtb right after the kernel image. This behavior has 
been changed by mistake in v1/v2. v3 now uses the correct storage 
location again.


Hi David,

I think what David means is that when he tried to apply your v3 patch to 
his ppc-for-2.12 tree at 
https://github.com/dgibson/qemu/commits/ppc-for-2.12 using git-am then 
it failed - presumably because someone else has made other changes to 
that code since you submitted your v3 patch.


Please can you submit a v4 rebased on top of the current ppc-for-2.12 
branch?


Hi Mark,

thanks for the info. I will provide an updated version.

Best regards
- David

Re: [Qemu-devel] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

Am 02.03.2018 um 02:45 schrieb David Gibson:

On Thu, Feb 15, 2018 at 10:36:00AM +0100, David Engraf wrote:

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image.

When here do not use bios_name/size for the kernel and use a more generic
name called payload_name/size.

New in v3: dtb must be stored between kernel and initrd because Linux can
handle the dtb only within the first 64MB. Add a comment to
clarify the behavior.

Signed-off-by: David Engraf <david.eng...@sysgo.com>


Sorry I've taken so long to reply to this.  It looks fine to me,
however, other changes mean it longer quite applies to the
ppc-for-2.12 tree.  Can you fix that up and repost please.


What other changes do you mean? In v3 I just restored the old behavior 
by putting the dtb right after the kernel image. This behavior has been 
changed by mistake in v1/v2. v3 now uses the correct storage location again.


Best regards
- David




Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>


---
  hw/ppc/e500.c | 116 +++---
  1 file changed, 70 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..414c4beaab 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -784,8 +784,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  int initrd_size = 0;
  hwaddr cur_base = 0;
  char *filename;
+const char *payload_name;
+bool kernel_as_payload;
  hwaddr bios_entry = 0;
-target_long bios_size;
+target_long payload_size;
  struct boot_info *boot_info;
  int dt_size;
  int i;
@@ -913,11 +915,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  /* Register spinning region */
  sysbus_create_simple("e500-spin", params->spin_base, NULL);
  
-if (cur_base < (32 * 1024 * 1024)) {

-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
  if (params->has_mpc8xxx_gpio) {
  qemu_irq poweroff_irq;
  
@@ -952,8 +949,61 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)

  sysbus_mmio_get_region(s, 0));
  }
  
-/* Load kernel. */

-if (machine->kernel_filename) {
+/*
+ * Smart firmware defaults ahead!
+ *
+ * We follow the following table to select which payload we execute.
+ *
+ *  -kernel | -bios | payload
+ * -+---+-
+ * N|   Y   | u-boot
+ * N|   N   | u-boot
+ * Y|   Y   | u-boot
+ * Y|   N   | kernel
+ *
+ * This ensures backwards compatibility with how we used to expose
+ * -kernel to users but allows them to run through u-boot as well.
+ */
+kernel_as_payload = false;
+if (bios_name == NULL) {
+if (machine->kernel_filename) {
+payload_name = machine->kernel_filename;
+kernel_as_payload = true;
+} else {
+payload_name = "u-boot.e500";
+}
+} else {
+payload_name = bios_name;
+}
+
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+
+payload_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+1, PPC_ELF_MACHINE, 0, 0);
+if (payload_size < 0) {
+/*
+ * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
+ * ePAPR compliant kernel
+ */
+payload_size = load_uimage(filename, _entry, , NULL,
+   NULL, NULL);
+if (payload_size < 0) {
+fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
+exit(1);
+}
+}
+
+g_free(filename);
+
+if (kernel_as_payload) {
+kernel_base = loadaddr;
+kernel_size = payload_size;
+}
+
+cur_base = loadaddr + payload_size;
+
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename && !kernel_as_payload) {
  kernel_base = cur_base;
  kernel_size = load_image_targphys(machine->kernel_filename,
cur_base,
@@ -967,6 +1017,11 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  cur_base += kernel_size;
  }
  
+if (cur_base < (32 * 1024 * 1024)) {

+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
+
  /* Load

[Qemu-devel] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image.

When here do not use bios_name/size for the kernel and use a more generic
name called payload_name/size.

New in v3: dtb must be stored between kernel and initrd because Linux can
   handle the dtb only within the first 64MB. Add a comment to
   clarify the behavior.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 hw/ppc/e500.c | 116 +++---
 1 file changed, 70 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..414c4beaab 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -784,8 +784,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 int initrd_size = 0;
 hwaddr cur_base = 0;
 char *filename;
+const char *payload_name;
+bool kernel_as_payload;
 hwaddr bios_entry = 0;
-target_long bios_size;
+target_long payload_size;
 struct boot_info *boot_info;
 int dt_size;
 int i;
@@ -913,11 +915,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 /* Register spinning region */
 sysbus_create_simple("e500-spin", params->spin_base, NULL);
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 if (params->has_mpc8xxx_gpio) {
 qemu_irq poweroff_irq;
 
@@ -952,8 +949,61 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 sysbus_mmio_get_region(s, 0));
 }
 
-/* Load kernel. */
-if (machine->kernel_filename) {
+/*
+ * Smart firmware defaults ahead!
+ *
+ * We follow the following table to select which payload we execute.
+ *
+ *  -kernel | -bios | payload
+ * -+---+-
+ * N|   Y   | u-boot
+ * N|   N   | u-boot
+ * Y|   Y   | u-boot
+ * Y|   N   | kernel
+ *
+ * This ensures backwards compatibility with how we used to expose
+ * -kernel to users but allows them to run through u-boot as well.
+ */
+kernel_as_payload = false;
+if (bios_name == NULL) {
+if (machine->kernel_filename) {
+payload_name = machine->kernel_filename;
+kernel_as_payload = true;
+} else {
+payload_name = "u-boot.e500";
+}
+} else {
+payload_name = bios_name;
+}
+
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+
+payload_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+1, PPC_ELF_MACHINE, 0, 0);
+if (payload_size < 0) {
+/*
+ * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
+ * ePAPR compliant kernel
+ */
+payload_size = load_uimage(filename, _entry, , NULL,
+   NULL, NULL);
+if (payload_size < 0) {
+fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
+exit(1);
+}
+}
+
+g_free(filename);
+
+if (kernel_as_payload) {
+kernel_base = loadaddr;
+kernel_size = payload_size;
+}
+
+cur_base = loadaddr + payload_size;
+
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename && !kernel_as_payload) {
 kernel_base = cur_base;
 kernel_size = load_image_targphys(machine->kernel_filename,
   cur_base,
@@ -967,6 +1017,11 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 cur_base += kernel_size;
 }
 
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
+
 /* Load initrd. */
 if (machine->initrd_filename) {
 initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
@@ -983,47 +1038,16 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 
 /*
- * Smart firmware defaults ahead!
- *
- * We follow the following table to select which payload we execute.
- *
- *  -kernel | -bios | payload
- * -+---+-
- * N|   Y   | u-boot
- * N|   N   | u-boot
- * Y|   Y   | u-boot
- * Y|   N   | kernel
- *
- * This ensures backwards compatibility with how we u

[Qemu-devel] [PATCH v2] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image. It also ensures that
the device tree is generated behind bios/kernel/initrd.

When here do not use bios_name/size for the kernel and use a more generic
name called payload_name/size.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 hw/ppc/e500.c | 112 ++
 1 file changed, 65 insertions(+), 47 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..d12b9d6121 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 MemoryRegion *ram = g_new(MemoryRegion, 1);
 PCIBus *pci_bus;
 CPUPPCState *env = NULL;
-uint64_t loadaddr;
 hwaddr kernel_base = -1LL;
 int kernel_size = 0;
 hwaddr dt_base = 0;
@@ -784,8 +783,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 int initrd_size = 0;
 hwaddr cur_base = 0;
 char *filename;
+const char *payload_name;
+bool kernel_as_payload;
 hwaddr bios_entry = 0;
-target_long bios_size;
+target_long payload_size;
 struct boot_info *boot_info;
 int dt_size;
 int i;
@@ -913,11 +914,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 /* Register spinning region */
 sysbus_create_simple("e500-spin", params->spin_base, NULL);
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 if (params->has_mpc8xxx_gpio) {
 qemu_irq poweroff_irq;
 
@@ -952,36 +948,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 sysbus_mmio_get_region(s, 0));
 }
 
-/* Load kernel. */
-if (machine->kernel_filename) {
-kernel_base = cur_base;
-kernel_size = load_image_targphys(machine->kernel_filename,
-  cur_base,
-  ram_size - cur_base);
-if (kernel_size < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-machine->kernel_filename);
-exit(1);
-}
-
-cur_base += kernel_size;
-}
-
-/* Load initrd. */
-if (machine->initrd_filename) {
-initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
-  ram_size - initrd_base);
-
-if (initrd_size < 0) {
-fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-machine->initrd_filename);
-exit(1);
-}
-
-cur_base = initrd_base + initrd_size;
-}
-
 /*
  * Smart firmware defaults ahead!
  *
@@ -997,33 +963,85 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  * This ensures backwards compatibility with how we used to expose
  * -kernel to users but allows them to run through u-boot as well.
  */
+kernel_as_payload = false;
 if (bios_name == NULL) {
 if (machine->kernel_filename) {
-bios_name = machine->kernel_filename;
+payload_name = machine->kernel_filename;
+kernel_as_payload = true;
 } else {
-bios_name = "u-boot.e500";
+payload_name = "u-boot.e500";
 }
+} else {
+payload_name = bios_name;
 }
-filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
 
-bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,
- 1, PPC_ELF_MACHINE, 0, 0);
-if (bios_size < 0) {
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+
+payload_size = load_elf(filename, NULL, NULL, _entry, _base, NULL,
+1, PPC_ELF_MACHINE, 0, 0);
+if (payload_size < 0) {
 /*
  * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
  * ePAPR compliant kernel
  */
-kernel_size = load_uimage(filename, _entry, , NULL,
-  NULL, NULL);
-if (kernel_size < 0) {
+payload_size = load_uimage(filename, _entry, _base, NULL,
+   NULL, NULL);
+if (payload_size < 0) {
 fprintf(stderr, "qemu: could not load firmware '%s'\n", file

Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

Am 13.02.2018 um 04:51 schrieb David Gibson:

On Fri, Feb 09, 2018 at 08:49:45AM +0100, David Engraf wrote:

Hello David,

Am 09.02.2018 um 06:33 schrieb David Gibson:

On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image. It also ensures that
the device tree is generated behind bios/kernel/initrd.

Signed-off-by: David Engraf <david.eng...@sysgo.com>


Sorry I've taken so long to respond to this.  I've been busy, then
away, then busy, then recovering from surgery, then...

I think this looks good overall, just a couple of details I'd like to
check, see below.


---
   hw/ppc/e500.c | 89 
---
   1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..0321bd66a8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
   MemoryRegion *ram = g_new(MemoryRegion, 1);
   PCIBus *pci_bus;
   CPUPPCState *env = NULL;
-uint64_t loadaddr;
   hwaddr kernel_base = -1LL;
   int kernel_size = 0;
   hwaddr dt_base = 0;
@@ -913,11 +912,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
   /* Register spinning region */
   sysbus_create_simple("e500-spin", params->spin_base, NULL);
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
   if (params->has_mpc8xxx_gpio) {
   qemu_irq poweroff_irq;
@@ -952,36 +946,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
   sysbus_mmio_get_region(s, 0));
   }
-/* Load kernel. */
-if (machine->kernel_filename) {
-kernel_base = cur_base;
-kernel_size = load_image_targphys(machine->kernel_filename,
-  cur_base,
-  ram_size - cur_base);
-if (kernel_size < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-machine->kernel_filename);
-exit(1);
-}
-
-cur_base += kernel_size;
-}
-
-/* Load initrd. */
-if (machine->initrd_filename) {
-initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
-  ram_size - initrd_base);
-
-if (initrd_size < 0) {
-fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-machine->initrd_filename);
-exit(1);
-}
-
-cur_base = initrd_base + initrd_size;
-}
-
   /*
* Smart firmware defaults ahead!
*
@@ -1006,24 +970,67 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
   }
   filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+bios_size = load_elf(filename, NULL, NULL, _entry, _base, NULL,
1, PPC_ELF_MACHINE, 0, 0);
   if (bios_size < 0) {
   /*
* Hrm. No ELF image? Try a uImage, maybe someone is giving us an
* ePAPR compliant kernel
*/
-kernel_size = load_uimage(filename, _entry, , NULL,
-  NULL, NULL);
-if (kernel_size < 0) {
+bios_size = load_uimage(filename, _entry, _base, NULL,
+NULL, NULL);
+if (bios_size < 0) {
   fprintf(stderr, "qemu: could not load firmware '%s'\n", 
filename);
   exit(1);
   }
   }
+cur_base += bios_size;
   g_free(filename);
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename != bios_name) {


This condition seems weird.  Why would the kernel and bios images be
the same.  I guess this because of the logic setting bios_name above,
which also seems kind of weird.  Can you clarify what's going on here,
changing that bios_name setting logic, if necessary?


Basically I didn't change the logic to store the kernel name into bios_name
when the -bios options was not provided. In this case the kernel will be
used as payload. Indeed the name is a bit weird. What do you think about
changing the names from 

Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

Hello David,

Am 09.02.2018 um 06:33 schrieb David Gibson:

On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image. It also ensures that
the device tree is generated behind bios/kernel/initrd.

Signed-off-by: David Engraf <david.eng...@sysgo.com>


Sorry I've taken so long to respond to this.  I've been busy, then
away, then busy, then recovering from surgery, then...

I think this looks good overall, just a couple of details I'd like to
check, see below.


---
  hw/ppc/e500.c | 89 ---
  1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..0321bd66a8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  MemoryRegion *ram = g_new(MemoryRegion, 1);
  PCIBus *pci_bus;
  CPUPPCState *env = NULL;
-uint64_t loadaddr;
  hwaddr kernel_base = -1LL;
  int kernel_size = 0;
  hwaddr dt_base = 0;
@@ -913,11 +912,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  /* Register spinning region */
  sysbus_create_simple("e500-spin", params->spin_base, NULL);
  
-if (cur_base < (32 * 1024 * 1024)) {

-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
  if (params->has_mpc8xxx_gpio) {
  qemu_irq poweroff_irq;
  
@@ -952,36 +946,6 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)

  sysbus_mmio_get_region(s, 0));
  }
  
-/* Load kernel. */

-if (machine->kernel_filename) {
-kernel_base = cur_base;
-kernel_size = load_image_targphys(machine->kernel_filename,
-  cur_base,
-  ram_size - cur_base);
-if (kernel_size < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-machine->kernel_filename);
-exit(1);
-}
-
-cur_base += kernel_size;
-}
-
-/* Load initrd. */
-if (machine->initrd_filename) {
-initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
-  ram_size - initrd_base);
-
-if (initrd_size < 0) {
-fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-machine->initrd_filename);
-exit(1);
-}
-
-cur_base = initrd_base + initrd_size;
-}
-
  /*
   * Smart firmware defaults ahead!
   *
@@ -1006,24 +970,67 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
  }
  filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
  
-bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,

+bios_size = load_elf(filename, NULL, NULL, _entry, _base, NULL,
   1, PPC_ELF_MACHINE, 0, 0);
  if (bios_size < 0) {
  /*
   * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
   * ePAPR compliant kernel
   */
-kernel_size = load_uimage(filename, _entry, , NULL,
-  NULL, NULL);
-if (kernel_size < 0) {
+bios_size = load_uimage(filename, _entry, _base, NULL,
+NULL, NULL);
+if (bios_size < 0) {
  fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
  exit(1);
  }
  }
+cur_base += bios_size;
  g_free(filename);
  
+/* Load bare kernel only if no bios/u-boot has been provided */

+if (machine->kernel_filename != bios_name) {


This condition seems weird.  Why would the kernel and bios images be
the same.  I guess this because of the logic setting bios_name above,
which also seems kind of weird.  Can you clarify what's going on here,
changing that bios_name setting logic, if necessary?


Basically I didn't change the logic to store the kernel name into 
bios_name when the -bios options was not provided. In this case the 
kernel will be used as payload. Indeed the name is a bit weird. What do 
you think about changing the names from bios to payload?



+kernel_base = cur_base;
+kernel_size = lo

[Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image. It also ensures that
the device tree is generated behind bios/kernel/initrd.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 hw/ppc/e500.c | 89 ---
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..0321bd66a8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 MemoryRegion *ram = g_new(MemoryRegion, 1);
 PCIBus *pci_bus;
 CPUPPCState *env = NULL;
-uint64_t loadaddr;
 hwaddr kernel_base = -1LL;
 int kernel_size = 0;
 hwaddr dt_base = 0;
@@ -913,11 +912,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 /* Register spinning region */
 sysbus_create_simple("e500-spin", params->spin_base, NULL);
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 if (params->has_mpc8xxx_gpio) {
 qemu_irq poweroff_irq;
 
@@ -952,36 +946,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 sysbus_mmio_get_region(s, 0));
 }
 
-/* Load kernel. */
-if (machine->kernel_filename) {
-kernel_base = cur_base;
-kernel_size = load_image_targphys(machine->kernel_filename,
-  cur_base,
-  ram_size - cur_base);
-if (kernel_size < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-machine->kernel_filename);
-exit(1);
-}
-
-cur_base += kernel_size;
-}
-
-/* Load initrd. */
-if (machine->initrd_filename) {
-initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
-  ram_size - initrd_base);
-
-if (initrd_size < 0) {
-fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-machine->initrd_filename);
-exit(1);
-}
-
-cur_base = initrd_base + initrd_size;
-}
-
 /*
  * Smart firmware defaults ahead!
  *
@@ -1006,24 +970,67 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
 
-bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+bios_size = load_elf(filename, NULL, NULL, _entry, _base, NULL,
  1, PPC_ELF_MACHINE, 0, 0);
 if (bios_size < 0) {
 /*
  * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
  * ePAPR compliant kernel
  */
-kernel_size = load_uimage(filename, _entry, , NULL,
-  NULL, NULL);
-if (kernel_size < 0) {
+bios_size = load_uimage(filename, _entry, _base, NULL,
+NULL, NULL);
+if (bios_size < 0) {
 fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
 exit(1);
 }
 }
+cur_base += bios_size;
 g_free(filename);
 
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename != bios_name) {
+kernel_base = cur_base;
+kernel_size = load_image_targphys(machine->kernel_filename,
+  cur_base,
+  ram_size - cur_base);
+if (kernel_size < 0) {
+fprintf(stderr, "qemu: could not load kernel '%s'\n",
+machine->kernel_filename);
+exit(1);
+}
+
+cur_base += kernel_size;
+} else {
+kernel_base = cur_base;
+kernel_size = bios_size;
+}
+
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
+
+/* Load initrd. */
+if (machine->initrd_filename) {
+initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
+initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
+  ram_size - initrd_base);
+
+i

[Qemu-devel] [PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Engraf]

This patch fixes an incorrect behavior when the -kernel argument has been
specified without -bios. In this case the kernel was loaded twice. At address
32M as a raw image and afterwards by load_elf/load_uimage at the
corresponding load address. In this case the region for the device tree and
the raw kernel image may overlap.

The patch fixes the behavior by loading the kernel image once with
load_elf/load_uimage and skips loading the raw image. It also ensures that
the device tree is generated behind bios/kernel/initrd.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 hw/ppc/e500.c | 89 ---
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c4fe06ea2a..0321bd66a8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 MemoryRegion *ram = g_new(MemoryRegion, 1);
 PCIBus *pci_bus;
 CPUPPCState *env = NULL;
-uint64_t loadaddr;
 hwaddr kernel_base = -1LL;
 int kernel_size = 0;
 hwaddr dt_base = 0;
@@ -913,11 +912,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 /* Register spinning region */
 sysbus_create_simple("e500-spin", params->spin_base, NULL);
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 if (params->has_mpc8xxx_gpio) {
 qemu_irq poweroff_irq;
 
@@ -952,36 +946,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 sysbus_mmio_get_region(s, 0));
 }
 
-/* Load kernel. */
-if (machine->kernel_filename) {
-kernel_base = cur_base;
-kernel_size = load_image_targphys(machine->kernel_filename,
-  cur_base,
-  ram_size - cur_base);
-if (kernel_size < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-machine->kernel_filename);
-exit(1);
-}
-
-cur_base += kernel_size;
-}
-
-/* Load initrd. */
-if (machine->initrd_filename) {
-initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
-  ram_size - initrd_base);
-
-if (initrd_size < 0) {
-fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-machine->initrd_filename);
-exit(1);
-}
-
-cur_base = initrd_base + initrd_size;
-}
-
 /*
  * Smart firmware defaults ahead!
  *
@@ -1006,24 +970,67 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
 
-bios_size = load_elf(filename, NULL, NULL, _entry, , NULL,
+bios_size = load_elf(filename, NULL, NULL, _entry, _base, NULL,
  1, PPC_ELF_MACHINE, 0, 0);
 if (bios_size < 0) {
 /*
  * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
  * ePAPR compliant kernel
  */
-kernel_size = load_uimage(filename, _entry, , NULL,
-  NULL, NULL);
-if (kernel_size < 0) {
+bios_size = load_uimage(filename, _entry, _base, NULL,
+NULL, NULL);
+if (bios_size < 0) {
 fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
 exit(1);
 }
 }
+cur_base += bios_size;
 g_free(filename);
 
+/* Load bare kernel only if no bios/u-boot has been provided */
+if (machine->kernel_filename != bios_name) {
+kernel_base = cur_base;
+kernel_size = load_image_targphys(machine->kernel_filename,
+  cur_base,
+  ram_size - cur_base);
+if (kernel_size < 0) {
+fprintf(stderr, "qemu: could not load kernel '%s'\n",
+machine->kernel_filename);
+exit(1);
+}
+
+cur_base += kernel_size;
+} else {
+kernel_base = cur_base;
+kernel_size = bios_size;
+}
+
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
+
+/* Load initrd. */
+if (machine->initrd_filename) {
+initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
+initrd_size = load_image_targphys(machine->initrd_filename, 
initrd_base,
+  ram_size - initrd_base);
+
+i

Re: [Qemu-devel] [PATCH v2] pflash_cfi01: fix per device sector length in CFI table

[David Engraf]

Am 16.01.2017 um 11:26 schrieb Dr. David Alan Gilbert:

* Andrew Jones (drjo...@redhat.com) wrote:

On Thu, Jan 12, 2017 at 10:42:41AM +, Peter Maydell wrote:

On 12 January 2017 at 10:35, David Engraf <david.eng...@sysgo.com> wrote:

The CFI entry for sector length must be set to sector length per device.
This is important for boards using multiple devices like the ARM Vexpress
board (width = 4, device-width = 2).

Linux and u-boots calculate the size ratio by dividing both values:

size_ratio = info->portwidth / info->chipwidth;

After that the sector length will be multiplied by the size_ratio, thus the
CFI entry for sector length is doubled. When Linux or u-boot send a sector
erase, they expect to erase the doubled sector length, but QEMU only erases
the board specified sector length.

This patch fixes the sector length in the CFI table to match the length per
device, equal to blocks_per_device.


Thanks for the patch. I haven't checked against the pflash spec yet,
but this looks like it's probably the right thing.

The only two machines which use a setup with multiple devices (ie
which specify device_width to the pflash_cfi01) are vexpress and virt.
For all other machines this patch leaves the behaviour unchanged.

Q: do we need to have some kind of nasty hack so that pre-2.9 virt
still gets the old broken values in the CFI table, for version and
migration compatibility? Ccing Drew for an opinion...



I'm pretty sure we need the nasty hack, but I'm also Ccing David for
his opinion.


Hmm I don't understand enough about pflash to understand the change here;
but generally if you need to keep something unchanged for older
machine types, add a property and then set that property in
the compatibility macros.

See include/hw/compat.h, I think you'd add the entry to HW_COMPAT_2_8
and follow a simple example like  old_msi_addr on intel-hda.c,
that should get picked up by aarch, x86, ppc etc versioned machine types.


This is a new version of the previous patch including the HW_COMPAT entry.

After further tests with u-boot and Linux, I found another problem with 
the blocks per device and write block size. Blocks per device must not 
be divided with the number of devices because the resulting device size 
would not match the overall size. However write block size must be 
modified depending on the number of devices because the entry is per 
device and when u-boot writes into the flash it calculates the write 
size by using the CFI entry (write size per device) multiplied by the 
number of chips.


Here is a configuration example of the vexpress board:

VEXPRESS_FLASH_SIZE = 64M
VEXPRESS_FLASH_SECT_SIZE 256K
num-blocks = VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE = 256
sector-length = 256K
width = 4
device-width = 2

The code will fill the CFI entry with the following entries
num-blocks = 256
sector-length = 128K
writeblock_size = 2048

This results in two chips, each with 256 * 128K = 32M device size and a 
write block size of 2048.


A sector erase will be send to both chips, thus 256K must be erased. 
When u-boot sends a block write command, it will write 4096 bytes data 
at once (2048 per device).


I did not modify the CFI entry for write block size. Instead I kept the 
hard coded value of 2048 in the CFI table and fixed the internal entry 
for writeblock_size.


Best regards
- David


Signed-off-by: David Engraf <david.eng...@sysgo.com>

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 5f0ee9d..996daa3 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -99,6 +99,7 @@ struct pflash_t {
 char *name;
 void *storage;
 VMChangeStateEntry *vmstate;
+bool old_multiple_chip_handling;
 };
 
 static int pflash_post_load(void *opaque, int version_id);
@@ -703,7 +704,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 pflash_t *pfl = CFI_PFLASH01(dev);
 uint64_t total_len;
 int ret;
-uint64_t blocks_per_device, device_len;
+uint64_t blocks_per_device, sector_len_per_device, device_len;
 int num_devices;
 Error *local_err = NULL;
 
@@ -726,8 +727,14 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
  * in the cfi_table[].
  */
 num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
-blocks_per_device = pfl->nb_blocs / num_devices;
-device_len = pfl->sector_len * blocks_per_device;
+if (pfl->old_multiple_chip_handling) {
+blocks_per_device = pfl->nb_blocs / num_devices;
+sector_len_per_device = pfl->sector_len;
+} else {
+blocks_per_device = pfl->nb_blocs;
+sector_len_per_device = pfl->sector_len / num_devices;
+}
+device_len = sector_len_per_device * blocks_per_device;
 
 /* XXX: to be fixed */
 #if 0
@@ -832,6 +839,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 pfl->cfi_table[0x2A] = 0x0B;
 }
 pfl->writ

Re: [Qemu-devel] [PATCH] pflash_cfi01: fix per device sector length in CFI table

[David Engraf]

Am 12.01.2017 um 12:36 schrieb Andrew Jones:

On Thu, Jan 12, 2017 at 10:42:41AM +, Peter Maydell wrote:

On 12 January 2017 at 10:35, David Engraf <david.eng...@sysgo.com> wrote:

The CFI entry for sector length must be set to sector length per device.
This is important for boards using multiple devices like the ARM Vexpress
board (width = 4, device-width = 2).

Linux and u-boots calculate the size ratio by dividing both values:

size_ratio = info->portwidth / info->chipwidth;

After that the sector length will be multiplied by the size_ratio, thus the
CFI entry for sector length is doubled. When Linux or u-boot send a sector
erase, they expect to erase the doubled sector length, but QEMU only erases
the board specified sector length.

This patch fixes the sector length in the CFI table to match the length per
device, equal to blocks_per_device.


Thanks for the patch. I haven't checked against the pflash spec yet,
but this looks like it's probably the right thing.

The only two machines which use a setup with multiple devices (ie
which specify device_width to the pflash_cfi01) are vexpress and virt.
For all other machines this patch leaves the behaviour unchanged.

Q: do we need to have some kind of nasty hack so that pre-2.9 virt
still gets the old broken values in the CFI table, for version and
migration compatibility? Ccing Drew for an opinion...



I'm pretty sure we need the nasty hack, but I'm also Ccing David for
his opinion.


I can update the patch if you give me some more information about the 
hack. Some ifdef for older versions?


Best regards
- David

[Qemu-devel] [PATCH] pflash_cfi01: fix per device sector length in CFI table

[David Engraf]

The CFI entry for sector length must be set to sector length per device. 
This is important for boards using multiple devices like the ARM 
Vexpress board (width = 4, device-width = 2).


Linux and u-boots calculate the size ratio by dividing both values:

size_ratio = info->portwidth / info->chipwidth;

After that the sector length will be multiplied by the size_ratio, thus 
the CFI entry for sector length is doubled. When Linux or u-boot send a 
sector erase, they expect to erase the doubled sector length, but QEMU 
only erases the board specified sector length.


This patch fixes the sector length in the CFI table to match the length 
per device, equal to blocks_per_device.


Signed-off-by: David Engraf <david.eng...@sysgo.com>

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 5f0ee9d..8bb61e4 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -703,7 +703,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 pflash_t *pfl = CFI_PFLASH01(dev);
 uint64_t total_len;
 int ret;
-uint64_t blocks_per_device, device_len;
+uint64_t blocks_per_device, sector_len_per_device, device_len;
 int num_devices;
 Error *local_err = NULL;
 
@@ -727,7 +727,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
  */
 num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
 blocks_per_device = pfl->nb_blocs / num_devices;
-device_len = pfl->sector_len * blocks_per_device;
+sector_len_per_device = pfl->sector_len / num_devices;
+device_len = sector_len_per_device * blocks_per_device;
 
 /* XXX: to be fixed */
 #if 0
@@ -839,8 +840,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 /* Erase block region 1 */
 pfl->cfi_table[0x2D] = blocks_per_device - 1;
 pfl->cfi_table[0x2E] = (blocks_per_device - 1) >> 8;
-pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
-pfl->cfi_table[0x30] = pfl->sector_len >> 16;
+pfl->cfi_table[0x2F] = sector_len_per_device >> 8;
+pfl->cfi_table[0x30] = sector_len_per_device >> 16;
 
 /* Extended */
 pfl->cfi_table[0x31] = 'P';

Re: [Qemu-devel] [PATCH] vexpress fix flash device-width

[David Engraf]

Am 05.01.2017 um 19:32 schrieb Peter Maydell:

On 22 December 2016 at 14:02, David Engraf <david.eng...@sysgo.com> wrote:

The current implementation uses width = 4 and device-width = 2 for the flash
configuration. When using u-boot or Linux, the flash is detected as 32 x 16
bit, thus the sector size is doubled to 512 KB. When u-boot sends a sector
erase, only the first 256 KB are erased because the QEMU flash
implementation uses the configured sector size of 256 KB and ignores the
width and device-width ratio.

This patch will change device-width to 4, thus the width and device-width
are equal and u-boot detects the flash as 32 x 32 bit with the correct
sector size of 256 KB.


Thanks for this patch. However I'm not sure it is correct, because
I think the real hardware does have 2x16 devices here. On real vexpress
TC2 if you boot Linux it detects the flash as:
"800.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
Manufacturer ID 0x89 Chip ID 0x008919"

So while we presumably have a QEMU bug if this isn't behaving
like the hardware, I don't think that changing the device-width to
4 is the right fix for it. Maybe:
 * our erase implementation is broken?
 * one of the other parameters we use to create the flash
   is wrong?
 * the calculation of the erase block information that we put in
   the flash cfi_table[] is wrong?

(Does u-boot hard-code the erase block sizes, or does
it read them from the CFI data? QEMU's flash doesn't have the
hardware's two-eraseblock-regions setup where part of the disk is
256K erase-blocks and part is 64K blocks, but if u-boot doesn't
hardcode that it should be able to read the data from QEMU's
flash implementation.)


u-boot and Linux are using the same way to detect the sector erase size:

size_ratio = info->portwidth / info->chipwidth;

In our case we have a width of 4 and a device-width of 2. Thus the per 
device sector erase size is doubled from 256K to 512K. This calculation 
is correct because an erase command is send to both chips, each erasing 
256K.


So there are two ways to fix this, depending how the QEMU property 
"sector-length" should be interpreted. If the parameter specifies the 
per device sector length, we have to fix the length in the erase 
command. The other way would be to expect "sector-length" as the overall 
sector size, which makes more sense in my eyes. Thus in our case this 
value is halved and the result is stored in the CFI table. BTW this is 
equal to blocks_per_device which is already calculated by the overall 
number of blocks divided by num_devices.


I did a quick test with the attached patch and it was working in Linux. 
Please have a look at the attached patch, if this is what you expect, I 
will send a new mail with the correct description.



Also, for patches to QEMU we need you to provide a
Signed-off-by: line to say that you're happy to contribute the
patch to QEMU. More detail about what this means here:
http://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line
(the other details of patch formatting are mostly nice-to-haves,
but the s-o-by line is a hard requirement.)


Sorry, forgot this.

Thanks
- David

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 5f0ee9d..8bb61e4 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -703,7 +703,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 pflash_t *pfl = CFI_PFLASH01(dev);
 uint64_t total_len;
 int ret;
-uint64_t blocks_per_device, device_len;
+uint64_t blocks_per_device, sector_len_per_device, device_len;
 int num_devices;
 Error *local_err = NULL;
 
@@ -727,7 +727,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
  */
 num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
 blocks_per_device = pfl->nb_blocs / num_devices;
-device_len = pfl->sector_len * blocks_per_device;
+sector_len_per_device = pfl->sector_len / num_devices;
+device_len = sector_len_per_device * blocks_per_device;
 
 /* XXX: to be fixed */
 #if 0
@@ -839,8 +840,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 /* Erase block region 1 */
 pfl->cfi_table[0x2D] = blocks_per_device - 1;
 pfl->cfi_table[0x2E] = (blocks_per_device - 1) >> 8;
-pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
-pfl->cfi_table[0x30] = pfl->sector_len >> 16;
+pfl->cfi_table[0x2F] = sector_len_per_device >> 8;
+pfl->cfi_table[0x30] = sector_len_per_device >> 16;
 
 /* Extended */
 pfl->cfi_table[0x31] = 'P';

[Qemu-devel] [PATCH] vexpress fix flash device-width

[David Engraf]

The current implementation uses width = 4 and device-width = 2 for the 
flash configuration. When using u-boot or Linux, the flash is detected 
as 32 x 16 bit, thus the sector size is doubled to 512 KB. When u-boot 
sends a sector erase, only the first 256 KB are erased because the QEMU 
flash implementation uses the configured sector size of 256 KB and 
ignores the width and device-width ratio.


This patch will change device-width to 4, thus the width and 
device-width are equal and u-boot detects the flash as 32 x 32 bit with 
the correct sector size of 256 KB.


- David

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 58760f4..85ec347 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -526,7 +526,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
  VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
 qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
 qdev_prop_set_uint8(dev, "width", 4);
-qdev_prop_set_uint8(dev, "device-width", 2);
+qdev_prop_set_uint8(dev, "device-width", 4);
 qdev_prop_set_bit(dev, "big-endian", false);
 qdev_prop_set_uint16(dev, "id0", 0x89);
 qdev_prop_set_uint16(dev, "id1", 0x18);

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:



On 25/11/2015 16:48, David Engraf wrote:


Indeed, TLS handling is broken. The address of iothread_locked is always
the same between threads and I can see that a different thread sets
iothread_locked to false, thus my current thread uses an invalid state.
I will have to check why my compiler produces invalid TLS code.


That rings a bell, I think there are different CRT libraries or
something like that.  Stefan, what would break TLS under Windows?


"--extra-cflags=-mthreads" is the solution. iothread_locked is unique 
for each thread now. I saw that this is already mentioned in the 
Documentation [1], but why isn't that enabled by default if QEMU 
requires this option on Windows? Without this option, even the latest 
version of gcc doesn't produce working TLS code.


Many thanks for your support!

David


[1] http://wiki.qemu.org/Hosts/W32

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 26.11.2015 um 12:25 schrieb Stefan Weil:

Am 26.11.2015 um 10:12 schrieb David Engraf:

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:



On 25/11/2015 16:48, David Engraf wrote:


Indeed, TLS handling is broken. The address of iothread_locked is
always
the same between threads and I can see that a different thread sets
iothread_locked to false, thus my current thread uses an invalid state.
I will have to check why my compiler produces invalid TLS code.


That rings a bell, I think there are different CRT libraries or
something like that.  Stefan, what would break TLS under Windows?


"--extra-cflags=-mthreads" is the solution. iothread_locked is unique
for each thread now. I saw that this is already mentioned in the
Documentation [1], but why isn't that enabled by default if QEMU
requires this option on Windows? Without this option, even the latest
version of gcc doesn't produce working TLS code.

Many thanks for your support!

David

[1] http://wiki.qemu.org/Hosts/W32


Hi David,

I just prepared a patch which will enable that option by default
for all compilations targeting Windows.

Thanks for your report!


Thank you.

David

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 25.11.2015 um 15:36 schrieb Paolo Bonzini:



On 25/11/2015 15:04, David Engraf wrote:




No, you don't.  Who is reading iothread_locked during
qemu_cond_wait_iothread?  No one, because it is a thread-local variable
whose address is never taken.


prepare_mmio_access is reading iothread_locked by using
qemu_mutex_iothread_locked after qemu_tcg_wait_io_event calls
qemu_cond_wait. All one the same thread.


Sure, but who has set iothread_locked to false during the execution of
qemu_cond_wait?  No one, because it's a thread-local variable.  If it's
true before qemu_cond_wait, it will be true after qemu_cond_wait and you
don't need qemu_cond_wait_iothread... unless your compiler is broken and
doesn't generate TLS properly.


Indeed, TLS handling is broken. The address of iothread_locked is always 
the same between threads and I can see that a different thread sets 
iothread_locked to false, thus my current thread uses an invalid state. 
I will have to check why my compiler produces invalid TLS code.


David



Can you compile cpus.c with -S and attach it?

Paolo


qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
-> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec
-> cpu_exec ends up in calling prepare_mmio_access

Re: [Qemu-devel] [PATCH] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Hi Paolo,

Am 24.11.2015 um 16:54 schrieb Paolo Bonzini:

On 24/11/2015 16:43, David Engraf wrote:

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function
qemu_mutex_iothread_locked to avoid recursive locking of the iothread
lock. The iothread_locked variable uses the __thread attribute which
results in a per thread storage location whereas the qemu_global_mutex
is not thread specific. This leads to race conditions because the
variable is not in sync between threads.


Frankly, this makes no sense.  You're modifying the
qemu_mutex_iothread_locked function to return whether _some_ thread has
taken the mutex, but the function tells you whether _the calling_ thread
has taken the mutex (that's the point about recursive locking).  This
breaks the callers of qemu_mutex_iothread_locked completely.

The variable need not be in sync between the different threads; each
thread only needs to know about itself.  The current code works because:

>

1) the iothread mutex is not locked before qemu_mutex_lock_iothread

2) the iothread mutex is not locked after qemu_mutex_lock_iothread

3) qemu_cond_wait doesn't matter because the thread does not run during
a qemu_cond_wait.


But this is my issue on Windows:

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
   -> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec
   -> cpu_exec ends up in calling prepare_mmio_access
  prepare_mmio_access uses qemu_mutex_iothread_locked to check if
  the lock is already held for this thread, but it returns 0
  because qemu_cond_wait doesn't set iothread_locked but the mutex
  is locked. Thus the function calls qemu_mutex_lock_iothread which
  tries to acquire the mutex again. On Windows this results in an
  assertion:

Assertion failed: mutex->owner == 0, file util/qemu-thread-win32.c, line 60

I'm using a self compiled, cross gcc-5.2.0 mingw compiler.

David

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Hi Paolo,

please check the new version. I removed changing the iothread_locked
variable. But I still need to set the correct value of iothread_locked
when using qemu_cond_wait. This will fix my race condition on Windows
when prepare_mmio_access is called and checks if the lock is already
held by the current thread.

David


Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function
qemu_mutex_iothread_locked to avoid recursive locking of the iothread
lock. But the function qemu_cond_wait directly acquires the lock without
modifying iothread_locked. This leads to condition where
qemu_tcg_wait_io_event acquires the mutex by using qemu_cond_wait and
later calls tcg_exec_all > tcg_cpu_exec -> cpu_exec >
prepare_mmio_access checking if the current thread already holds the
lock. This is done by checking the variable iothread_locked which is
still 0, thus the function tries to acquire the mutex again.

The patch adds a function called qemu_cond_wait_iothread to keep
iothread_locked in sync.

Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 cpus.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/cpus.c b/cpus.c
index 877bd70..e4d2d4b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -70,6 +70,8 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;

+static void qemu_cond_wait_iothread(QemuCond *cond);
+
 /* vcpu throttling controls */
 static QEMUTimer *throttle_timer;
 static unsigned int throttle_percentage;
@@ -920,7 +922,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void 
*data), void *data)

 while (!atomic_mb_read()) {
 CPUState *self_cpu = current_cpu;

-qemu_cond_wait(_work_cond, _global_mutex);
+qemu_cond_wait_iothread(_work_cond);
 current_cpu = self_cpu;
 }
 }
@@ -998,11 +1000,11 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
/* Start accounting real time to the virtual clock if the CPUs
   are idle.  */
 qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
-qemu_cond_wait(cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(cpu->halt_cond);
 }

 while (iothread_requesting_mutex) {
-qemu_cond_wait(_io_proceeded_cond, _global_mutex);
+qemu_cond_wait_iothread(_io_proceeded_cond);
 }

 CPU_FOREACH(cpu) {
@@ -1013,7 +1015,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
 static void qemu_kvm_wait_io_event(CPUState *cpu)
 {
 while (cpu_thread_is_idle(cpu)) {
-qemu_cond_wait(cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(cpu->halt_cond);
 }

 qemu_kvm_eat_signals(cpu);
@@ -1123,7 +1125,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)

 /* wait for initial kick-off after machine start */
 while (first_cpu->stopped) {
-qemu_cond_wait(first_cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(first_cpu->halt_cond);

 /* process any pending work */
 CPU_FOREACH(cpu) {
@@ -1215,6 +1217,13 @@ bool qemu_mutex_iothread_locked(void)
 return iothread_locked;
 }

+static void qemu_cond_wait_iothread(QemuCond *cond)
+{
+iothread_locked = false;
+qemu_cond_wait(cond, _global_mutex);
+iothread_locked = true;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
 atomic_inc(_requesting_mutex);
@@ -1277,7 +1286,7 @@ void pause_all_vcpus(void)
 }

 while (!all_vcpus_paused()) {
-qemu_cond_wait(_pause_cond, _global_mutex);
+qemu_cond_wait_iothread(_pause_cond);
 CPU_FOREACH(cpu) {
 qemu_cpu_kick(cpu);
 }
@@ -1326,7 +1335,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
 cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 tcg_cpu_thread = cpu->thread;
 } else {
@@ -1347,7 +1356,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
 qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
cpu, QEMU_THREAD_JOINABLE);
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 }

@@ -1363,7 +1372,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
 qemu_thread_create(cpu->thread, thread_name, 
qemu_dummy_cpu_thread_fn, cpu,

QEMU_THREAD_JOINABLE);
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 }

--
1.9.1



Am 25.11.2015 um 10:08 schrieb David Engraf:

Hi Paolo,

Am 24.11.2015 um 16:54 schrieb Paolo Bonzini:

On 24/11/2015 16:43, David Engraf wrote:

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function
qemu_mutex_iothread_locked to avoid recursive locking of the iothread
lock. The iothread_locked variable uses the __thr

Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Am 25.11.2015 um 14:26 schrieb Paolo Bonzini:



On 25/11/2015 13:31, David Engraf wrote:

Hi Paolo,

please check the new version. I removed changing the iothread_locked
variable. But I still need to set the correct value of iothread_locked
when using qemu_cond_wait.


No, you don't.  Who is reading iothread_locked during
qemu_cond_wait_iothread?  No one, because it is a thread-local variable
whose address is never taken.


prepare_mmio_access is reading iothread_locked by using 
qemu_mutex_iothread_locked after qemu_tcg_wait_io_event calls 
qemu_cond_wait. All one the same thread.


qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
   -> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec
-> cpu_exec ends up in calling prepare_mmio_access

David

[Qemu-devel] [PATCH] qemu_mutex_iothread_locked not correctly synchronized

[David Engraf]

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function
qemu_mutex_iothread_locked to avoid recursive locking of the iothread 
lock. The iothread_locked variable uses the __thread attribute which 
results in a per thread storage location whereas the qemu_global_mutex 
is not thread specific. This leads to race conditions because the 
variable is not in sync between threads. I triggered this problem 
reproducible on a Windows machine whereas Linux works fine.


After removing the __thread attribute, iothread_locked may still return 
an invalid state because some functions directly use qemu_cond_wait 
which will unlock and lock the qemu_global_mutex based on a condition. 
These calls must be synced with iothread_locked as well.


The patch removes the __thread flag from iothread_locked and adds a 
function called qemu_cond_wait_iothread to keep iothread_locked in sync.


Signed-off-by: David Engraf <david.eng...@sysgo.com>
---
 cpus.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/cpus.c b/cpus.c
index 877bd70..d7cdd11 100644
--- a/cpus.c
+++ b/cpus.c
@@ -70,6 +70,8 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;

+static void qemu_cond_wait_iothread(QemuCond *cond);
+
 /* vcpu throttling controls */
 static QEMUTimer *throttle_timer;
 static unsigned int throttle_percentage;
@@ -920,7 +922,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void 
*data), void *data)

 while (!atomic_mb_read()) {
 CPUState *self_cpu = current_cpu;

-qemu_cond_wait(_work_cond, _global_mutex);
+qemu_cond_wait_iothread(_work_cond);
 current_cpu = self_cpu;
 }
 }
@@ -998,11 +1000,11 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
/* Start accounting real time to the virtual clock if the CPUs
   are idle.  */
 qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
-qemu_cond_wait(cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(cpu->halt_cond);
 }

 while (iothread_requesting_mutex) {
-qemu_cond_wait(_io_proceeded_cond, _global_mutex);
+qemu_cond_wait_iothread(_io_proceeded_cond);
 }

 CPU_FOREACH(cpu) {
@@ -1013,7 +1015,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
 static void qemu_kvm_wait_io_event(CPUState *cpu)
 {
 while (cpu_thread_is_idle(cpu)) {
-qemu_cond_wait(cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(cpu->halt_cond);
 }

 qemu_kvm_eat_signals(cpu);
@@ -1123,7 +1125,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)

 /* wait for initial kick-off after machine start */
 while (first_cpu->stopped) {
-qemu_cond_wait(first_cpu->halt_cond, _global_mutex);
+qemu_cond_wait_iothread(first_cpu->halt_cond);

 /* process any pending work */
 CPU_FOREACH(cpu) {
@@ -1208,13 +1210,20 @@ bool qemu_in_vcpu_thread(void)
 return current_cpu && qemu_cpu_is_self(current_cpu);
 }

-static __thread bool iothread_locked = false;
+static bool iothread_locked;

 bool qemu_mutex_iothread_locked(void)
 {
 return iothread_locked;
 }

+static void qemu_cond_wait_iothread(QemuCond *cond)
+{
+iothread_locked = false;
+qemu_cond_wait(cond, _global_mutex);
+iothread_locked = true;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
 atomic_inc(_requesting_mutex);
@@ -1277,7 +1286,7 @@ void pause_all_vcpus(void)
 }

 while (!all_vcpus_paused()) {
-qemu_cond_wait(_pause_cond, _global_mutex);
+qemu_cond_wait_iothread(_pause_cond);
 CPU_FOREACH(cpu) {
 qemu_cpu_kick(cpu);
 }
@@ -1326,7 +1335,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
 cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 tcg_cpu_thread = cpu->thread;
 } else {
@@ -1347,7 +1356,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
 qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
cpu, QEMU_THREAD_JOINABLE);
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 }

@@ -1363,7 +1372,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
 qemu_thread_create(cpu->thread, thread_name, 
qemu_dummy_cpu_thread_fn, cpu,

QEMU_THREAD_JOINABLE);
 while (!cpu->created) {
-qemu_cond_wait(_cpu_cond, _global_mutex);
+qemu_cond_wait_iothread(_cpu_cond);
 }
 }

--
1.9.1