Re: [Qemu-devel] [PATCH v2] pflash_cfi01: fix per device sector length in CFI table
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 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 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->writeblock_size = 1 << pfl-
[Qemu-devel] [PATCH] hw/arm/virt: Fix non-secure flash mode
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) &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 duplicate kernel load and device tree overlap
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 --- 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, &bios_entry, &loadaddr, NULL, +bios_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_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, &bios_entry, &loadaddr, NULL, - NULL, NULL); -if (kernel_size < 0) { +bios_size = load_uimage(filename, &bios_entry, &cur_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_filenam
Re: [Qemu-devel] [PATCH] vexpress fix flash device-width
Am 05.01.2017 um 19:32 schrieb Peter Maydell: On 22 December 2016 at 14:02, David Engraf 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] pflash_cfi01: fix per device sector length in CFI table
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 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] pflash_cfi01: fix per device sector length in CFI table
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 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] vexpress fix flash device-width
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);
[Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap
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 --- 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, &bios_entry, &loadaddr, NULL, +bios_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_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, &bios_entry, &loadaddr, NULL, - NULL, NULL); -if (kernel_size < 0) { +bios_size = load_uimage(filename, &bios_entry, &cur_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_filenam
Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap
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 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, &bios_entry, &loadaddr, NULL, +bios_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_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, &bios_entry, &loadaddr, NULL, - NULL, NULL); -if (kernel_size < 0) { +bios_size = load_uimage(filename, &bios_entry, &cur_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 fr
Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap
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 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, &bios_entry, &loadaddr, NULL, +bios_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_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, &bios_entry, &loadaddr, NULL, - NULL, NULL); -if (kernel_size < 0) { +bios_size = load_uimage(filename, &bios_entry, &cur_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 wil
[Qemu-devel] [PATCH v2] PPC: e500: Fix duplicate kernel load and device tree overlap
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 --- 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, &bios_entry, &loadaddr, 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, &bios_entry, &cur_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, &bios_entry, &loadaddr, NULL, - NULL, NULL); -if (kernel_size < 0) { +payload_size = load_uimage(filename, &bios_entry, &cur_base, NULL, + NULL, NULL); +if (payload_size < 0) {
[Qemu-devel] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap
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 --- 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, &bios_entry, &loadaddr, 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, &bios_entry, &loadaddr, 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 compati
Re: [Qemu-devel] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap
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 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 --- 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, &bios_entry, &loadaddr, 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, &bios_entry, &loadaddr, 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); +} + /* Loa
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] PPC: e500: Fix duplicate kernel load and device tree overlap
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 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
[Qemu-devel] [PATCH v4] PPC: e500: Fix duplicate kernel load and device tree overlap
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 Reviewed-by: David Gibson --- 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, &bios_entry, &loadaddr, 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, &bios_entry, &loadaddr, 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 - * - * This ensures ba
[Qemu-devel] [PATCH] PPC e500: Fix gap between u-boot and kernel
This patch moves the gap between u-boot and kernel at the correct location. Signed-off-by: David Engraf --- 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] qemu_mutex_iothread_locked not correctly synchronized
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 --- 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(&wi.done)) { CPUState *self_cpu = current_cpu; -qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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, &qemu_global_mutex); +qemu_cond_wait_iothread(cpu->halt_cond); } while (iothread_requesting_mutex) { -qemu_cond_wait(&qemu_io_proceeded_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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, &qemu_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, &qemu_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, &qemu_global_mutex); +iothread_locked = true; +} + void qemu_mutex_lock_iothread(void) { atomic_inc(&iothread_requesting_mutex); @@ -1277,7 +1286,7 @@ void pause_all_vcpus(void) } while (!all_vcpus_paused()) { -qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_cpu_cond); } } -- 1.9.1
Re: [Qemu-devel] [PATCH] qemu_mutex_iothread_locked not correctly synchronized
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
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 --- 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(&wi.done)) { CPUState *self_cpu = current_cpu; -qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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, &qemu_global_mutex); +qemu_cond_wait_iothread(cpu->halt_cond); } while (iothread_requesting_mutex) { -qemu_cond_wait(&qemu_io_proceeded_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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, &qemu_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, &qemu_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, &qemu_global_mutex); +iothread_locked = true; +} + void qemu_mutex_lock_iothread(void) { atomic_inc(&iothread_requesting_mutex); @@ -1277,7 +1286,7 @@ void pause_all_vcpus(void) } while (!all_vcpus_paused()) { -qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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(&qemu_cpu_cond, &qemu_global_mutex); +qemu_cond_wait_iothread(&qemu_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 wrot
Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized
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
Re: [Qemu-devel] [PATCH v2] qemu_mutex_iothread_locked not correctly synchronized
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 v2] qemu_mutex_iothread_locked not correctly synchronized
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
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