Re: [PATCH 2/5] spapr/ddw: Remove confuse return value in spapr_phb_get_free_liobn()
On 2/16/23 09:25, Philippe Mathieu-Daudé wrote:
The '1' returned value isn't used because
spapr_phb_get_free_liobn_cb() isn't called recursively
(it is only called once in spapr_phb_get_free_liobn()).
The next commit will convert object_child_foreach()
handlers to return a boolean indicating error.
Remove this value to avoid confusion.
Signed-off-by: Philippe Mathieu-Daudé
---
Reviewed-by: Daniel Henrique Barboza
hw/ppc/spapr_rtas_ddw.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/ppc/spapr_rtas_ddw.c b/hw/ppc/spapr_rtas_ddw.c
index 7ba11382bc..98f1310c6e 100644
--- a/hw/ppc/spapr_rtas_ddw.c
+++ b/hw/ppc/spapr_rtas_ddw.c
@@ -51,7 +51,6 @@ static int spapr_phb_get_free_liobn_cb(Object *child, void
*opaque)
tcet = (SpaprTceTable *) object_dynamic_cast(child, TYPE_SPAPR_TCE_TABLE);
if (tcet && !tcet->nb_table) {
*(uint32_t *)opaque = tcet->liobn;
-return 1;
}
return 0;
}
Re: [PATCH 06/20] hw/riscv: Use generic DeviceState instead of PFlashCFI01
On 1/4/23 19:04, Philippe Mathieu-Daudé wrote:
Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.
Signed-off-by: Philippe Mathieu-Daudé
---
Reviewed-by: Daniel Henrique Barboza
hw/riscv/virt.c | 9 +
include/hw/riscv/virt.h | 3 +--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 400bd9329f..b421a9dc12 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -46,6 +46,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
#include "sysemu/tpm.h"
+#include "hw/block/flash.h"
#include "hw/pci/pci.h"
#include "hw/pci-host/gpex.h"
#include "hw/display/ramfb.h"
@@ -106,7 +107,7 @@ static MemMapEntry virt_high_pcie_memmap;
#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
-static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
+static DeviceState *virt_flash_create1(RISCVVirtState *s,
const char *name,
const char *alias_prop_name)
{
@@ -130,7 +131,7 @@ static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
object_property_add_alias(OBJECT(s), alias_prop_name,
OBJECT(dev), "drive");
-return PFLASH_CFI01(dev);
+return dev;
}
static void virt_flash_create(RISCVVirtState *s)
@@ -139,7 +140,7 @@ static void virt_flash_create(RISCVVirtState *s)
s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
}
-static void virt_flash_map1(PFlashCFI01 *flash,
+static void virt_flash_map1(DeviceState *flash,
hwaddr base, hwaddr size,
MemoryRegion *sysmem)
{
@@ -1517,7 +1518,7 @@ static void virt_machine_init(MachineState *machine)
for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
/* Map legacy -drive if=pflash to machine properties */
-pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
+pflash_cfi01_legacy_drive(s->flash[i],
drive_get(IF_PFLASH, 0, i));
}
virt_flash_map(s, system_memory);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index be4ab8fe7f..b700a46763 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -21,7 +21,6 @@
#include "hw/riscv/riscv_hart.h"
#include "hw/sysbus.h"
-#include "hw/block/flash.h"
#include "qom/object.h"
#define VIRT_CPUS_MAX_BITS 9
@@ -49,7 +48,7 @@ struct RISCVVirtState {
DeviceState *platform_bus_dev;
RISCVHartArrayState soc[VIRT_SOCKETS_MAX];
DeviceState *irqchip[VIRT_SOCKETS_MAX];
-PFlashCFI01 *flash[2];
+DeviceState *flash[2];
FWCfgState *fw_cfg;
int fdt_size;
Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC
On 11/3/22 23:31, BALATON Zoltan wrote:
On Thu, 3 Nov 2022, Daniel Henrique Barboza wrote:
On 11/3/22 09:51, BALATON Zoltan wrote:
On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
This is a respin of Bernhard's v4 with Freescale eSDHC implemented
as an 'UNIMP' region. See v4 cover here:
https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shen...@gmail.com/
Since v5:
- Rebased (ppc-next merged)
- Properly handle big-endian
Since v4:
- Do not rename ESDHC_* definitions to USDHC_*
- Do not modify SDHCIState structure
Supersedes: <20221031115402.91912-1-phi...@linaro.org>
Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
freeze for 7.2).
Could you please always use ppc-next to queue patches for the next upcoming
version and ppc-7.2 for the current version? Unless this makes your workflow
harder in which case ignore this but the reason I ask is because then it's
enough for me to only track ppc-next if I need to rebase patches on that and
don't have to add a new branch at every release (unless I have some patches to
rebase on it during a freeze but that's less likely than rebasing on your
queued patches for the next release xo using version for the current branch and
keep next for the future versions makes more sense to me).
Note that doing "ppc-7.2" for the current release and ppc-next for the
next release will not prevent you from adding a new branch at every
release, e.g. for the next release you would need to add a ppc-8.0
branch.
'ppc-next' is used like a standard, a way of telling 'this is the next
pull request that is going upstream'. Can we change it? Sure, but if the
idea is to avoid new branches every new release then I suggest the
following:
- ppc-next: keep it as is
- ppc-next-release/ppc-future: branch that will host any code for the next
release during the code freeze window. Note that this branch will become
'ppc-next' when the new release cycle begins
This would avoid changing everyone's workflow with the current ppc-next
usage, while also standardize a branch for the future release patches
during freeze.
As I said above if this means changing your or other's workflow making it more
inconvenient for you then just ignore my request as it does not worth the
trouble this might cause for others. So only change it if it's not much trouble.
As for using next for future release and versioned branch for current one in
freeze this might not completely eliminate the need to track it for me but
makes it much less likely as I only need the freeze branch when I have to
submit a fix during the freeze AND you also already have other fixes queued AND
those fixes conflict with my patch. This is very unlikely so in most cases I
can just base the fix on master during the freeze and not care about the freeze
branch only in very rare cases. It's much more likely that I have outstanding
patches that I have to rebase for the next release when you already queued
patches e.g. during a freeze (or during development before pull requests but
the latter case already uses ppc-next).
Philippe's solution to use something like ppc-freze, -fixes whatever without a
version number for pathces during a freeze would also work as then I only need
to track those two branches but this would also break your condition of always
using ppc-next for the next pull request so again if this causes any trouble
for others then just leave it as it is.
I think I'll actually make the change I proposed:
- ppc-next will always be the next incoming content for the current release,
like it's has been for some time now.
- ppc-future will consist of ppc-next + patches that didn't make the freeze.
And yeah, every change I make on ppc-next I'll rebase ppc-future accordingly.
I won't be using any versioned branch like ppc-7.2, ppc-8.0 or something.
These two branches can be used regardless of the current release number.
I'll do that later today.
Daniel
BTW, checkpatch complained about this line being too long (83 chars):
3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
WARNING: line over 80 characters
#150: FILE: hw/ppc/e500.c:1024:
+ pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
The code except is this:
if (pmc->has_esdhc) {
create_unimplemented_device("esdhc",
pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
MPC85XX_ESDHC_REGS_SIZE);
To get rid of the warning we would need to make a python-esque identation (line
break after "(" ) or create a new variable to hold the sum. Both seems overkill
so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
it.
Or you could break indentation and not start at the ( but 3 chars back. I.e.:
create_unimplemented_device("esdhc",
Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC
On 11/3/22 09:51, BALATON Zoltan wrote:
On Wed, 2 Nov 2022, Daniel Henrique Barboza wrote:
On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
This is a respin of Bernhard's v4 with Freescale eSDHC implemented
as an 'UNIMP' region. See v4 cover here:
https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shen...@gmail.com/
Since v5:
- Rebased (ppc-next merged)
- Properly handle big-endian
Since v4:
- Do not rename ESDHC_* definitions to USDHC_*
- Do not modify SDHCIState structure
Supersedes: <20221031115402.91912-1-phi...@linaro.org>
Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
freeze for 7.2).
Could you please always use ppc-next to queue patches for the next upcoming
version and ppc-7.2 for the current version? Unless this makes your workflow
harder in which case ignore this but the reason I ask is because then it's
enough for me to only track ppc-next if I need to rebase patches on that and
don't have to add a new branch at every release (unless I have some patches to
rebase on it during a freeze but that's less likely than rebasing on your
queued patches for the next release xo using version for the current branch and
keep next for the future versions makes more sense to me).
Note that doing "ppc-7.2" for the current release and ppc-next for the
next release will not prevent you from adding a new branch at every
release, e.g. for the next release you would need to add a ppc-8.0
branch.
'ppc-next' is used like a standard, a way of telling 'this is the next
pull request that is going upstream'. Can we change it? Sure, but if the
idea is to avoid new branches every new release then I suggest the
following:
- ppc-next: keep it as is
- ppc-next-release/ppc-future: branch that will host any code for the next
release during the code freeze window. Note that this branch will become
'ppc-next' when the new release cycle begins
This would avoid changing everyone's workflow with the current ppc-next
usage, while also standardize a branch for the future release patches
during freeze.
BTW, checkpatch complained about this line being too long (83 chars):
3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
WARNING: line over 80 characters
#150: FILE: hw/ppc/e500.c:1024:
+ pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
The code except is this:
if (pmc->has_esdhc) {
create_unimplemented_device("esdhc",
pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
MPC85XX_ESDHC_REGS_SIZE);
To get rid of the warning we would need to make a python-esque identation (line
break after "(" ) or create a new variable to hold the sum. Both seems overkill
so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
it.
Or you could break indentation and not start at the ( but 3 chars back. I.e.:
create_unimplemented_device("esdhc",
pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
MPC85XX_ESDHC_REGS_SIZE);
But I think it can be just ignored in this case.
And I'll follow it up with my usual plea in these cases: can we move the line
size warning to 100 chars? For QEMU 8.0? Pretty please?
I think the consensus was to keep 80 columns if possible, this is good becuase
you can open more files side by side (although it does not match well with the
long _ naming convention of glib and qemu) but we have a distinction between
checkpatch warning and error in line length. I think it will give error at 90
chars but as long as it's just warns that means: fix it if you can but in rare
cases if it's more readable with a slightly longer line then it is still
acceptable. I think that's the case here, splitting the line would be less
readable than a few chars longer line.
Yeah I know that we can usually ignore these warnings. I keep bringing
this up because it's weird to keep bothering with 80 chars per line when
people are using 28" flat screen monitors, multiple screen desktops
and so on.
Thanks,
Daniel
Regards,
BALATON Zoltan
Re: [PATCH v6 0/3] ppc/e500: Add support for eSDHC
On 11/1/22 19:29, Philippe Mathieu-Daudé wrote:
This is a respin of Bernhard's v4 with Freescale eSDHC implemented
as an 'UNIMP' region. See v4 cover here:
https://lore.kernel.org/qemu-devel/20221018210146.193159-1-shen...@gmail.com/
Since v5:
- Rebased (ppc-next merged)
- Properly handle big-endian
Since v4:
- Do not rename ESDHC_* definitions to USDHC_*
- Do not modify SDHCIState structure
Supersedes: <20221031115402.91912-1-phi...@linaro.org>
Queued in gitlab.com/danielhb/qemu/tree/ppc-8.0 (since we missed the
freeze for 7.2).
BTW, checkpatch complained about this line being too long (83 chars):
3/3 Checking commit bc7b8cc88560 (hw/ppc/e500: Add Freescale eSDHC to e500plat)
WARNING: line over 80 characters
#150: FILE: hw/ppc/e500.c:1024:
+pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
The code except is this:
if (pmc->has_esdhc) {
create_unimplemented_device("esdhc",
pmc->ccsrbar_base +
MPC85XX_ESDHC_REGS_OFFSET,
MPC85XX_ESDHC_REGS_SIZE);
To get rid of the warning we would need to make a python-esque identation (line
break after "(" ) or create a new variable to hold the sum. Both seems overkill
so I'll ignore the warning. Phil is welcome to re-send if he thinks it's worth
it.
And I'll follow it up with my usual plea in these cases: can we move the line
size
warning to 100 chars? For QEMU 8.0? Pretty please?
Daniel
Philippe Mathieu-Daudé (3):
hw/sd/sdhci: MMIO region is implemented in 32-bit accesses
hw/sd/sdhci: Support big endian SD host controller interfaces
hw/ppc/e500: Add Freescale eSDHC to e500plat
docs/system/ppc/ppce500.rst | 13 ++
hw/ppc/Kconfig | 2 ++
hw/ppc/e500.c | 48 -
hw/ppc/e500.h | 1 +
hw/ppc/e500plat.c | 1 +
hw/sd/sdhci-internal.h | 1 +
hw/sd/sdhci.c | 36 +---
include/hw/sd/sdhci.h | 1 +
8 files changed, 99 insertions(+), 4 deletions(-)
Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling
On 10/28/22 19:42, Philippe Mathieu-Daudé wrote:
On 28/10/22 17:09, Daniel Henrique Barboza wrote:
Bernhard,
The 32 builds aren't fancying this patch. The issue is down there:
On 10/18/22 18:01, Bernhard Beschow wrote:
Allows e500 boards to have their root file system reside on flash using
only builtin devices located in the eLBC memory region.
Note that the flash memory area is only created when a -pflash argument is
given, and that the size is determined by the given file. The idea is to
put users into control.
Signed-off-by: Bernhard Beschow
---
docs/system/ppc/ppce500.rst | 16
hw/ppc/Kconfig | 1 +
hw/ppc/e500.c | 79 +
3 files changed, 96 insertions(+)
@@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
pmc->platform_bus_base,
>pbus_dev->mmio);
+ dinfo = drive_get(IF_PFLASH, 0, 0);
+ if (dinfo) {
+ BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
+ BlockDriverState *bs = blk_bs(blk);
+ uint64_t size = bdrv_getlength(bs);
+ uint64_t mmio_size = pms->pbus_dev->mmio.size;
^ here. The issue is that on a 32 bit system it is not possible to cast the
Int128 type to uint64_t:
FAILED: libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o
3746cc -m32 -Ilibqemu-ppc64-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /builds/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /builds/danielhb/qemu -iquote /builds/danielhb/qemu/include -iquote /builds/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value
-Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc64-softmmu-config-devices.h"' -MD -MQ libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -MF libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o.d -o libqemu-ppc64-softmmu.fa.p/hw_ppc_e500.c.o -c ../hw/ppc/e500.c
3747../hw/ppc/e500.c: In function 'ppce500_init':
3748../hw/ppc/e500.c:1069:30: error: incompatible types when initializing type
'uint64_t' {aka 'long long unsigned int'} using type 'Int128'
3749 1069 | uint64_t mmio_size = pms->pbus_dev->mmio.size;
3750 | ^~~
3751[3207/5331] Compiling C object
libqemu-ppc64-softmmu.fa.p/hw_ppc_mpc8544_guts.c.o
What I did to solve the problem is this:
+ uint64_t mmio_size = int128_get64(pms->pbus_dev->mmio.size); >
This will get the lower 64 bits and return an uint64_t.
Note that this function will assert if mmio.size is bigger than UINT64_MAX, but
since you're doing an error(1) on the "if size > mmio_size" conditional, this
assert() is not introducing a new side effect. We'll just fail earlier with
a different error message.
Simply use:
memory_region_size(pms->pbus_dev->mmio);
Nice! I'll change it in-tree before re-sending the PR.
Daniel
Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling
Bernhard,
The 32 builds aren't fancying this patch. The issue is down there:
On 10/18/22 18:01, Bernhard Beschow wrote:
Allows e500 boards to have their root file system reside on flash using
only builtin devices located in the eLBC memory region.
Note that the flash memory area is only created when a -pflash argument is
given, and that the size is determined by the given file. The idea is to
put users into control.
Signed-off-by: Bernhard Beschow
---
docs/system/ppc/ppce500.rst | 16
hw/ppc/Kconfig | 1 +
hw/ppc/e500.c | 79 +
3 files changed, 96 insertions(+)
diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index 7b5eb3c4ee..38f8ceb0cf 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
.. code-block:: bash
-netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
+
+Root file system on flash drive
+---
+
+Rather than using a root file system on ram disk, it is possible to have it on
+CFI flash. Given an ext2 image whose size must be a power of two, it can be
used
+as follows:
+
+.. code-block:: bash
+
+ $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
+ -display none -serial stdio \
+ -kernel vmlinux \
+ -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
+ -append "rootwait root=/dev/mtdblock0"
+
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 791fe78a50..769a1ead1c 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -126,6 +126,7 @@ config E500
select ETSEC
select GPIO_MPC8XXX
select OPENPIC
+select PFLASH_CFI01
select PLATFORM_BUS
select PPCE500_PCI
select SERIAL
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3e950ea3ba..73198adac8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -23,8 +23,10 @@
#include "e500-ccsr.h"
#include "net/net.h"
#include "qemu/config-file.h"
+#include "hw/block/flash.h"
#include "hw/char/serial.h"
#include "hw/pci/pci.h"
+#include "sysemu/block-backend-io.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
#include "sysemu/reset.h"
@@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice
*sbdev, void *opaque)
}
}
+static void create_devtree_flash(SysBusDevice *sbdev,
+ PlatformDevtreeData *data)
+{
+g_autofree char *name = NULL;
+uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
+ "num-blocks",
+ _fatal);
+uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
+ "sector-length",
+ _fatal);
+uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
+ "width",
+ _fatal);
+hwaddr flashbase = 0;
+hwaddr flashsize = num_blocks * sector_length;
+void *fdt = data->fdt;
+
+name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
+qemu_fdt_add_subnode(fdt, name);
+qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
+qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+ 1, flashbase, 1, flashsize);
+qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
+}
+
static void platform_bus_create_devtree(PPCE500MachineState *pms,
void *fdt, const char *mpic)
{
@@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState
*pms,
uint64_t addr = pmc->platform_bus_base;
uint64_t size = pmc->platform_bus_size;
int irq_start = pmc->platform_bus_first_irq;
+SysBusDevice *sbdev;
+bool ambiguous;
/* Create a /platform node that we can put all devices into */
@@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
/* Loop through all dynamic sysbus devices and create nodes for them */
foreach_dynamic_sysbus_device(sysbus_device_create_devtree, );
+sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
+));
+if (sbdev) {
+assert(!ambiguous);
+create_devtree_flash(sbdev, );
+}
+
g_free(node);
}
@@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
IrqLines *irqs;
DeviceState *dev, *mpicdev;
+DriveInfo *dinfo;
CPUPPCState *firstenv = NULL;
MemoryRegion *ccsr_addr_space;
SysBusDevice *s;
@@ -1024,6 +1061,48 @@ void ppce500_init(MachineState *machine)
pmc->platform_bus_base,
Re: [PATCH v5 00/13] Instantiate VT82xx functions in host device
On 10/27/22 05:21, Bernhard Beschow wrote: Am 16. September 2022 14:36:05 UTC schrieb "Philippe Mathieu-Daudé" : On 12/9/22 21:50, Bernhard Beschow wrote: Am 1. September 2022 11:41:14 UTC schrieb Bernhard Beschow : Testing done: * `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel morphos-3.17/boot.img` Boots successfully and it is possible to open games and tools. * I was unable to test the fuloong2e board even before this series since it seems to be unfinished [1]. A buildroot-baked kernel [2] booted but doesn't find its root partition, though the issues could be in the buildroot receipt I created. [1] https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 [2] https://github.com/shentok/buildroot/commits/fuloong2e Copying from v2 (just found it in my spam folder :/): Series: Reviewed-by: Philippe Mathieu-Daudé Review seems complete, thanks to all who participated! Now we just need someone to queue this series. Best regards, Bernhard Excellent cleanup! Series queued to mips-next. Hi Phil, would you mind doing a pull request in time for 7.2? I believe Phil was having problems with his amsat.org email. It's better to CC him using his work email phi...@linaro.org (just added it). Phil, since this has pegasos2 changes I can queue it up via ppc-next if you like. I'll toss a PR tomorrow. Daniel Thanks, Bernhard
Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup
On 10/26/22 16:51, B wrote:
Am 26. Oktober 2022 17:18:14 UTC schrieb Daniel Henrique Barboza
:
Hi,
Since this is being sent to qemu-ppc and has to do with e500 I decided to
take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as well
but I'd rather not ack it it's SD specific code.
I'll send a PowerPC pull request this week. I can grab this series via the ppc
tree if someone with SD authority acks patch 6.
This would be awesome. If we can't reach consensus on the eSDHC device until
then perhaps you could pull everything but the last two patches?
That's fair enough. Just applied 1-5 to ppc-next.
I'll send a PR most likely Friday. If patch 6 gets an ACK until then I'll
pick 6 and 7 as well.
I'll be offline start of next week so this will be my last PR before the freeze.
If a patch 6 ACK arrives after Friday we'll need the SD maintainers to pick
those
in before the freeze. Patch 7 has my ACK so feel free to take it.
Thanks Daniel for generally absorbing any patches floating around which look
remotely useful for the ppc tree. This is rewarding.
Glad I'm able to help!
Daniel
Best regards,
Bernhard
Thanks,
Daniel
On 10/18/22 18:01, Bernhard Beschow wrote:
Cover letter:
~
This series adds support for -pflash and direct SD card access to the
PPC e500 boards. The idea is to increase compatibility with "real" firmware
images where only the bare minimum of drivers is compiled in.
The series is structured as follows:
Patches 1-4 perform some general cleanup which paves the way for the rest of
the series.
Patch 5 adds -pflash handling where memory-mapped flash can be added on
user's behalf. That is, the flash memory region in the eLBC is only added if
the -pflash argument is supplied. Note that the cfi01 device model becomes
stricter in checking the size of the emulated flash space.
Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
boards which was missing so far.
User documentation is also added as the new features become available.
Tesing done:
* `qeu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
if=pflash,file=rootfs.ext2,format=raw`
* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
id=mydrive,if=none,file=rootfs.ext2,format=raw`
The load was created using latest Buildroot with `make
qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
In both cases it was possible to log in and explore the root file system.
v4:
~~~
Zoltan:
- Don't suggest presence of qemu-system-ppc32 in documentation
Bin:
- New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
Peter:
- Inline pflash_cfi01_register() rather than modify it (similar to v2)
v3:
~~~
Phil:
- Also add power-of-2 fix to pflash_cfi02
- Resolve cfi01-specific assertion in e500 code
- Resolve unused define in eSDHC device model
- Resolve redundant alignment checks in eSDHC device model
Bin:
- Add dedicated flash chapter to documentation
Bernhard:
- Use is_power_of_2() instead of ctpop64() for better readability
- Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
- Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
v2:
~~~
Bin:
- Add source for MPC8544DS platform bus' memory map in commit message.
- Keep "ESDHC" in comment referring to Linux driver.
- Use "qemu-system-ppc{64|32} in documentation.
- Use g_autofree in device tree code.
- Remove unneeded device tree properties.
- Error out if pflash size doesn't fit into eLBC memory window.
- Remove unused ESDHC defines.
- Define macro ESDHC_WML for register offset with magic constant.
- Fix some whitespace issues when adding eSDHC device to e500.
Phil:
- Fix tense in commit message.
Bernhard Beschow (7):
docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
two
hw/sd/sdhci-internal: Unexport ESDHC defines
hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
hw/ppc/e500: Implement pflash handling
hw/sd/sdhci: Implement Freescale eSDHC device model
hw/ppc/e500: Add Freescale eSDHC to e500plat
docs/system/ppc/ppce500.rst | 38 +++-
hw/block/pflash_cfi01.c | 8 +-
hw/block/pflash_cfi02.c | 5 +
hw/ppc/Kconfig | 2 +
hw/ppc/e500.c | 114 +-
hw/ppc/e500.h | 1 +
hw/ppc/e500plat.c | 1 +
hw/sd/sdhci-internal.h | 20
hw/sd/sdhci.c | 183 +++-
include/hw/sd/sdhci.h | 3 +
10 files changed, 324 insertions(+), 51 deletions(-)
Re: [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup
Hi,
Since this is being sent to qemu-ppc and has to do with e500 I decided to
take a look. I acked the e500 related patches, 5 and 7. Patch 6 LGTM as well
but I'd rather not ack it it's SD specific code.
I'll send a PowerPC pull request this week. I can grab this series via the ppc
tree if someone with SD authority acks patch 6.
Thanks,
Daniel
On 10/18/22 18:01, Bernhard Beschow wrote:
Cover letter:
~
This series adds support for -pflash and direct SD card access to the
PPC e500 boards. The idea is to increase compatibility with "real" firmware
images where only the bare minimum of drivers is compiled in.
The series is structured as follows:
Patches 1-4 perform some general cleanup which paves the way for the rest of
the series.
Patch 5 adds -pflash handling where memory-mapped flash can be added on
user's behalf. That is, the flash memory region in the eLBC is only added if
the -pflash argument is supplied. Note that the cfi01 device model becomes
stricter in checking the size of the emulated flash space.
Patches 6 and 7 add a new device model - the Freescale eSDHC - to the e500
boards which was missing so far.
User documentation is also added as the new features become available.
Tesing done:
* `qeu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive
if=pflash,file=rootfs.ext2,format=raw`
* `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append
"console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive
id=mydrive,if=none,file=rootfs.ext2,format=raw`
The load was created using latest Buildroot with `make
qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type.
In both cases it was possible to log in and explore the root file system.
v4:
~~~
Zoltan:
- Don't suggest presence of qemu-system-ppc32 in documentation
Bin:
- New patch: docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
Peter:
- Inline pflash_cfi01_register() rather than modify it (similar to v2)
v3:
~~~
Phil:
- Also add power-of-2 fix to pflash_cfi02
- Resolve cfi01-specific assertion in e500 code
- Resolve unused define in eSDHC device model
- Resolve redundant alignment checks in eSDHC device model
Bin:
- Add dedicated flash chapter to documentation
Bernhard:
- Use is_power_of_2() instead of ctpop64() for better readability
- Only instantiate eSDHC device model in ppce500 (not used in MPC8544DS)
- Rebase onto gitlab.com/danielhb/qemu/tree/ppc-next
v2:
~~~
Bin:
- Add source for MPC8544DS platform bus' memory map in commit message.
- Keep "ESDHC" in comment referring to Linux driver.
- Use "qemu-system-ppc{64|32} in documentation.
- Use g_autofree in device tree code.
- Remove unneeded device tree properties.
- Error out if pflash size doesn't fit into eLBC memory window.
- Remove unused ESDHC defines.
- Define macro ESDHC_WML for register offset with magic constant.
- Fix some whitespace issues when adding eSDHC device to e500.
Phil:
- Fix tense in commit message.
Bernhard Beschow (7):
docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s)
hw/block/pflash_cfi0{1,2}: Error out if device length isn't a power of
two
hw/sd/sdhci-internal: Unexport ESDHC defines
hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
hw/ppc/e500: Implement pflash handling
hw/sd/sdhci: Implement Freescale eSDHC device model
hw/ppc/e500: Add Freescale eSDHC to e500plat
docs/system/ppc/ppce500.rst | 38 +++-
hw/block/pflash_cfi01.c | 8 +-
hw/block/pflash_cfi02.c | 5 +
hw/ppc/Kconfig | 2 +
hw/ppc/e500.c | 114 +-
hw/ppc/e500.h | 1 +
hw/ppc/e500plat.c | 1 +
hw/sd/sdhci-internal.h | 20
hw/sd/sdhci.c | 183 +++-
include/hw/sd/sdhci.h | 3 +
10 files changed, 324 insertions(+), 51 deletions(-)
Re: [PATCH v4 7/7] hw/ppc/e500: Add Freescale eSDHC to e500plat
On 10/18/22 18:01, Bernhard Beschow wrote:
Adds missing functionality to e500plat machine which increases the
chance of given "real" firmware images to access SD cards.
Signed-off-by: Bernhard Beschow
---
Reviewed-by: Daniel Henrique Barboza
docs/system/ppc/ppce500.rst | 12
hw/ppc/Kconfig | 1 +
hw/ppc/e500.c | 35 ++-
hw/ppc/e500.h | 1 +
hw/ppc/e500plat.c | 1 +
5 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index 38f8ceb0cf..c9fe0915dc 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -19,6 +19,7 @@ The ``ppce500`` machine supports the following devices:
* Power-off functionality via one GPIO pin
* 1 Freescale MPC8xxx PCI host controller
* VirtIO devices via PCI bus
+* 1 Freescale Enhanced Secure Digital Host controller (eSDHC)
* 1 Freescale Enhanced Triple Speed Ethernet controller (eTSEC)
Hardware configuration information
@@ -181,3 +182,14 @@ as follows:
-drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
-append "rootwait root=/dev/mtdblock0"
+Alternatively, the root file system can also reside on an emulated SD card
+whose size must again be a power of two:
+
+.. code-block:: bash
+
+ $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
+ -display none -serial stdio \
+ -kernel vmlinux \
+ -device sd-card,drive=mydrive \
+ -drive id=mydrive,if=none,file=/path/to/rootfs.ext2,format=raw \
+ -append "rootwait root=/dev/mmcblk0"
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 769a1ead1c..6e31f568ba 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -129,6 +129,7 @@ config E500
select PFLASH_CFI01
select PLATFORM_BUS
select PPCE500_PCI
+select SDHCI
select SERIAL
select MPC_I2C
select FDT_PPC
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 73198adac8..15d1f5ea00 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -48,6 +48,7 @@
#include "hw/net/fsl_etsec/etsec.h"
#include "hw/i2c/i2c.h"
#include "hw/irq.h"
+#include "hw/sd/sdhci.h"
#define EPAPR_MAGIC(0x45504150)
#define DTC_LOAD_PAD 0x180
@@ -66,11 +67,14 @@
#define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
#define MPC8544_PCI_REGS_OFFSET0x8000ULL
#define MPC8544_PCI_REGS_SIZE 0x1000ULL
+#define MPC85XX_ESDHC_REGS_OFFSET 0x2e000ULL
+#define MPC85XX_ESDHC_REGS_SIZE0x1000ULL
#define MPC8544_UTIL_OFFSET0xeULL
#define MPC8XXX_GPIO_OFFSET0x000FF000ULL
#define MPC8544_I2C_REGS_OFFSET0x3000ULL
#define MPC8XXX_GPIO_IRQ 47
#define MPC8544_I2C_IRQ43
+#define MPC85XX_ESDHC_IRQ 72
#define RTC_REGS_OFFSET0x68
#define PLATFORM_CLK_FREQ_HZ (400 * 1000 * 1000)
@@ -203,6 +207,22 @@ static void dt_i2c_create(void *fdt, const char *soc,
const char *mpic,
g_free(i2c);
}
+static void dt_sdhc_create(void *fdt, const char *parent, const char *mpic)
+{
+hwaddr mmio = MPC85XX_ESDHC_REGS_OFFSET;
+hwaddr size = MPC85XX_ESDHC_REGS_SIZE;
+int irq = MPC85XX_ESDHC_IRQ;
+g_autofree char *name = NULL;
+
+name = g_strdup_printf("%s/sdhc@%" PRIx64, parent, mmio);
+qemu_fdt_add_subnode(fdt, name);
+qemu_fdt_setprop(fdt, name, "sdhci,auto-cmd12", NULL, 0);
+qemu_fdt_setprop_phandle(fdt, name, "interrupt-parent", mpic);
+qemu_fdt_setprop_cells(fdt, name, "bus-width", 4);
+qemu_fdt_setprop_cells(fdt, name, "interrupts", irq, 0x2);
+qemu_fdt_setprop_cells(fdt, name, "reg", mmio, size);
+qemu_fdt_setprop_string(fdt, name, "compatible", "fsl,esdhc");
+}
typedef struct PlatformDevtreeData {
void *fdt;
@@ -553,6 +573,10 @@ static int ppce500_load_device_tree(PPCE500MachineState
*pms,
dt_rtc_create(fdt, "i2c", "rtc");
+/* sdhc */
+if (pmc->has_esdhc) {
+dt_sdhc_create(fdt, soc, mpic);
+}
gutil = g_strdup_printf("%s/global-utilities@%llx", soc,
MPC8544_UTIL_OFFSET);
@@ -982,7 +1006,8 @@ void ppce500_init(MachineState *machine)
0, qdev_get_gpio_in(mpicdev, 42), 399193,
serial_hd(1), DEVICE_BIG_ENDIAN);
}
-/* I2C */
+
+/* I2C */
dev = qdev_new("mpc-i2c");
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -992,6 +1017,14 @@ void ppce500_init(MachineState *machine)
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
+/* eSDHC */
+if (pmc->has_esdhc) {
+dev = qdev_new(TYPE_FSL_E
Re: [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling
On 10/18/22 18:01, Bernhard Beschow wrote:
Allows e500 boards to have their root file system reside on flash using
only builtin devices located in the eLBC memory region.
Note that the flash memory area is only created when a -pflash argument is
given, and that the size is determined by the given file. The idea is to
put users into control.
Signed-off-by: Bernhard Beschow
---
Reviewed-by: Daniel Henrique Barboza
docs/system/ppc/ppce500.rst | 16
hw/ppc/Kconfig | 1 +
hw/ppc/e500.c | 79 +
3 files changed, 96 insertions(+)
diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
index 7b5eb3c4ee..38f8ceb0cf 100644
--- a/docs/system/ppc/ppce500.rst
+++ b/docs/system/ppc/ppce500.rst
@@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
.. code-block:: bash
-netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device eTSEC,netdev=net0
+
+Root file system on flash drive
+---
+
+Rather than using a root file system on ram disk, it is possible to have it on
+CFI flash. Given an ext2 image whose size must be a power of two, it can be
used
+as follows:
+
+.. code-block:: bash
+
+ $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
+ -display none -serial stdio \
+ -kernel vmlinux \
+ -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
+ -append "rootwait root=/dev/mtdblock0"
+
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 791fe78a50..769a1ead1c 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -126,6 +126,7 @@ config E500
select ETSEC
select GPIO_MPC8XXX
select OPENPIC
+select PFLASH_CFI01
select PLATFORM_BUS
select PPCE500_PCI
select SERIAL
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3e950ea3ba..73198adac8 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -23,8 +23,10 @@
#include "e500-ccsr.h"
#include "net/net.h"
#include "qemu/config-file.h"
+#include "hw/block/flash.h"
#include "hw/char/serial.h"
#include "hw/pci/pci.h"
+#include "sysemu/block-backend-io.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
#include "sysemu/reset.h"
@@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice
*sbdev, void *opaque)
}
}
+static void create_devtree_flash(SysBusDevice *sbdev,
+ PlatformDevtreeData *data)
+{
+g_autofree char *name = NULL;
+uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
+ "num-blocks",
+ _fatal);
+uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
+ "sector-length",
+ _fatal);
+uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
+ "width",
+ _fatal);
+hwaddr flashbase = 0;
+hwaddr flashsize = num_blocks * sector_length;
+void *fdt = data->fdt;
+
+name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
+qemu_fdt_add_subnode(fdt, name);
+qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
+qemu_fdt_setprop_sized_cells(fdt, name, "reg",
+ 1, flashbase, 1, flashsize);
+qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
+}
+
static void platform_bus_create_devtree(PPCE500MachineState *pms,
void *fdt, const char *mpic)
{
@@ -276,6 +303,8 @@ static void platform_bus_create_devtree(PPCE500MachineState
*pms,
uint64_t addr = pmc->platform_bus_base;
uint64_t size = pmc->platform_bus_size;
int irq_start = pmc->platform_bus_first_irq;
+SysBusDevice *sbdev;
+bool ambiguous;
/* Create a /platform node that we can put all devices into */
@@ -302,6 +331,13 @@ static void platform_bus_create_devtree(PPCE500MachineState *pms,
/* Loop through all dynamic sysbus devices and create nodes for them */
foreach_dynamic_sysbus_device(sysbus_device_create_devtree, );
+sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
+));
+if (sbdev) {
+assert(!ambiguous);
+create_devtree_flash(sbdev, );
+}
+
g_free(node);
}
@@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
IrqLines *irqs;
DeviceState *dev, *mpicdev;
+DriveInfo *dinfo;
CPUPPCState *firstenv = NULL;
Memo
Re: [PATCH v2 00/13] ppc/e500: Add support for two types of flash, cleanup
On 10/9/22 00:30, Bin Meng wrote: On Sun, Oct 9, 2022 at 12:11 AM Bernhard Beschow wrote: Am 4. Oktober 2022 12:43:35 UTC schrieb Daniel Henrique Barboza : Hey, On 10/3/22 18:27, Philippe Mathieu-Daudé wrote: Hi Daniel, On 3/10/22 22:31, Bernhard Beschow wrote: Cover letter: ~ This series adds support for -pflash and direct SD card access to the PPC e500 boards. The idea is to increase compatibility with "real" firmware images where only the bare minimum of drivers is compiled in. Bernhard Beschow (13): hw/ppc/meson: Allow e500 boards to be enabled separately hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx docs/system/ppc/ppce500: Add heading for networking chapter hw/ppc/e500: Reduce usage of sysbus API hw/ppc/mpc8544ds: Rename wrongly named method hw/ppc/mpc8544ds: Add platform bus hw/ppc/e500: Remove if statement which is now always true This first part is mostly reviewed and can already go via your ppc-next queue. We're missing an ACK in patch 6/13: hw/ppc/mpc8544ds: Add platform bus Bin: Ping? Sorry for the delay. I have provided the R-b to this patch. Thanks for the review. Patches 1-7 queued in gitlab.com/danielhb/qemu/tree/ppc-next. Daniel Regards, Bin
Re: [PATCH v2 00/13] ppc/e500: Add support for two types of flash, cleanup
Hey, On 10/3/22 18:27, Philippe Mathieu-Daudé wrote: Hi Daniel, On 3/10/22 22:31, Bernhard Beschow wrote: Cover letter: ~ This series adds support for -pflash and direct SD card access to the PPC e500 boards. The idea is to increase compatibility with "real" firmware images where only the bare minimum of drivers is compiled in. Bernhard Beschow (13): hw/ppc/meson: Allow e500 boards to be enabled separately hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx docs/system/ppc/ppce500: Add heading for networking chapter hw/ppc/e500: Reduce usage of sysbus API hw/ppc/mpc8544ds: Rename wrongly named method hw/ppc/mpc8544ds: Add platform bus hw/ppc/e500: Remove if statement which is now always true This first part is mostly reviewed and can already go via your ppc-next queue. We're missing an ACK in patch 6/13: hw/ppc/mpc8544ds: Add platform bus I'll need some time to understand what's been doing there to provide my own R-b. Or you can toss a R-b there :D Thanks, Daniel hw/block/pflash_cfi01: Error out if device length isn't a power of two hw/ppc/e500: Implement pflash handling hw/sd/sdhci-internal: Unexport ESDHC defines hw/sd/sdhci: Rename ESDHC_* defines to USDHC_* hw/sd/sdhci: Implement Freescale eSDHC device model hw/ppc/e500: Add Freescale eSDHC to e500 boards This second part still need work. I can take it via the sdmmc-next queue. Regards, Phil.
Re: [PATCH 4/9] hw/ppc/spapr: Fix code style problems reported by checkpatch
On 9/19/22 20:17, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
---
Reviewed-by: Daniel Henrique Barboza
And I've queued it in gitlab.com/danielhb/qemu/tree/ppc-next since it's not
tied with the rest of the series. Thanks,
Daniel
include/hw/ppc/spapr.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 530d739b1d..04a95669ab 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -848,7 +848,8 @@ static inline uint64_t ppc64_phys_to_real(uint64_t addr)
static inline uint32_t rtas_ld(target_ulong phys, int n)
{
-return ldl_be_phys(_space_memory, ppc64_phys_to_real(phys + 4*n));
+return ldl_be_phys(_space_memory,
+ ppc64_phys_to_real(phys + 4 * n));
}
static inline uint64_t rtas_ldq(target_ulong phys, int n)
@@ -858,7 +859,7 @@ static inline uint64_t rtas_ldq(target_ulong phys, int n)
static inline void rtas_st(target_ulong phys, int n, uint32_t val)
{
-stl_be_phys(_space_memory, ppc64_phys_to_real(phys + 4*n), val);
+stl_be_phys(_space_memory, ppc64_phys_to_real(phys + 4 * n), val);
}
typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, SpaprMachineState *sm,
Re: [PATCH v5 00/13] Instantiate VT82xx functions in host device
On 9/8/22 05:34, Bernhard Beschow wrote: Am 1. September 2022 11:41:14 UTC schrieb Bernhard Beschow : v5: * Add patch "Inline vt82c686b_southbridge_init() and remove it" (Zoltan) * Use machine parameter when creating rtc-time alias (Zoltan) Testing done: Same as in v3. v4: * Fix in comment: AC97 Modem -> MC97 Modem (Zoltan) * Introduce TYPE_VT82C686B_USB_UHCI define (Zoltan) * Introduce TYPE_VIA_IDE define (for consistency) v3: * Replace pre increment by post increment in for loop (Zoltan) * Move class defines close to where the class is defined (Zoltan) Testing done: * `make check-avocado` Passes for boot_linux_console.py for mips64el_fuloong2e * `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel morphos-3.17/boot.img` Boots successfully and it is possible to open games and tools. v2: * Keep the call to pci_ide_create_devs() in board code for consistency (Zoltan) * Create rtc-time alias in board rather than in south bridge code * Remove stale comments about PCI functions (Zoltan) v1: This series instantiates all PCI functions of the VT82xx south bridges in the south bridges themselves. For the IDE function this is especially important since its interrupt routing is configured in the ISA function, hence doesn't make sense to instantiate it as a "Frankenstein" device. The interrupt routing is currently hardcoded and changing that is currently not in the scope of this series. Testing done: * `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel morphos-3.17/boot.img` Boots successfully and it is possible to open games and tools. * I was unable to test the fuloong2e board even before this series since it seems to be unfinished [1]. A buildroot-baked kernel [2] booted but doesn't find its root partition, though the issues could be in the buildroot receipt I created. [1] https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 [2] https://github.com/shentok/buildroot/commits/fuloong2e Ping Zoltan, would you mind giving your Reviewed-by for 'hw/mips/fuloong2e: Inline vt82c686b_southbridge_init() and remove it' explicitly? Perhaps I was too eager to omit it since I didn't want to put words in your mouth. What else is missing? Who would do the pull request? The bulk of the changes were done in hw/isa/vt82c686.c and hw/mips/fuloong2e.c. The Fuloong 2E maintainers are already CCed, so I believe they're already aware of this series. I did my test round with the PowerPC test suit with this series and it didn't break anything, so I acked all patches that changed hw/ppc/pegasos2.c. Feel free to push those changes in the Fuloong 2E pull request. Thanks, Daniel Thanks, Bernhard Bernhard Beschow (13): hw/isa/vt82c686: Resolve chip-specific realize methods hw/isa/vt82c686: Resolve unneeded attribute hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory() hw/isa/vt82c686: Reuse errp hw/isa/vt82c686: Introduce TYPE_VIA_IDE define hw/isa/vt82c686: Instantiate IDE function in host device hw/isa/vt82c686: Introduce TYPE_VT82C686B_USB_UHCI define hw/isa/vt82c686: Instantiate USB functions in host device hw/isa/vt82c686: Instantiate PM function in host device hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device hw/mips/fuloong2e: Inline vt82c686b_southbridge_init() and remove it hw/isa/vt82c686: Embed RTCState in host device hw/isa/vt82c686: Create rtc-time alias in boards instead configs/devices/mips64el-softmmu/default.mak | 1 - hw/ide/via.c | 2 +- hw/isa/Kconfig | 1 + hw/isa/vt82c686.c| 120 +++ hw/mips/fuloong2e.c | 39 +++--- hw/ppc/Kconfig | 1 - hw/ppc/pegasos2.c| 25 ++-- hw/usb/vt82c686-uhci-pci.c | 4 +- include/hw/isa/vt82c686.h| 4 +- 9 files changed, 126 insertions(+), 71 deletions(-) -- > 2.37.3
Re: [PATCH v5 13/13] hw/isa/vt82c686: Create rtc-time alias in boards instead
On 9/1/22 08:41, Bernhard Beschow wrote:
According to good QOM practice, an object should only deal with objects
of its own sub tree. Having devices create an alias on the machine
object doesn't respect this good practice. To resolve this, create the
alias in the machine's code.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/isa/vt82c686.c | 2 --
hw/mips/fuloong2e.c | 4
hw/ppc/pegasos2.c | 4
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 48cd4d0036..3f9bd0c04d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -632,8 +632,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
return;
}
-object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(>rtc),
- "date");
isa_connect_gpio_out(ISA_DEVICE(>rtc), 0, s->rtc.isairq);
for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 3c46215616..b478483706 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -295,6 +295,10 @@ static void mips_fuloong2e_init(MachineState *machine)
pci_dev = pci_create_simple_multifunction(pci_bus,
PCI_DEVFN(FULOONG2E_VIA_SLOT,
0),
true, TYPE_VT82C686B_ISA);
+object_property_add_alias(OBJECT(machine), "rtc-time",
+ object_resolve_path_component(OBJECT(pci_dev),
+"rtc"),
+ "date");
qdev_connect_gpio_out(DEVICE(pci_dev), 0, env->irq[5]);
dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 09fdb7557f..49b753c7cc 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -161,6 +161,10 @@ static void pegasos2_init(MachineState *machine)
/* VIA VT8231 South Bridge (multifunction PCI device) */
via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
TYPE_VT8231_ISA);
+object_property_add_alias(OBJECT(machine), "rtc-time",
+ object_resolve_path_component(OBJECT(via),
+"rtc"),
+ "date");
qdev_connect_gpio_out(DEVICE(via), 0,
qdev_get_gpio_in_named(pm->mv, "gpp", 31));
Re: [PATCH v5 10/13] hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device
On 9/1/22 08:41, Bernhard Beschow wrote:
The AC97 function's wakeup status is wired to the PM function and both
the AC97 and MC97 interrupt routing is determined by the ISA function.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/isa/vt82c686.c | 16
hw/mips/fuloong2e.c | 4
hw/ppc/pegasos2.c | 5 -
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index d048607079..91686e9570 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -553,6 +553,8 @@ struct ViaISAState {
PCIIDEState ide;
UHCIState uhci[2];
ViaPMState pm;
+PCIDevice ac97;
+PCIDevice mc97;
};
static const VMStateDescription vmstate_via = {
@@ -572,6 +574,8 @@ static void via_isa_init(Object *obj)
object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
object_initialize_child(obj, "uhci1", >uhci[0],
TYPE_VT82C686B_USB_UHCI);
object_initialize_child(obj, "uhci2", >uhci[1],
TYPE_VT82C686B_USB_UHCI);
+object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
+object_initialize_child(obj, "mc97", >mc97, TYPE_VIA_MC97);
}
static const TypeInfo via_isa_info = {
@@ -652,6 +656,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
return;
}
+
+/* Function 5: AC97 Audio */
+qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);
+if (!qdev_realize(DEVICE(>ac97), BUS(pci_bus), errp)) {
+return;
+}
+
+/* Function 6: MC97 Modem */
+qdev_prop_set_int32(DEVICE(>mc97), "addr", d->devfn + 6);
+if (!qdev_realize(DEVICE(>mc97), BUS(pci_bus), errp)) {
+return;
+}
}
/* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 377108d313..2d8723ab74 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -210,10 +210,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus,
int slot, qemu_irq intc,
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
*i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-
-/* Audio support */
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
}
/* Network support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index e32944ee2b..09fdb7557f 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -159,7 +159,6 @@ static void pegasos2_init(MachineState *machine)
pci_bus = mv64361_get_pci_bus(pm->mv, 1);
/* VIA VT8231 South Bridge (multifunction PCI device) */
-/* VT8231 function 0: PCI-to-ISA Bridge */
via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
TYPE_VT8231_ISA);
qdev_connect_gpio_out(DEVICE(via), 0,
@@ -173,10 +172,6 @@ static void pegasos2_init(MachineState *machine)
spd_data = spd_data_generate(DDR, machine->ram_size);
smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
-/* VT8231 function 5-6: AC97 Audio & Modem */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
-pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
-
/* other PC hardware */
pci_vga_init(pci_bus);
Re: [PATCH v5 09/13] hw/isa/vt82c686: Instantiate PM function in host device
On 9/1/22 08:41, Bernhard Beschow wrote:
The PM controller has activity bits which monitor activity of other
built-in devices in the host device.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/isa/vt82c686.c | 13 +
hw/mips/fuloong2e.c | 2 +-
hw/ppc/pegasos2.c | 3 +--
include/hw/isa/vt82c686.h | 2 --
4 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f05fd9948a..d048607079 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -250,6 +250,8 @@ static const ViaPMInitInfo vt82c686b_pm_init_info = {
.device_id = PCI_DEVICE_ID_VIA_82C686B_PM,
};
+#define TYPE_VT82C686B_PM "vt82c686b-pm"
+
static const TypeInfo vt82c686b_pm_info = {
.name = TYPE_VT82C686B_PM,
.parent= TYPE_VIA_PM,
@@ -261,6 +263,8 @@ static const ViaPMInitInfo vt8231_pm_init_info = {
.device_id = PCI_DEVICE_ID_VIA_8231_PM,
};
+#define TYPE_VT8231_PM "vt8231-pm"
+
static const TypeInfo vt8231_pm_info = {
.name = TYPE_VT8231_PM,
.parent= TYPE_VIA_PM,
@@ -548,6 +552,7 @@ struct ViaISAState {
ViaSuperIOState via_sio;
PCIIDEState ide;
UHCIState uhci[2];
+ViaPMState pm;
};
static const VMStateDescription vmstate_via = {
@@ -641,6 +646,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
return;
}
}
+
+/* Function 4: Power Management */
+qdev_prop_set_int32(DEVICE(>pm), "addr", d->devfn + 4);
+if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
+return;
+}
}
/* TYPE_VT82C686B_ISA */
@@ -683,6 +694,7 @@ static void vt82c686b_init(Object *obj)
ViaISAState *s = VIA_ISA(obj);
object_initialize_child(obj, "sio", >via_sio, TYPE_VT82C686B_SUPERIO);
+object_initialize_child(obj, "pm", >pm, TYPE_VT82C686B_PM);
}
static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -746,6 +758,7 @@ static void vt8231_init(Object *obj)
ViaISAState *s = VIA_ISA(obj);
object_initialize_child(obj, "sio", >via_sio, TYPE_VT8231_SUPERIO);
+object_initialize_child(obj, "pm", >pm, TYPE_VT8231_PM);
}
static void vt8231_class_init(ObjectClass *klass, void *data)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index dc92223b76..377108d313 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,7 +208,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
*i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
/* Audio support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 85cca6f7a6..e32944ee2b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -168,8 +168,7 @@ static void pegasos2_init(MachineState *machine)
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
-/* VT8231 function 4: Power Management Controller */
-dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "pm"));
i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
spd_data = spd_data_generate(DDR, machine->ram_size);
smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index e6f6dd4d43..eaa07881c5 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -4,10 +4,8 @@
#include "hw/pci/pci.h"
#define TYPE_VT82C686B_ISA "vt82c686b-isa"
-#define TYPE_VT82C686B_PM "vt82c686b-pm"
#define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
#define TYPE_VT8231_ISA "vt8231-isa"
-#define TYPE_VT8231_PM "vt8231-pm"
#define TYPE_VIA_AC97 "via-ac97"
#define TYPE_VIA_IDE "via-ide"
#define TYPE_VIA_MC97 "via-mc97"
Re: [PATCH v5 07/13] hw/isa/vt82c686: Introduce TYPE_VT82C686B_USB_UHCI define
On 9/1/22 08:41, Bernhard Beschow wrote:
Suggested-by: BALATON Zoltan
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/mips/fuloong2e.c| 4 ++--
hw/ppc/pegasos2.c | 4 ++--
hw/usb/vt82c686-uhci-pci.c | 4 ++--
include/hw/isa/vt82c686.h | 1 +
4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 32605901e7..6b7370f2aa 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,8 +208,8 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
+pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), TYPE_VT82C686B_USB_UHCI);
+pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), TYPE_VT82C686B_USB_UHCI);
dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
*i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 8bc528a560..70776558c9 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -169,8 +169,8 @@ static void pegasos2_init(MachineState *machine)
pci_ide_create_devs(dev);
/* VT8231 function 2-3: USB Ports */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
-pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
+pci_create_simple(pci_bus, PCI_DEVFN(12, 2), TYPE_VT82C686B_USB_UHCI);
+pci_create_simple(pci_bus, PCI_DEVFN(12, 3), TYPE_VT82C686B_USB_UHCI);
/* VT8231 function 4: Power Management Controller */
dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index 0bf2b72ff0..46a901f56f 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -31,7 +31,7 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error
**errp)
static UHCIInfo uhci_info[] = {
{
-.name = "vt82c686b-usb-uhci",
+.name = TYPE_VT82C686B_USB_UHCI,
.vendor_id = PCI_VENDOR_ID_VIA,
.device_id = PCI_DEVICE_ID_VIA_UHCI,
.revision = 0x01,
@@ -45,7 +45,7 @@ static UHCIInfo uhci_info[] = {
static const TypeInfo vt82c686b_usb_uhci_type_info = {
.parent = TYPE_UHCI,
-.name = "vt82c686b-usb-uhci",
+.name = TYPE_VT82C686B_USB_UHCI,
.class_init = uhci_data_class_init,
.class_data = uhci_info,
};
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 87aca3e5bb..e6f6dd4d43 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -5,6 +5,7 @@
#define TYPE_VT82C686B_ISA "vt82c686b-isa"
#define TYPE_VT82C686B_PM "vt82c686b-pm"
+#define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
#define TYPE_VT8231_ISA "vt8231-isa"
#define TYPE_VT8231_PM "vt8231-pm"
#define TYPE_VIA_AC97 "via-ac97"
Re: [PATCH v5 08/13] hw/isa/vt82c686: Instantiate USB functions in host device
On 9/1/22 08:41, Bernhard Beschow wrote:
The USB functions can be enabled/disabled through the ISA function. Also
its interrupt routing can be influenced there.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/isa/vt82c686.c | 12
hw/mips/fuloong2e.c | 3 ---
hw/ppc/pegasos2.c | 4
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 63c1e3b8ce..f05fd9948a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -23,6 +23,7 @@
#include "hw/intc/i8259.h"
#include "hw/irq.h"
#include "hw/dma/i8257.h"
+#include "hw/usb/hcd-uhci.h"
#include "hw/timer/i8254.h"
#include "hw/rtc/mc146818rtc.h"
#include "migration/vmstate.h"
@@ -546,6 +547,7 @@ struct ViaISAState {
qemu_irq *isa_irqs;
ViaSuperIOState via_sio;
PCIIDEState ide;
+UHCIState uhci[2];
};
static const VMStateDescription vmstate_via = {
@@ -563,6 +565,8 @@ static void via_isa_init(Object *obj)
ViaISAState *s = VIA_ISA(obj);
object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
+object_initialize_child(obj, "uhci1", >uhci[0],
TYPE_VT82C686B_USB_UHCI);
+object_initialize_child(obj, "uhci2", >uhci[1],
TYPE_VT82C686B_USB_UHCI);
}
static const TypeInfo via_isa_info = {
@@ -629,6 +633,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
return;
}
+
+/* Functions 2-3: USB Ports */
+for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
+qdev_prop_set_int32(DEVICE(>uhci[i]), "addr", d->devfn + 2 + i);
+if (!qdev_realize(DEVICE(>uhci[i]), BUS(pci_bus), errp)) {
+return;
+}
+}
}
/* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 6b7370f2aa..dc92223b76 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -208,9 +208,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), TYPE_VT82C686B_USB_UHCI);
-pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), TYPE_VT82C686B_USB_UHCI);
-
dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
*i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 70776558c9..85cca6f7a6 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -168,10 +168,6 @@ static void pegasos2_init(MachineState *machine)
dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
-/* VT8231 function 2-3: USB Ports */
-pci_create_simple(pci_bus, PCI_DEVFN(12, 2), TYPE_VT82C686B_USB_UHCI);
-pci_create_simple(pci_bus, PCI_DEVFN(12, 3), TYPE_VT82C686B_USB_UHCI);
-
/* VT8231 function 4: Power Management Controller */
dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
Re: [PATCH v5 06/13] hw/isa/vt82c686: Instantiate IDE function in host device
On 9/1/22 08:41, Bernhard Beschow wrote:
The IDE function is closely tied to the ISA function (e.g. the IDE
interrupt routing happens there), so it makes sense that the IDE
function is instantiated within the south bridge itself.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
configs/devices/mips64el-softmmu/default.mak | 1 -
hw/isa/Kconfig | 1 +
hw/isa/vt82c686.c| 17 +
hw/mips/fuloong2e.c | 8
hw/ppc/Kconfig | 1 -
hw/ppc/pegasos2.c| 9 -
6 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/configs/devices/mips64el-softmmu/default.mak
b/configs/devices/mips64el-softmmu/default.mak
index c610749ac1..d5188f7ea5 100644
--- a/configs/devices/mips64el-softmmu/default.mak
+++ b/configs/devices/mips64el-softmmu/default.mak
@@ -1,7 +1,6 @@
# Default configuration for mips64el-softmmu
include ../mips-softmmu/common.mak
-CONFIG_IDE_VIA=y
CONFIG_FULOONG=y
CONFIG_LOONGSON3V=y
CONFIG_ATI_VGA=y
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index d42143a991..20de7e9294 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -53,6 +53,7 @@ config VT82C686
select I8254
select I8257
select I8259
+select IDE_VIA
select MC146818RTC
select PARALLEL
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 37e37b3855..63c1e3b8ce 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -17,6 +17,7 @@
#include "hw/isa/vt82c686.h"
#include "hw/pci/pci.h"
#include "hw/qdev-properties.h"
+#include "hw/ide/pci.h"
#include "hw/isa/isa.h"
#include "hw/isa/superio.h"
#include "hw/intc/i8259.h"
@@ -544,6 +545,7 @@ struct ViaISAState {
qemu_irq cpu_intr;
qemu_irq *isa_irqs;
ViaSuperIOState via_sio;
+PCIIDEState ide;
};
static const VMStateDescription vmstate_via = {
@@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
}
};
+static void via_isa_init(Object *obj)
+{
+ViaISAState *s = VIA_ISA(obj);
+
+object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
+}
+
static const TypeInfo via_isa_info = {
.name = TYPE_VIA_ISA,
.parent= TYPE_PCI_DEVICE,
.instance_size = sizeof(ViaISAState),
+.instance_init = via_isa_init,
.abstract = true,
.interfaces= (InterfaceInfo[]) {
{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
{
ViaISAState *s = VIA_ISA(d);
DeviceState *dev = DEVICE(d);
+PCIBus *pci_bus = pci_get_bus(d);
qemu_irq *isa_irq;
ISABus *isa_bus;
int i;
@@ -612,6 +623,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
if (!qdev_realize(DEVICE(>via_sio), BUS(isa_bus), errp)) {
return;
}
+
+/* Function 1: IDE */
+qdev_prop_set_int32(DEVICE(>ide), "addr", d->devfn + 1);
+if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
+return;
+}
}
/* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 44225fbe33..32605901e7 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -199,13 +199,13 @@ static void main_cpu_reset(void *opaque)
static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq
intc,
I2CBus **i2c_bus)
{
-PCIDevice *dev;
+PCIDevice *dev, *via;
-dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
+via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
TYPE_VT82C686B_ISA);
-qdev_connect_gpio_out(DEVICE(dev), 0, intc);
+qdev_connect_gpio_out(DEVICE(via), 0, intc);
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), TYPE_VIA_IDE);
+dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
pci_ide_create_devs(dev);
pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 400511c6b7..18565e966b 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -74,7 +74,6 @@ config PEGASOS2
bool
select MV64361
select VT82C686
-select IDE_VIA
select SMBUS_EEPROM
select VOF
# This should come with VT82C686
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 8039775f80..8bc528a560 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -102,7 +102,7 @@ static void pegasos2_init(MachineState *machine)
CPUPPCState *env;
MemoryRegion *rom = g_new(MemoryRegion, 1);
PCIBus *pci_bus;
-PCIDevice *dev;
+PCIDevice *dev, *via;
I2CBus *i2c_bus;
const char *fwname = ma
Re: [PATCH v5 05/13] hw/isa/vt82c686: Introduce TYPE_VIA_IDE define
On 9/1/22 08:41, Bernhard Beschow wrote:
Establishes consistency with other (VIA) devices.
Signed-off-by: Bernhard Beschow
---
Acked-by: Daniel Henrique Barboza
hw/ide/via.c | 2 +-
hw/mips/fuloong2e.c | 2 +-
hw/ppc/pegasos2.c | 2 +-
include/hw/isa/vt82c686.h | 1 +
4 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..e1a429405d 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -230,7 +230,7 @@ static void via_ide_class_init(ObjectClass *klass, void
*data)
}
static const TypeInfo via_ide_info = {
-.name = "via-ide",
+.name = TYPE_VIA_IDE,
.parent= TYPE_PCI_IDE,
.class_init= via_ide_class_init,
};
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 5ee546f5f6..44225fbe33 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -205,7 +205,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
TYPE_VT82C686B_ISA);
qdev_connect_gpio_out(DEVICE(dev), 0, intc);
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), TYPE_VIA_IDE);
pci_ide_create_devs(dev);
pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..8039775f80 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -166,7 +166,7 @@ static void pegasos2_init(MachineState *machine)
qdev_get_gpio_in_named(pm->mv, "gpp", 31));
/* VT8231 function 1: IDE Controller */
-dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
+dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), TYPE_VIA_IDE);
pci_ide_create_devs(dev);
/* VT8231 function 2-3: USB Ports */
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 56ac141be3..87aca3e5bb 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -8,6 +8,7 @@
#define TYPE_VT8231_ISA "vt8231-isa"
#define TYPE_VT8231_PM "vt8231-pm"
#define TYPE_VIA_AC97 "via-ac97"
+#define TYPE_VIA_IDE "via-ide"
#define TYPE_VIA_MC97 "via-mc97"
void via_isa_set_irq(PCIDevice *d, int n, int level);
Re: [PATCH 1/2] Trivial: 3 char repeat typos
On 6/14/22 07:40, Dr. David Alan Gilbert (git) wrote:
From: "Dr. David Alan Gilbert"
Inspired by Julia Lawall's fixing of Linux
kernel comments, I looked at qemu, although I did it manually.
Signed-off-by: Dr. David Alan Gilbert
---
Reviewed-by: Daniel Henrique Barboza
hw/intc/openpic.c| 2 +-
hw/net/imx_fec.c | 2 +-
hw/pci/pcie_aer.c| 2 +-
hw/pci/shpc.c| 3 ++-
hw/ppc/spapr_caps.c | 2 +-
hw/scsi/spapr_vscsi.c| 2 +-
qapi/net.json| 2 +-
tools/virtiofsd/passthrough_ll.c | 2 +-
ui/input.c | 2 +-
9 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 49504e740f..b0787e8ee7 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -729,7 +729,7 @@ static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t
val, bool enabled)
}
/*
- * Returns the currrent tccr value, i.e., timer value (in clocks) with
+ * Returns the current tccr value, i.e., timer value (in clocks) with
* appropriate TOG.
*/
static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 0db9aaf76a..8c11b237de 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -438,7 +438,7 @@ static void imx_eth_update(IMXFECState *s)
* assignment fail.
*
* To ensure that all versions of Linux work, generate ENET_INT_MAC
- * interrrupts on both interrupt lines. This should be changed if and when
+ * interrupts on both interrupt lines. This should be changed if and when
* qemu supports IOMUX.
*/
if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] &
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 92bd0530dd..eff62f3945 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -323,7 +323,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const
PCIEAERMsg *msg)
*/
}
-/* Errro Message Received: Root Error Status register */
+/* Error Message Received: Root Error Status register */
switch (msg->severity) {
case PCI_ERR_ROOT_CMD_COR_EN:
if (root_status & PCI_ERR_ROOT_COR_RCV) {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index f822f18b98..e71f3a7483 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -480,7 +480,8 @@ static const MemoryRegionOps shpc_mmio_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
.valid = {
/* SHPC ECN requires dword accesses, but the original 1.0 spec
doesn't.
- * It's easier to suppport all sizes than worry about it. */
+ * It's easier to support all sizes than worry about it.
+ */
.min_access_size = 1,
.max_access_size = 4,
},
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 655ab856a0..b4283055c1 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -553,7 +553,7 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr,
uint8_t val,
* instruction is a harmless no-op. It won't correctly
* implement the cache count flush *but* if we have
* count-cache-disabled in the host, that flush is
- * unnnecessary. So, specifically allow this case. This
+ * unnecessary. So, specifically allow this case. This
* allows us to have better performance on POWER9 DD2.3,
* while still working on POWER9 DD2.2 and POWER8 host
* cpus.
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index a07a8e1523..e320ccaa23 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -1013,7 +1013,7 @@ static int vscsi_send_capabilities(VSCSIState *s,
vscsi_req *req)
}
/*
- * Current implementation does not suppport any migration or
+ * Current implementation does not support any migration or
* reservation capabilities. Construct the response telling the
* guest not to use them.
*/
diff --git a/qapi/net.json b/qapi/net.json
index d6f7cfd4d6..9af11e9a3b 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -298,7 +298,7 @@
#
# @udp: use the udp version of l2tpv3 encapsulation
#
-# @cookie64: use 64 bit coookies
+# @cookie64: use 64 bit cookies
#
# @counter: have sequence counter
#
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b15c631ca5..7a73dfcce9 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2319,7 +2319,7 @@ static int do_lo_create(fuse_req_t req, struct lo_inode
*parent_inode,
* If security.selinux has not been remapped and selinux is enabled,
* use fscreate to set context before file creation. If not, use
* tmpfile method for regular files. Otherwise fallback to
- * non-atomic method of file creation and xattr settting.
+ * non-atomic method of
Re: [PATCH] block-qdict: Fix -Werror=maybe-uninitialized build failure
On 3/10/22 17:05, Murilo Opsfelder Araujo wrote:
Building QEMU on Fedora 37 (Rawhide Prerelease) ppc64le failed with the
following error:
$ ../configure --prefix=/usr/local/qemu-disabletcg
--target-list=ppc-softmmu,ppc64-softmmu --disable-tcg --disable-linux-user
...
$ make -j$(nproc)
...
FAILED: libqemuutil.a.p/qobject_block-qdict.c.o
cc -m64 -mlittle-endian -Ilibqemuutil.a.p -I. -I..
-Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace -Iui
-Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
-I/usr/include/sysprof-4 -I/usr/include/lib
mount -I/usr/include/blkid -I/usr/include/gio-unix-2.0
-I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto -Wall
-Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /root/qemu/linux-headers
-isystem linux-headers -iquote
. -iquote /root/qemu -iquote /root/qemu/include -iquote
/root/qemu/disas/libvixl -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wundef -Wwrite
-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-label
s -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi
-fstack-protector-strong -fPIE -MD -MQ libqemuutil.a.p/qobject_block-qdict.c.o
-MF libqemuutil.a.p/qobject_block-qdict.c.o.d -
o libqemuutil.a.p/qobject_block-qdict.c.o -c ../qobject/block-qdict.c
In file included from /root/qemu/include/qapi/qmp/qdict.h:16,
from /root/qemu/include/block/qdict.h:13,
from ../qobject/block-qdict.c:11:
/root/qemu/include/qapi/qmp/qobject.h: In function ‘qdict_array_split’:
/root/qemu/include/qapi/qmp/qobject.h:49:17: error: ‘subqdict’ may be used
uninitialized [-Werror=maybe-uninitialized]
49 | typeof(obj) _obj = (obj); \
| ^~~~
../qobject/block-qdict.c:227:16: note: ‘subqdict’ declared here
227 | QDict *subqdict;
|^~~~
cc1: all warnings being treated as errors
Fix build failure by initializing the QDict variable with NULL.
Signed-off-by: Murilo Opsfelder Araujo
Cc: Kevin Wolf
Cc: Hanna Reitz
Cc: Markus Armbruster
---
Reviewed-by: Daniel Henrique Barboza
qobject/block-qdict.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 1487cc5dd8..b26524429c 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -224,7 +224,7 @@ void qdict_array_split(QDict *src, QList **dst)
for (i = 0; i < UINT_MAX; i++) {
QObject *subqobj;
bool is_subqdict;
-QDict *subqdict;
+QDict *subqdict = NULL;
char indexstr[32], prefix[32];
size_t snprintf_ret;
Re: [PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails
Ping On 1/30/20 6:39 PM, Daniel Henrique Barboza wrote: The version 8 of this patch series got buried and it's now conflicting with master. Rebase and re-sending it. Also, I contemplated the idea of moving/copying the password verification in qcrypto_block_luks_create() all the way back to the start of block_crypto_co_create_opts_luks(), failing early before the bdrv_create_file(), avoiding the problem altogether without the need of a delete_file API I'm trying to push here (see patch 03 commit message for detailed info about the bug). This idea was dropped after I saw that: - We would need to store the resulting password, now being retrieved early in block_crypto_co_create_opts_luks(), in a new QCryptoBlockCreateOptions string to be used inside qcrypto_block_luks_create() as intended. An alternative would be to call qcrypto_secret_lookup_as_utf8() twice, discarding the first string; - There are a lot of ways to fail in qcrypto_block_luks_create() other than a non-UTF8 password that would trigger the same problem. A more appropiate way of doing what I intended, instead of copying/hacking code around to fail before bdrv_create(), is some sort of bdrv_validate() API that would encapsulate everything that is related to user input validation for the security drivers. This API could then be called before the file creation (maybe inside bdrv_create itself) and fail early if needed. This is too overkill for what I'm trying to fix here, and I'm not sure if it would be a net gain compared to the delete_file API. All that said, I believe that this patch series presents a sane solution with the code we have ATM. changes in this version: - rebase with current master at 204aa60b37 - previous version: https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html Daniel Henrique Barboza (4): block: introducing 'bdrv_co_delete_file' interface block.c: adding bdrv_co_delete_file crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails qemu-iotests: adding LUKS cleanup for non-UTF8 secret error block.c| 26 +++ block/crypto.c | 18 ++ block/file-posix.c | 23 + include/block/block.h | 1 + include/block/block_int.h | 4 +++ tests/qemu-iotests/282 | 67 ++ tests/qemu-iotests/282.out | 11 +++ tests/qemu-iotests/group | 1 + 8 files changed, 151 insertions(+) create mode 100755 tests/qemu-iotests/282 create mode 100644 tests/qemu-iotests/282.out
[PATCH v9 2/4] block.c: adding bdrv_co_delete_file
Using the new 'bdrv_co_delete_file' interface, a pure co_routine function
'bdrv_co_delete_file' inside block.c can can be used in a way similar of
the existing bdrv_create_file to to clean up a created file.
We're creating a pure co_routine because the only caller of
'bdrv_co_delete_file' will be already in co_routine context, thus there
is no need to add all the machinery to check for qemu_in_coroutine() and
create a separated co_routine to do the job.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block.c | 26 ++
include/block/block.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/block.c b/block.c
index ecd09dbbfd..c26d8271a1 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,32 @@ int bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp)
return ret;
}
+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
+{
+Error *local_err = NULL;
+int ret;
+
+assert(bs != NULL);
+
+if (!bs->drv) {
+error_setg(errp, "Block node '%s' is not opened", bs->filename);
+return -ENOMEDIUM;
+}
+
+if (!bs->drv->bdrv_co_delete_file) {
+error_setg(errp, "Driver '%s' does not support image deletion",
+ bs->drv->format_name);
+return -ENOTSUP;
+}
+
+ret = bs->drv->bdrv_co_delete_file(bs, _err);
+if (ret < 0) {
+error_propagate(errp, local_err);
+}
+
+return ret;
+}
+
/**
* Try to get @bs's logical and physical block size.
* On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index e9dcfef7fa..f7db094859 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -373,6 +373,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs,
BlockDriverState *base,
int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
Error **errp);
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
typedef struct BdrvCheckResult {
--
2.24.1
[PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails
The version 8 of this patch series got buried and it's now conflicting with master. Rebase and re-sending it. Also, I contemplated the idea of moving/copying the password verification in qcrypto_block_luks_create() all the way back to the start of block_crypto_co_create_opts_luks(), failing early before the bdrv_create_file(), avoiding the problem altogether without the need of a delete_file API I'm trying to push here (see patch 03 commit message for detailed info about the bug). This idea was dropped after I saw that: - We would need to store the resulting password, now being retrieved early in block_crypto_co_create_opts_luks(), in a new QCryptoBlockCreateOptions string to be used inside qcrypto_block_luks_create() as intended. An alternative would be to call qcrypto_secret_lookup_as_utf8() twice, discarding the first string; - There are a lot of ways to fail in qcrypto_block_luks_create() other than a non-UTF8 password that would trigger the same problem. A more appropiate way of doing what I intended, instead of copying/hacking code around to fail before bdrv_create(), is some sort of bdrv_validate() API that would encapsulate everything that is related to user input validation for the security drivers. This API could then be called before the file creation (maybe inside bdrv_create itself) and fail early if needed. This is too overkill for what I'm trying to fix here, and I'm not sure if it would be a net gain compared to the delete_file API. All that said, I believe that this patch series presents a sane solution with the code we have ATM. changes in this version: - rebase with current master at 204aa60b37 - previous version: https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html Daniel Henrique Barboza (4): block: introducing 'bdrv_co_delete_file' interface block.c: adding bdrv_co_delete_file crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails qemu-iotests: adding LUKS cleanup for non-UTF8 secret error block.c| 26 +++ block/crypto.c | 18 ++ block/file-posix.c | 23 + include/block/block.h | 1 + include/block/block_int.h | 4 +++ tests/qemu-iotests/282 | 67 ++ tests/qemu-iotests/282.out | 11 +++ tests/qemu-iotests/group | 1 + 8 files changed, 151 insertions(+) create mode 100755 tests/qemu-iotests/282 create mode 100644 tests/qemu-iotests/282.out -- 2.24.1
[PATCH v9 1/4] block: introducing 'bdrv_co_delete_file' interface
Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.
This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c. The
implementation is given by 'raw_co_delete_file'.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block/file-posix.c| 23 +++
include/block/block_int.h | 4
2 files changed, 27 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 1b805bd938..ed28234bb8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2386,6 +2386,28 @@ static int coroutine_fn raw_co_create_opts(const char
*filename, QemuOpts *opts,
return raw_co_create(, errp);
}
+static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,
+ Error **errp)
+{
+struct stat st;
+int ret;
+
+if (!(stat(bs->filename, ) == 0) || !S_ISREG(st.st_mode)) {
+error_setg_errno(errp, ENOENT, "%s is not a regular file",
+ bs->filename);
+return -ENOENT;
+}
+
+ret = unlink(bs->filename);
+if (ret < 0) {
+ret = -errno;
+error_setg_errno(errp, -ret, "Error when deleting file %s",
+ bs->filename);
+}
+
+return ret;
+}
+
/*
* Find allocation range in @bs around offset @start.
* May change underlying file descriptor's file offset.
@@ -3017,6 +3039,7 @@ BlockDriver bdrv_file = {
.bdrv_co_block_status = raw_co_block_status,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+.bdrv_co_delete_file = raw_co_delete_file,
.bdrv_co_preadv = raw_co_preadv,
.bdrv_co_pwritev= raw_co_pwritev,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..d938d3e8d2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -314,6 +314,10 @@ struct BlockDriver {
*/
int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+/* Delete a created file. */
+int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
+Error **errp);
+
/*
* Flushes all data that was already written to the OS all the way down to
* the disk (for example file-posix.c calls fsync()).
--
2.24.1
[PATCH v9 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails
When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:
$ qemu-img create -f luks --object
secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o
key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760
key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not
valid UTF-8
However, the created file '/var/tmp/pool_target/vol_1' is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.
The volume file is created inside block_crypto_co_create_opts_luks(), in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.
This patch changes block_crypto_co_create_opts_luks() to delete
'filename' in case of failure. A failure in this point means that
the volume is now truncated/corrupted, so even if 'filename' was an
existing volume before calling qemu-img, it is now unusable. Deleting
the file it is not much worse than leaving it in the filesystem in
this scenario, and we don't have to deal with checking the file
pre-existence in the code.
* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.
Reported-by: Srikanth Aithal
Suggested-by: Kevin Wolf
Signed-off-by: Daniel Henrique Barboza
---
block/crypto.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..00e8ec537d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -30,6 +30,7 @@
#include "qapi/error.h"
#include "qemu/module.h"
#include "qemu/option.h"
+#include "qemu/cutils.h"
#include "crypto.h"
typedef struct BlockCrypto BlockCrypto;
@@ -596,6 +597,23 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
ret = 0;
fail:
+/*
+ * If an error occurred, delete 'filename'. Even if the file existed
+ * beforehand, it has been truncated and corrupted in the process.
+ */
+if (ret && bs) {
+Error *local_delete_err = NULL;
+int r_del = bdrv_co_delete_file(bs, _delete_err);
+/*
+ * ENOTSUP will happen if the block driver doesn't support
+ * the 'bdrv_co_delete_file' interface. This is a predictable
+ * scenario and shouldn't be reported back to the user.
+ */
+if ((r_del < 0) && (r_del != -ENOTSUP)) {
+error_report_err(local_delete_err);
+}
+}
+
bdrv_unref(bs);
qapi_free_QCryptoBlockCreateOptions(create_opts);
qobject_unref(cryptoopts);
--
2.24.1
[PATCH v9 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
This patch adds a new test file to exercise the case where
qemu-img fails to complete for the LUKS format when a non-UTF8
secret is used.
Signed-off-by: Daniel Henrique Barboza
---
tests/qemu-iotests/282 | 67 ++
tests/qemu-iotests/282.out | 11 +++
tests/qemu-iotests/group | 1 +
3 files changed, 79 insertions(+)
create mode 100755 tests/qemu-iotests/282
create mode 100644 tests/qemu-iotests/282.out
diff --git a/tests/qemu-iotests/282 b/tests/qemu-iotests/282
new file mode 100755
index 00..081eb12080
--- /dev/null
+++ b/tests/qemu-iotests/282
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret
+#
+# Copyright (C) 2020, IBM Corporation.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+TEST_IMAGE_FILE='vol.img'
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm non_utf8_secret
+ rm -f $TEST_IMAGE_FILE
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto generic
+_unsupported_proto vxhs
+
+echo "== Create non-UTF8 secret =="
+echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret
+SECRET="secret,id=sec0,file=non_utf8_secret"
+
+echo "== Throws an error because of invalid UTF-8 secret =="
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0"
$TEST_IMAGE_FILE 4M
+
+echo "== Image file should not exist after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+exit 1
+fi
+
+echo "== Create a stub image file and run qemu-img again =="
+touch $TEST_IMAGE_FILE
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0"
$TEST_IMAGE_FILE 4M
+
+echo "== Pre-existing image file should also be deleted after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+exit 1
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out
new file mode 100644
index 00..5d079dabce
--- /dev/null
+++ b/tests/qemu-iotests/282.out
@@ -0,0 +1,11 @@
+QA output created by 282
+== Create non-UTF8 secret ==
+== Throws an error because of invalid UTF-8 secret ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Image file should not exist after the error ==
+== Create a stub image file and run qemu-img again ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Pre-existing image file should also be deleted after the error ==
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index e041cc1ee3..9d8bf8f783 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -289,3 +289,4 @@
279 rw backing quick
280 rw migration quick
281 rw quick
+282 rw img quick
--
2.24.1
[PATCH v1 32/59] qemu-img.c: remove 'out4' label in img_compare
'out4' can be replaced by 'return 2'.
CC: Kevin Wolf
CC: Max Reitz
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
qemu-img.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..fc7b08e7ee 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1346,8 +1346,7 @@ static int img_compare(int argc, char **argv)
opts = qemu_opts_parse_noisily(_object_opts,
optarg, true);
if (!opts) {
-ret = 2;
-goto out4;
+return 2;
}
} break;
case OPTION_IMAGE_OPTS:
@@ -1371,8 +1370,7 @@ static int img_compare(int argc, char **argv)
if (qemu_opts_foreach(_object_opts,
user_creatable_add_opts_foreach,
qemu_img_object_print_help, _fatal)) {
-ret = 2;
-goto out4;
+return 2;
}
/* Initialize before goto out */
@@ -1559,7 +1557,6 @@ out2:
blk_unref(blk1);
out3:
qemu_progress_end();
-out4:
return ret;
}
--
2.24.1
[PATCH v1 21/59] block/backup.c remove unneeded 'out' label in backup_run()
'out' can be replaced by 'return ret' and 'return -ECANCELED'.
CC: John Snow
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/backup.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index cf62b1a38c..e73294872c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -254,13 +254,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
for (offset = 0; offset < s->len; ) {
if (yield_and_check(s)) {
-ret = -ECANCELED;
-goto out;
+return -ECANCELED;
}
ret = block_copy_reset_unallocated(s->bcs, offset, );
if (ret < 0) {
-goto out;
+return ret;
}
offset += count;
@@ -284,7 +283,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
ret = backup_loop(s);
}
- out:
return ret;
}
--
2.24.1
[PATCH v1 17/59] block/dmg.c: remove unneeded 'fail' label in dmg_read_mish_block()
Both the 'fail' label and 'ret' variable can be removed from the
function. Instead, call 'return -EINVAL' in the error conditions.
CC: Stefan Hajnoczi
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/dmg.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 4a045f2b3e..ce39a87864 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -217,7 +217,6 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
uint8_t *buffer, uint32_t count)
{
uint32_t type, i;
-int ret;
size_t new_size;
uint32_t chunk_count;
int64_t offset = 0;
@@ -273,8 +272,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
error_report("sector count %" PRIu64 " for chunk %" PRIu32
" is larger than max (%u)",
s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
/* offset in (compressed) data fork */
@@ -288,8 +286,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
error_report("length %" PRIu64 " for chunk %" PRIu32
" is larger than max (%u)",
s->lengths[i], i, DMG_LENGTHS_MAX);
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
update_max_chunk_size(s, i, >max_compressed_size,
@@ -298,9 +295,6 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
}
s->n_chunks += chunk_count;
return 0;
-
-fail:
-return ret;
}
static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
--
2.24.1
[PATCH v1 13/59] block/vdi.c: remove 'fail' label in vdi_open()
The 'fail' label can be replaced by 'return ret' or by
a 'return' with the error code that was being set right
before the 'goto' call.
CC: Stefan Weil
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vdi.c | 40 +---
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 0142da7233..6d486ffed9 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -388,7 +388,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
ret = bdrv_pread(bs->file, 0, , sizeof(header));
if (ret < 0) {
-goto fail;
+return ret;
}
vdi_header_to_cpu();
@@ -400,8 +400,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
", max supported is 0x%" PRIx64 ")",
header.disk_size, VDI_DISK_SIZE_MAX);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
}
uuid_link = header.uuid_link;
@@ -418,58 +417,48 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
if (header.signature != VDI_SIGNATURE) {
error_setg(errp, "Image not in VDI format (bad signature %08" PRIx32
")", header.signature);
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
} else if (header.version != VDI_VERSION_1_1) {
error_setg(errp, "unsupported VDI image (version %" PRIu32 ".%" PRIu32
")", header.version >> 16, header.version & 0x);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.offset_bmap % SECTOR_SIZE != 0) {
/* We only support block maps which start on a sector boundary. */
error_setg(errp, "unsupported VDI image (unaligned block map offset "
"0x%" PRIx32 ")", header.offset_bmap);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.offset_data % SECTOR_SIZE != 0) {
/* We only support data blocks which start on a sector boundary. */
error_setg(errp, "unsupported VDI image (unaligned data offset 0x%"
PRIx32 ")", header.offset_data);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.sector_size != SECTOR_SIZE) {
error_setg(errp, "unsupported VDI image (sector size %" PRIu32
" is not %u)", header.sector_size, SECTOR_SIZE);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
error_setg(errp, "unsupported VDI image (block size %" PRIu32
" is not %" PRIu32 ")",
header.block_size, DEFAULT_CLUSTER_SIZE);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.disk_size >
(uint64_t)header.blocks_in_image * header.block_size) {
error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
"image bitmap has room for %" PRIu64 ")",
header.disk_size,
(uint64_t)header.blocks_in_image * header.block_size);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (!qemu_uuid_is_null(_link)) {
error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (!qemu_uuid_is_null(_parent)) {
error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
} else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
error_setg(errp, "unsupported VDI image "
"(too many blocks %u, max is %u)",
header.blocks_in_image, VDI_BLOCKS_IN_IMAGE_MAX);
-ret = -ENOTSUP;
-goto fail;
+return -ENOTSUP;
}
bs->total_sectors = header.disk_size / SECTOR_SIZE;
@@ -482,8 +471,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
bmap_size = DIV_ROUND_UP(bmap_size, SECTOR_SIZE);
s->bmap = qemu_try_blockalign(bs->file->bs, bmap_size * SECTOR_SIZE);
if (s->bmap == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
}
ret = bdrv_pread(bs->file, header.offset_bmap, s->bmap,
@@ -509,8 +497,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
fail_free_bmap:
qemu_vfree(s->bmap);
-
- fail:
return ret;
}
--
2.24.1
[PATCH v1 25/59] block/vhdx.c: remove unneeded 'exit' labels
'exit' label of vhdx_region_check() and vhdx_update_header()
can be replaced by 'return' statements.
CC: Jeff Cody
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vhdx.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index f02d2611be..7a1ea312ed 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -229,7 +229,6 @@ void vhdx_guid_generate(MSGUID *guid)
/* Check for region overlaps inside the VHDX image */
static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
{
-int ret = 0;
uint64_t end;
VHDXRegionEntry *r;
@@ -239,13 +238,11 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t
start, uint64_t length)
error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
"region %" PRIu64 "-%." PRIu64, start, end, r->start,
r->end);
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
}
}
-exit:
-return ret;
+return 0;
}
/* Register a region for future checks */
@@ -390,11 +387,10 @@ static int vhdx_update_header(BlockDriverState *bs,
BDRVVHDXState *s,
ret = vhdx_write_header(bs->file, inactive_header, header_offset, true);
if (ret < 0) {
-goto exit;
+return ret;
}
s->curr_header = hdr_idx;
-exit:
return ret;
}
--
2.24.1
[PATCH v1 27/59] crypto/block-luks.c: remove unneeded label in qcrypto_block_luks_find_key
'error' can be replaced by 'return -1' directly.
CC: Kevin Wolf
CC: Max Reitz
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
crypto/block-luks.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 4861db810c..9742d4bdd9 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1057,7 +1057,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
opaque,
errp);
if (rv < 0) {
-goto error;
+return -1;
}
if (rv == 1) {
return 0;
@@ -1065,7 +1065,6 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
}
error_setg(errp, "Invalid password, cannot unlock any keyslot");
- error:
return -1;
}
--
2.24.1
[PATCH v1 22/59] block/vmdk.c: remove unneeded labels
'exit' label from vmdk_open_desc_file() can be replaced by
'return' calls with the appropriate value. Same thing with
the 'out' label from vmdk_co_create().
CC: Fam Zheng
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vmdk.c | 37 +
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 20e909d997..d1491486de 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1222,14 +1222,12 @@ out:
static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
QDict *options, Error **errp)
{
-int ret;
char ct[128];
BDRVVmdkState *s = bs->opaque;
if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
error_setg(errp, "invalid VMDK image descriptor");
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
}
if (strcmp(ct, "monolithicFlat") &&
strcmp(ct, "vmfs") &&
@@ -1238,14 +1236,11 @@ static int vmdk_open_desc_file(BlockDriverState *bs,
int flags, char *buf,
strcmp(ct, "twoGbMaxExtentSparse") &&
strcmp(ct, "twoGbMaxExtentFlat")) {
error_setg(errp, "Unsupported image type '%s'", ct);
-ret = -ENOTSUP;
-goto exit;
+return -ENOTSUP;
}
s->create_type = g_strdup(ct);
s->desc_offset = 0;
-ret = vmdk_parse_extents(buf, bs, options, errp);
-exit:
-return ret;
+return vmdk_parse_extents(buf, bs, options, errp);
}
static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
@@ -2738,7 +2733,6 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int
idx,
static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options,
Error **errp)
{
-int ret;
BlockdevCreateOptionsVmdk *opts;
opts = _options->u.vmdk;
@@ -2746,23 +2740,18 @@ static int coroutine_fn
vmdk_co_create(BlockdevCreateOptions *create_options,
/* Validate options */
if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) {
error_setg(errp, "Image size must be a multiple of 512 bytes");
-ret = -EINVAL;
-goto out;
+return -EINVAL;
}
-ret = vmdk_co_do_create(opts->size,
-opts->subformat,
-opts->adapter_type,
-opts->backing_file,
-opts->hwversion,
-false,
-opts->zeroed_grain,
-vmdk_co_create_cb,
-opts, errp);
-return ret;
-
-out:
-return ret;
+return vmdk_co_do_create(opts->size,
+ opts->subformat,
+ opts->adapter_type,
+ opts->backing_file,
+ opts->hwversion,
+ false,
+ opts->zeroed_grain,
+ vmdk_co_create_cb,
+ opts, errp);
}
static void vmdk_close(BlockDriverState *bs)
--
2.24.1
[PATCH v1 16/59] block/gluster.c: remove unneeded 'exit' label
'exit' label in find_allocation() can be replaced by a
'return -ENOTSUP' call.
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/gluster.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 4fa4a77a47..43bf76eed7 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1381,7 +1381,7 @@ static int find_allocation(BlockDriverState *bs, off_t
start,
BDRVGlusterState *s = bs->opaque;
if (!s->supports_seek_data) {
-goto exit;
+return -ENOTSUP;
}
#if defined SEEK_HOLE && defined SEEK_DATA
@@ -1466,7 +1466,6 @@ static int find_allocation(BlockDriverState *bs, off_t
start,
return -EBUSY;
#endif
-exit:
return -ENOTSUP;
}
--
2.24.1
[PATCH v1 24/59] block/vhdx-log.c: remove unneeded labels
Removing some 'exit' labels used in some functions that are
unneeded, changing them for the adequate 'return' value
for the context.
CC: Jeff Cody
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vhdx-log.c | 86 ++--
1 file changed, 32 insertions(+), 54 deletions(-)
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 13a49c2a33..ba89f0adc6 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -64,8 +64,7 @@ static int vhdx_log_peek_hdr(BlockDriverState *bs,
VHDXLogEntries *log,
/* peek is only supported on sector boundaries */
if (log->read % VHDX_LOG_SECTOR_SIZE) {
-ret = -EFAULT;
-goto exit;
+return -EFAULT;
}
read = log->read;
@@ -77,19 +76,17 @@ static int vhdx_log_peek_hdr(BlockDriverState *bs,
VHDXLogEntries *log,
}
if (read == log->write) {
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
}
offset = log->offset + read;
ret = bdrv_pread(bs->file, offset, hdr, sizeof(VHDXLogEntryHeader));
if (ret < 0) {
-goto exit;
+return ret;
}
vhdx_log_entry_hdr_le_import(hdr);
-exit:
return ret;
}
@@ -179,7 +176,7 @@ static int vhdx_log_write_sectors(BlockDriverState *bs,
VHDXLogEntries *log,
ret = vhdx_user_visible_write(bs, s);
if (ret < 0) {
-goto exit;
+return ret;
}
write = log->write;
@@ -196,7 +193,7 @@ static int vhdx_log_write_sectors(BlockDriverState *bs,
VHDXLogEntries *log,
ret = bdrv_pwrite(bs->file, offset, buffer_tmp,
VHDX_LOG_SECTOR_SIZE);
if (ret < 0) {
-goto exit;
+return ret;
}
buffer_tmp += VHDX_LOG_SECTOR_SIZE;
@@ -205,7 +202,6 @@ static int vhdx_log_write_sectors(BlockDriverState *bs,
VHDXLogEntries *log,
num_sectors--;
}
-exit:
return ret;
}
@@ -214,42 +210,37 @@ exit:
static bool vhdx_log_hdr_is_valid(VHDXLogEntries *log, VHDXLogEntryHeader *hdr,
BDRVVHDXState *s)
{
-int valid = false;
-
if (hdr->signature != VHDX_LOG_SIGNATURE) {
-goto exit;
+return false;
}
/* if the individual entry length is larger than the whole log
* buffer, that is obviously invalid */
if (log->length < hdr->entry_length) {
-goto exit;
+return false;
}
/* length of entire entry must be in units of 4KB (log sector size) */
if (hdr->entry_length % (VHDX_LOG_SECTOR_SIZE)) {
-goto exit;
+return false;
}
/* per spec, sequence # must be > 0 */
if (hdr->sequence_number == 0) {
-goto exit;
+return false;
}
/* log entries are only valid if they match the file-wide log guid
* found in the active header */
if (!guid_eq(hdr->log_guid, s->headers[s->curr_header]->log_guid)) {
-goto exit;
+return false;
}
if (hdr->descriptor_count * sizeof(VHDXLogDescriptor) > hdr->entry_length)
{
-goto exit;
+return false;
}
-valid = true;
-
-exit:
-return valid;
+return true;
}
/*
@@ -274,10 +265,10 @@ static bool vhdx_log_desc_is_valid(VHDXLogDescriptor
*desc,
bool ret = false;
if (desc->sequence_number != hdr->sequence_number) {
-goto exit;
+return false;
}
if (desc->file_offset % VHDX_LOG_SECTOR_SIZE) {
-goto exit;
+return false;
}
if (desc->signature == VHDX_LOG_ZERO_SIGNATURE) {
@@ -290,7 +281,6 @@ static bool vhdx_log_desc_is_valid(VHDXLogDescriptor *desc,
ret = true;
}
-exit:
return ret;
}
@@ -347,20 +337,18 @@ static int vhdx_log_read_desc(BlockDriverState *bs,
BDRVVHDXState *s,
ret = vhdx_log_peek_hdr(bs, log, );
if (ret < 0) {
-goto exit;
+return ret;
}
if (vhdx_log_hdr_is_valid(log, , s) == false) {
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
}
desc_sectors = vhdx_compute_desc_sectors(hdr.descriptor_count);
desc_entries = qemu_try_blockalign(bs->file->bs,
desc_sectors * VHDX_LOG_SECTOR_SIZE);
if (desc_entries == NULL) {
-ret = -ENOMEM;
-goto exit;
+return -ENOMEM;
}
ret = vhdx_log_read_sectors(bs, log, _read, desc_entries,
@@ -390,11 +378,10 @@ static int vhdx_log_read_desc(BlockDriverState *bs,
BDRVVHDXState *s,
}
*buffer = desc_entries;
-goto exit;
+return ret;
free_and_exit:
qemu_vfree(desc_entries);
-exit:
return ret;
}
@@ -688,7 +675,7 @@ static int vhdx_log_search(BlockDriverState *bs,
BDRVVHDXState *s,
ret = vhdx_validate_log_entry(bs, s, _log, curr_seq,
[PATCH v1 23/59] block/vxhs.c: remove unneeded 'out' label in vxhs_iio_callback()
'out' is just a 'return' call.
CC: Kevin Wolf
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vxhs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/block/vxhs.c b/block/vxhs.c
index d79fc97df6..e3db367918 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -96,7 +96,7 @@ static void vxhs_iio_callback(void *ctx, uint32_t opcode,
uint32_t error)
acb = ctx;
} else {
trace_vxhs_iio_callback(error);
-goto out;
+return;
}
if (error) {
@@ -122,8 +122,6 @@ static void vxhs_iio_callback(void *ctx, uint32_t opcode,
uint32_t error)
}
break;
}
-out:
-return;
}
static QemuOptsList runtime_opts = {
--
2.24.1
[PATCH v1 20/59] block/vpc.c: remove unneeded 'fail' label in create_dynamic_disk()
'fail' label can be replaced by 'return ret' on error and
'return 0' in the end of the function.
CC: Kevin Wolf
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/vpc.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index a65550298e..2678b48dfd 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -839,13 +839,13 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t
*buf,
ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
if (ret < 0) {
-goto fail;
+return ret;
}
offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
ret = blk_pwrite(blk, offset, buf, HEADER_SIZE, 0);
if (ret < 0) {
-goto fail;
+return ret;
}
/* Write the initial BAT */
@@ -855,7 +855,7 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t
*buf,
for (i = 0; i < DIV_ROUND_UP(num_bat_entries * 4, 512); i++) {
ret = blk_pwrite(blk, offset, buf, 512, 0);
if (ret < 0) {
-goto fail;
+return ret;
}
offset += 512;
}
@@ -882,12 +882,10 @@ static int create_dynamic_disk(BlockBackend *blk, uint8_t
*buf,
ret = blk_pwrite(blk, offset, buf, 1024, 0);
if (ret < 0) {
-goto fail;
+return ret;
}
-ret = 0;
- fail:
-return ret;
+return 0;
}
static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
--
2.24.1
[PATCH v1 19/59] block/ssh.c: remove unneeded labels
The 'out' labels for check_host_key_knownhosts() and authenticate()
functions can be removed and, instead, call 'return' with the
appropriate return value. The 'ret' integer from both functions
could also be removed.
CC: Richard W.M. Jones
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/ssh.c | 61 +
1 file changed, 19 insertions(+), 42 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index b4375cf7d2..e0c56d002a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -276,7 +276,6 @@ static void ssh_parse_filename(const char *filename, QDict
*options,
static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
{
-int ret;
#ifdef HAVE_LIBSSH_0_8
enum ssh_known_hosts_e state;
int r;
@@ -295,7 +294,6 @@ static int check_host_key_knownhosts(BDRVSSHState *s, Error
**errp)
trace_ssh_check_host_key_knownhosts();
break;
case SSH_KNOWN_HOSTS_CHANGED:
-ret = -EINVAL;
r = ssh_get_server_publickey(s->session, );
if (r == 0) {
r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA256,
@@ -320,28 +318,23 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
Error **errp)
"host key does not match the one in known_hosts; this "
"may be a possible attack");
}
-goto out;
+return -EINVAL;
case SSH_KNOWN_HOSTS_OTHER:
-ret = -EINVAL;
error_setg(errp,
"host key for this server not found, another type exists");
-goto out;
+return -EINVAL;
case SSH_KNOWN_HOSTS_UNKNOWN:
-ret = -EINVAL;
error_setg(errp, "no host key was found in known_hosts");
-goto out;
+return -EINVAL;
case SSH_KNOWN_HOSTS_NOT_FOUND:
-ret = -ENOENT;
error_setg(errp, "known_hosts file not found");
-goto out;
+return -ENOENT;
case SSH_KNOWN_HOSTS_ERROR:
-ret = -EINVAL;
error_setg(errp, "error while checking the host");
-goto out;
+return -EINVAL;
default:
-ret = -EINVAL;
error_setg(errp, "error while checking for known server (%d)", state);
-goto out;
+return -EINVAL;
}
#else /* !HAVE_LIBSSH_0_8 */
int state;
@@ -355,40 +348,31 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
Error **errp)
trace_ssh_check_host_key_knownhosts();
break;
case SSH_SERVER_KNOWN_CHANGED:
-ret = -EINVAL;
error_setg(errp,
"host key does not match the one in known_hosts; this "
"may be a possible attack");
-goto out;
+return -EINVAL;
case SSH_SERVER_FOUND_OTHER:
-ret = -EINVAL;
error_setg(errp,
"host key for this server not found, another type exists");
-goto out;
+return -EINVAL;
case SSH_SERVER_FILE_NOT_FOUND:
-ret = -ENOENT;
error_setg(errp, "known_hosts file not found");
-goto out;
+return -ENOENT;
case SSH_SERVER_NOT_KNOWN:
-ret = -EINVAL;
error_setg(errp, "no host key was found in known_hosts");
-goto out;
+return -EINVAL;
case SSH_SERVER_ERROR:
-ret = -EINVAL;
error_setg(errp, "server error");
-goto out;
+return -EINVAL;
default:
-ret = -EINVAL;
error_setg(errp, "error while checking for known server (%d)", state);
-goto out;
+return -EINVAL;
}
#endif /* !HAVE_LIBSSH_0_8 */
/* known_hosts checking successful. */
-ret = 0;
-
- out:
-return ret;
+return 0;
}
static unsigned hex2decimal(char ch)
@@ -501,20 +485,18 @@ static int check_host_key(BDRVSSHState *s,
SshHostKeyCheck *hkc, Error **errp)
static int authenticate(BDRVSSHState *s, Error **errp)
{
-int r, ret;
+int r;
int method;
/* Try to authenticate with the "none" method. */
r = ssh_userauth_none(s->session, NULL);
if (r == SSH_AUTH_ERROR) {
-ret = -EPERM;
session_error_setg(errp, s, "failed to authenticate using none "
"authentication");
-goto out;
+return -EPERM;
} else if (r == SSH_AUTH_SUCCESS) {
/* Authenticated! */
-ret = 0;
-goto out;
+return 0;
}
method = ssh_userauth_list(s->session, NULL);
@@ -527,23 +509,18 @@ static int authenticate(BDRVSSHState *s, Error **errp)
if (method & SSH_AUTH_METHOD_PUBLICKEY) {
r = ssh_userauth_publickey_auto(s->session, NULL, NULL);
if (r == SSH_AUTH_ERROR) {
-ret = -EINVAL;
session_error_set
[PATCH v1 18/59] qcow2-refcount.c: remove unneeded 'fail' label in qcow2_refcount_init()
'fail' label can be replaced by 'return ret' and 'return -ENOMEM'
calls.
CC: Kevin Wolf
CC: Max Reitz
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/qcow2-refcount.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f67ac6b2d8..1fe66677d1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -111,22 +111,19 @@ int qcow2_refcount_init(BlockDriverState *bs)
if (s->refcount_table_size > 0) {
if (s->refcount_table == NULL) {
-ret = -ENOMEM;
-goto fail;
+return -ENOMEM;
}
BLKDBG_EVENT(bs->file, BLKDBG_REFTABLE_LOAD);
ret = bdrv_pread(bs->file, s->refcount_table_offset,
s->refcount_table, refcount_table_size2);
if (ret < 0) {
-goto fail;
+return ret;
}
for(i = 0; i < s->refcount_table_size; i++)
be64_to_cpus(>refcount_table[i]);
update_max_refcount_table_index(s);
}
return 0;
- fail:
-return ret;
}
void qcow2_refcount_close(BlockDriverState *bs)
--
2.24.1
[PATCH v1 14/59] block/file-posix.c: remove unneeded labels
'out' label from cdrom_probe_device() can be removed, and its
only instance replaced by 'return prio'.
In raw_co_create(), 'out' can be replaced by 'return -errno' in
the error condition it is being called.
CC: Kevin Wolf
CC: qemu-block@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
block/file-posix.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 1b805bd938..b1a6549806 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2265,9 +2265,8 @@ raw_co_create(BlockdevCreateOptions *options, Error
**errp)
/* Create file */
fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
if (fd < 0) {
-result = -errno;
-error_setg_errno(errp, -result, "Could not create file");
-goto out;
+error_setg_errno(errp, errno, "Could not create file");
+return -errno;
}
/* Take permissions: We want to discard everything, so we need
@@ -2342,7 +2341,7 @@ out_close:
result = -errno;
error_setg_errno(errp, -result, "Could not close the new file");
}
-out:
+
return result;
}
@@ -3554,7 +3553,7 @@ static int cdrom_probe_device(const char *filename)
fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
if (fd < 0) {
-goto out;
+return prio;
}
ret = fstat(fd, );
if (ret == -1 || !S_ISBLK(st.st_mode)) {
@@ -3568,7 +3567,6 @@ static int cdrom_probe_device(const char *filename)
outc:
qemu_close(fd);
-out:
return prio;
}
--
2.24.1
Re: [Qemu-block] [PATCH v3 1/3] block: introducing 'bdrv_co_delete_file' interface
Hi John,
On 5/13/19 4:47 PM, John Snow wrote:
It looks like this one has gone un-noticed for a little while.
On 3/26/19 5:17 PM, Daniel Henrique Barboza wrote:
Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.
This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c. The
implementation is given by 'raw_co_delete_file'. The helper
'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
at this moment, but it will be used inside LUKS in a later patch.
Foreseeing this future use, let's put it in block.c and make it
public.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block.c | 11 +++
block/file-posix.c| 28
include/block/block.h | 1 +
include/block/block_int.h | 6 ++
4 files changed, 46 insertions(+)
diff --git a/block.c b/block.c
index 0a93ee9ac8..227362b282 100644
--- a/block.c
+++ b/block.c
@@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
#endif
}
+/**
+ * Helper that checks if a given string represents a regular
+ * local file.
+ */
+bool bdrv_path_is_regular_file(const char *path)
+{
+struct stat st;
+
+return (stat(path, ) == 0) && S_ISREG(st.st_mode);
+}
+
/*
* Detect host devices. By convention, /dev/cdrom[N] is always
* recognized as a host CDROM.
diff --git a/block/file-posix.c b/block/file-posix.c
index d102f3b222..09d84bab37 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2342,6 +2342,33 @@ static int coroutine_fn raw_co_create_opts(const char
*filename, QemuOpts *opts,
return raw_co_create(, errp);
}
+/**
+ * Co-routine function that erases a regular file.
+ */
+static int coroutine_fn raw_co_delete_file(const char *filename,
+ Error **errp)
Do we need to mark functions that make no use of coroutines as
coroutine_fn? I guess this way the interface is *allowed* to be a
coroutine if other drivers need to make use of that.
This function is used in a coroutine in patch 2. But to be honest, what I
did here was to emulate the existing behavior of bdrv_create. Which
is kind of lame if bdrv_create happens to have design problems or
inconsistencies, but at least it's based on something that's already
working.
+{
+int ret;
+
+/* Skip file: protocol prefix */
+strstart(filename, "file:", );
+
This sticks out as fragile to me, but I guess that's exactly how create
works, so... OK.
Yep, create does the same thing.
Thanks,
DHB
+if (!bdrv_path_is_regular_file(filename)) {
+ret = -ENOENT;
+error_setg_errno(errp, -ret, "%s is not a regular file", filename);
+goto done;
+}
+
+ret = unlink(filename);
+if (ret < 0) {
+ret = -errno;
+error_setg_errno(errp, -ret, "Error when deleting file %s", filename);
+}
+
+done:
+return ret;
+}
+
/*
* Find allocation range in @bs around offset @start.
* May change underlying file descriptor's file offset.
@@ -2867,6 +2894,7 @@ BlockDriver bdrv_file = {
.bdrv_co_block_status = raw_co_block_status,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+.bdrv_co_delete_file = raw_co_delete_file,
.bdrv_co_preadv = raw_co_preadv,
.bdrv_co_pwritev= raw_co_pwritev,
diff --git a/include/block/block.h b/include/block/block.h
index e452988b66..820643f96d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -363,6 +363,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base,
Error **errp);
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState
*base);
+bool bdrv_path_is_regular_file(const char *path);
typedef struct BdrvCheckResult {
int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01e855a066..74abb78ce7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -309,6 +309,12 @@ struct BlockDriver {
*/
int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+/*
+ * Delete a local created file.
+ */
+int coroutine_fn (*bdrv_co_delete_file)(const char *filename,
+Error **errp);
+
/*
* Flushes all data that was already written to the OS all the way down to
* the disk (for example file-posix.c calls fsync()).
Seems alright at a glance, if we want this interface.
Kevin, do we?
--js
Re: [Qemu-block] [PATCH v3 0/3] delete created files when block_crypto_co_create_opts_luks fails
Ping On 3/26/19 6:17 PM, Daniel Henrique Barboza wrote: In this new interation, the following changes were made from the previous version [1]: - moved the 'generic' code from block.c to file-posix.c - moved the file delete call from qemu_img to the error path of block_crypto_co_create_opts_luks [1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06444.html Daniel Henrique Barboza (3): block: introducing 'bdrv_co_delete_file' interface block.c: adding bdrv_delete_file crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails block.c | 82 +++ block/crypto.c| 31 +++ block/file-posix.c| 28 + include/block/block.h | 3 ++ include/block/block_int.h | 6 +++ 5 files changed, 150 insertions(+)
[Qemu-block] [PATCH v3 1/3] block: introducing 'bdrv_co_delete_file' interface
Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.
This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c. The
implementation is given by 'raw_co_delete_file'. The helper
'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
at this moment, but it will be used inside LUKS in a later patch.
Foreseeing this future use, let's put it in block.c and make it
public.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block.c | 11 +++
block/file-posix.c| 28
include/block/block.h | 1 +
include/block/block_int.h | 6 ++
4 files changed, 46 insertions(+)
diff --git a/block.c b/block.c
index 0a93ee9ac8..227362b282 100644
--- a/block.c
+++ b/block.c
@@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
#endif
}
+/**
+ * Helper that checks if a given string represents a regular
+ * local file.
+ */
+bool bdrv_path_is_regular_file(const char *path)
+{
+struct stat st;
+
+return (stat(path, ) == 0) && S_ISREG(st.st_mode);
+}
+
/*
* Detect host devices. By convention, /dev/cdrom[N] is always
* recognized as a host CDROM.
diff --git a/block/file-posix.c b/block/file-posix.c
index d102f3b222..09d84bab37 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2342,6 +2342,33 @@ static int coroutine_fn raw_co_create_opts(const char
*filename, QemuOpts *opts,
return raw_co_create(, errp);
}
+/**
+ * Co-routine function that erases a regular file.
+ */
+static int coroutine_fn raw_co_delete_file(const char *filename,
+ Error **errp)
+{
+int ret;
+
+/* Skip file: protocol prefix */
+strstart(filename, "file:", );
+
+if (!bdrv_path_is_regular_file(filename)) {
+ret = -ENOENT;
+error_setg_errno(errp, -ret, "%s is not a regular file", filename);
+goto done;
+}
+
+ret = unlink(filename);
+if (ret < 0) {
+ret = -errno;
+error_setg_errno(errp, -ret, "Error when deleting file %s", filename);
+}
+
+done:
+return ret;
+}
+
/*
* Find allocation range in @bs around offset @start.
* May change underlying file descriptor's file offset.
@@ -2867,6 +2894,7 @@ BlockDriver bdrv_file = {
.bdrv_co_block_status = raw_co_block_status,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
.bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+.bdrv_co_delete_file = raw_co_delete_file,
.bdrv_co_preadv = raw_co_preadv,
.bdrv_co_pwritev= raw_co_pwritev,
diff --git a/include/block/block.h b/include/block/block.h
index e452988b66..820643f96d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -363,6 +363,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base,
Error **errp);
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+bool bdrv_path_is_regular_file(const char *path);
typedef struct BdrvCheckResult {
int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01e855a066..74abb78ce7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -309,6 +309,12 @@ struct BlockDriver {
*/
int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+/*
+ * Delete a local created file.
+ */
+int coroutine_fn (*bdrv_co_delete_file)(const char *filename,
+Error **errp);
+
/*
* Flushes all data that was already written to the OS all the way down to
* the disk (for example file-posix.c calls fsync()).
--
2.20.1
[Qemu-block] [PATCH v3 2/3] block.c: adding bdrv_delete_file
Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
can be used in a way similar of the existing bdrv_create_file to
to clean up a created file.
The logic is also similar to what is already done in bdrv_create_file:
a qemu_coroutine is created if needed, a specialized function
bdrv_delete_co_entry is used to call the bdrv_co_delete_file
co-routine of the driver, if the driver implements it.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block.c | 71 +++
include/block/block.h | 2 ++
2 files changed, 73 insertions(+)
diff --git a/block.c b/block.c
index 227362b282..4eb9d27863 100644
--- a/block.c
+++ b/block.c
@@ -547,6 +547,77 @@ int bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp)
return ret;
}
+static void coroutine_fn bdrv_delete_co_entry(void *opaque)
+{
+Error *local_err = NULL;
+int ret;
+
+CreateCo *cco = opaque;
+assert(cco->drv);
+
+ret = cco->drv->bdrv_co_delete_file(cco->filename, _err);
+error_propagate(>err, local_err);
+cco->ret = ret;
+}
+
+int bdrv_delete_file(const char *filename, Error **errp)
+{
+
+BlockDriver *drv = bdrv_find_protocol(filename, true, errp);
+CreateCo cco = {
+.drv = drv,
+.filename = g_strdup(filename),
+.ret = NOT_DONE,
+.err = NULL,
+};
+Coroutine *co;
+int ret;
+
+if (!drv) {
+error_setg(errp, "File '%s' has unknown format", filename);
+ret = -ENOENT;
+goto out;
+}
+
+if (!drv->bdrv_co_delete_file) {
+error_setg(errp, "Driver '%s' does not support image delete",
+ drv->format_name);
+ret = -ENOTSUP;
+goto out;
+}
+
+if (!drv->bdrv_co_delete_file) {
+error_setg(errp, "Driver '%s' does not support image delete",
+ drv->format_name);
+ret = -ENOTSUP;
+goto out;
+}
+
+if (qemu_in_coroutine()) {
+/* Fast-path if already in coroutine context */
+bdrv_delete_co_entry();
+} else {
+co = qemu_coroutine_create(bdrv_delete_co_entry, );
+qemu_coroutine_enter(co);
+while (cco.ret == NOT_DONE) {
+aio_poll(qemu_get_aio_context(), true);
+}
+}
+
+ret = cco.ret;
+if (ret < 0) {
+if (cco.err) {
+error_propagate(errp, cco.err);
+} else {
+error_setg_errno(errp, -ret, "Could not delete image");
+}
+}
+
+out:
+g_free(cco.filename);
+return ret;
+}
+
/**
* Try to get @bs's logical and physical block size.
* On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index 820643f96d..76901a63de 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -364,6 +364,8 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base,
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
bool bdrv_path_is_regular_file(const char *path);
+int bdrv_delete_file(const char *filename, Error **errp);
+
typedef struct BdrvCheckResult {
int corruptions;
--
2.20.1
[Qemu-block] [PATCH v3 0/3] delete created files when block_crypto_co_create_opts_luks fails
In this new interation, the following changes were made from the previous version [1]: - moved the 'generic' code from block.c to file-posix.c - moved the file delete call from qemu_img to the error path of block_crypto_co_create_opts_luks [1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06444.html Daniel Henrique Barboza (3): block: introducing 'bdrv_co_delete_file' interface block.c: adding bdrv_delete_file crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails block.c | 82 +++ block/crypto.c| 31 +++ block/file-posix.c| 28 + include/block/block.h | 3 ++ include/block/block_int.h | 6 +++ 5 files changed, 150 insertions(+) -- 2.20.1
[Qemu-block] [PATCH v3 3/3] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails
When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:
$ qemu-img create -f luks --object
secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o
key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760
key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not
valid UTF-8
However, the created file /var/tmp/pool_target/vol_1 is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.
The volume file is created inside block_crypto_co_create_opts_luks, in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.
This patch changes block_crypto_co_create_opts_luks to check if @filename
is an existing file before bdrv_create_file is called. In case of failure,
if @filename didn't exist before, check again for its existence and,
if affirmative, erase it by calling bdrv_delete_file.
* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.
Reported-by: Srikanth Aithal
Suggested-by: Kevin Wolf
Signed-off-by: Daniel Henrique Barboza
---
block/crypto.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index 3af46b805f..7c1b80616c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -29,6 +29,7 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
#include "qemu/option.h"
+#include "qemu/cutils.h"
#include "crypto.h"
typedef struct BlockCrypto BlockCrypto;
@@ -533,6 +534,8 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
BlockDriverState *bs = NULL;
QDict *cryptoopts;
int64_t size;
+const char *path;
+bool file_already_existed = false;
int ret;
/* Parse options */
@@ -549,6 +552,15 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
goto fail;
}
+/*
+ * Check if 'filename' represents a local file that already
+ * exists in the file system prior to bdrv_create_file. Strip
+ * the leading 'file:' from the filename if it exists.
+ */
+path = filename;
+strstart(path, "file:", );
+file_already_existed = bdrv_path_is_regular_file(path);
+
/* Create protocol layer */
ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
@@ -573,6 +585,25 @@ fail:
bdrv_unref(bs);
qapi_free_QCryptoBlockCreateOptions(create_opts);
qobject_unref(cryptoopts);
+
+/*
+ * If an error occurred and we ended up creating a bogus
+ * 'filename' file, delete it
+ */
+if (ret && !file_already_existed && bdrv_path_is_regular_file(path)) {
+Error *local_err;
+int r_del = bdrv_delete_file(path, _err);
+/*
+ * ENOTSUP will happen if the block driver doesn't support
+ * 'bdrv_co_delete_file'. ENOENT will happen if the file
+ * doesn't exist. Both are predictable and shouldn't be
+ * reported back to the user.
+ */
+if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
+error_reportf_err(local_err, "%s: ", path);
+}
+}
+
return ret;
}
--
2.20.1
Re: [Qemu-block] [PATCH v2 1/3] block: introducing 'bdrv_co_delete_file' interface
On 3/25/19 9:10 AM, Kevin Wolf wrote: Am 22.03.2019 um 18:52 hat Daniel Henrique Barboza geschrieben: Adding to Block Drivers the capability of being able to clean up its created files can be useful in certain situations. For the LUKS driver, for instance, a failure in one of its authentication steps can leave files in the host that weren't there before. This patch adds the 'bdrv_co_delete_file' interface to block drivers and add it to the LUKS driver. The implementation is provided in a new 'bdrv_co_delete_file_generic' function inside block.c. This function is made public in case other block drivers wants to support this cleanup interface as well. Suggested-by: Daniel P. Berrangé Signed-off-by: Daniel Henrique Barboza This is still wrong. Consider a LUKS image that is accessed via http:// rather than in a local file. Instead of the "generic" implementation in block.c (which isn't actually generic, but very specific to local files), this needs to be the implementation for the file driver in block/file-posix.c. I'll move the code there in the next version. The call of bdrv_co_delete_file() must then be in the error path of the same function that called bdrv_create_file(), such as block_crypto_co_create_opts_luks() in your specific example. That makes sense for the problem that I'm aiming to fix. However, since we're moving the code to file-posix.c, keeping the bdrv_co_delete_file() in img_create will fix the issue for all other formats that ends up using the file driver as well. ATM I have no evidence that this error might happen with anything but LUKS, but if/when it does, the code can handle it. Now, making a counter-argument for what I just wrote, there are more callers of bdrv_create besides img_create: - bdrv_append_temp_snapshot - bdrv_create_file - bdrv_img_create - enable_write_target (vvfat) - img_convert - img_dd Moving the delete code from img_create to block_crypto_co_create_opts_luks(), as you suggested, will fix the case we know it can be problematic for these callers as well. The drawback is that we will need to check inside LUKS if the local file exists beforehand though. Thanks, dhb Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] qemu-img: removing created when img_create fails
On 3/22/19 4:02 PM, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20190322175241.5954-1-danielhb...@gmail.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === CC aarch64-softmmu/tcg/tcg-op.o CC aarch64-softmmu/tcg/tcg-op-vec.o /tmp/qemu-test/src/qemu-img.c: In function 'img_create': /tmp/qemu-test/src/qemu-img.c:568:13: error: 'filename' may be used uninitialized in this function [-Werror=maybe-uninitialized] error_reportf_err(local_err, "%s: ", filename); ^~ cc1: all warnings being treated as errors Not sure why it complained about this. Travis didn't care How can I reproduce this docker use test? I have a Fedora29 box with Docker and issuing "make docker-test-mingw(...)" as mentioned above doesn't work. Thanks, dhb The full log is available at http://patchew.org/logs/20190322175241.5954-1-danielhb...@gmail.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[Qemu-block] [PATCH v2 0/3] qemu-img: removing created when img_create fails
This is a patch series that follows up the patch [1] after the review from Daniel P. Berrange. The new interface is being implemented only by the LUKS driver because this is the error condition I'm trying to fix. My first idea when coding it was to implement this interface in all drivers that deals with local files, but I thought it would be overkill - at least for this second spin. [1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg05899.html Daniel Henrique Barboza (3): block: introducing 'bdrv_co_delete_file' interface block.c: adding bdrv_delete_file qemu-img.c: clean up created file on img_create failure block.c | 117 ++ block/crypto.c| 2 + include/block/block.h | 6 ++ include/block/block_int.h | 6 ++ qemu-img.c| 29 +- 5 files changed, 159 insertions(+), 1 deletion(-) -- 2.20.1
[Qemu-block] [PATCH v2 1/3] block: introducing 'bdrv_co_delete_file' interface
Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.
This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the LUKS driver. The implementation is provided
in a new 'bdrv_co_delete_file_generic' function inside block.c. This
function is made public in case other block drivers wants to
support this cleanup interface as well.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
block.c | 45 +++
block/crypto.c| 2 ++
include/block/block.h | 3 +++
include/block/block_int.h | 6 ++
4 files changed, 56 insertions(+)
diff --git a/block.c b/block.c
index 0a93ee9ac8..2b632baba2 100644
--- a/block.c
+++ b/block.c
@@ -547,6 +547,51 @@ int bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp)
return ret;
}
+/**
+ * Helper that checks if a given path represents a regular
+ * local file.
+ */
+bool bdrv_path_is_regular_file(const char *path)
+{
+struct stat st;
+
+return (stat(path, ) == 0) && S_ISREG(st.st_mode);
+}
+
+/**
+ * Co-routine function that erases a regular file. Its original
+ * intent is as a implementation of bdrv_co_delete_file for
+ * the "luks" driver that can leave created files behind in the
+ * file system when the authentication fails.
+ *
+ * The function is exposed here, and with 'generic' in its name,
+ * because file removal isn't usually format specific and any other
+ * BlockDriver might want to re-use this function.
+ */
+int coroutine_fn bdrv_co_delete_file_generic(const char *filename,
+ Error **errp)
+{
+int ret;
+
+/* Skip file: protocol prefix */
+strstart(filename, "file:", );
+
+if (!bdrv_path_is_regular_file(filename)) {
+ret = -ENOENT;
+error_setg_errno(errp, -ret, "%s is not a regular file", filename);
+goto done;
+}
+
+ret = unlink(filename);
+if (ret < 0) {
+ret = -errno;
+error_setg_errno(errp, -ret, "Error when deleting file %s", filename);
+}
+
+done:
+return ret;
+}
+
/**
* Try to get @bs's logical and physical block size.
* On success, store them in @bsz struct and return 0.
diff --git a/block/crypto.c b/block/crypto.c
index 3af46b805f..c604c96c93 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -639,6 +639,8 @@ static BlockDriver bdrv_crypto_luks = {
.bdrv_co_truncate = block_crypto_co_truncate,
.create_opts= _crypto_create_opts_luks,
+.bdrv_co_delete_file = bdrv_co_delete_file_generic,
+
.bdrv_reopen_prepare = block_crypto_reopen_prepare,
.bdrv_refresh_limits = block_crypto_refresh_limits,
.bdrv_co_preadv = block_crypto_co_preadv,
diff --git a/include/block/block.h b/include/block/block.h
index e452988b66..efb77daf9f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -363,6 +363,9 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base,
Error **errp);
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+bool bdrv_path_is_regular_file(const char *path);
+int coroutine_fn bdrv_co_delete_file_generic(const char *filename,
+ Error **errp);
typedef struct BdrvCheckResult {
int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01e855a066..74abb78ce7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -309,6 +309,12 @@ struct BlockDriver {
*/
int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+/*
+ * Delete a local created file.
+ */
+int coroutine_fn (*bdrv_co_delete_file)(const char *filename,
+Error **errp);
+
/*
* Flushes all data that was already written to the OS all the way down to
* the disk (for example file-posix.c calls fsync()).
--
2.20.1
[Qemu-block] [PATCH v2 2/3] block.c: adding bdrv_delete_file
Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
can be used in a way similar of the existing bdrv_create_file to
invoke a driver, given by a format @fmt, to clean up a created
file.
The logic is also similar to what is already done in bdrv_create_file:
a qemu_coroutine is created if needed, a specialized function
bdrv_delete_co_entry is used to call the bdrv_co_delete_file
co-routine of the driver, if the driver implements it.
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
@Daniel: I put the 'Suggested-by' tag here aware that what is being
done in this patch wasn't explicitly suggested by you in that review.
However, since it's a consequence of your suggestion, here it is.
If you mind the tag here, let me know and we can remove it.
block.c | 72 +++
include/block/block.h | 3 ++
2 files changed, 75 insertions(+)
diff --git a/block.c b/block.c
index 2b632baba2..5c7781e471 100644
--- a/block.c
+++ b/block.c
@@ -592,6 +592,78 @@ done:
return ret;
}
+static void coroutine_fn bdrv_delete_co_entry(void *opaque)
+{
+Error *local_err = NULL;
+int ret;
+
+CreateCo *cco = opaque;
+assert(cco->drv);
+
+ret = cco->drv->bdrv_co_delete_file(cco->filename, _err);
+error_propagate(>err, local_err);
+cco->ret = ret;
+}
+
+int bdrv_delete_file(const char *filename, const char *fmt,
+ Error **errp)
+{
+
+BlockDriver *drv = bdrv_find_format(fmt);
+Coroutine *co;
+CreateCo cco = {
+.drv = drv,
+.filename = g_strdup(filename),
+.ret = NOT_DONE,
+.err = NULL,
+};
+int ret;
+
+if (!drv) {
+error_setg(errp, "Unknown file format '%s'", fmt);
+ret = -ENOENT;
+goto out;
+}
+
+if (!drv->bdrv_co_delete_file) {
+error_setg(errp, "Driver '%s' does not support image delete",
+ drv->format_name);
+ret = -ENOTSUP;
+goto out;
+}
+
+if (!drv->bdrv_co_delete_file) {
+error_setg(errp, "Driver '%s' does not support image delete",
+ drv->format_name);
+ret = -ENOTSUP;
+goto out;
+}
+
+if (qemu_in_coroutine()) {
+/* Fast-path if already in coroutine context */
+bdrv_delete_co_entry();
+} else {
+co = qemu_coroutine_create(bdrv_delete_co_entry, );
+qemu_coroutine_enter(co);
+while (cco.ret == NOT_DONE) {
+aio_poll(qemu_get_aio_context(), true);
+}
+}
+
+ret = cco.ret;
+if (ret < 0) {
+if (cco.err) {
+error_propagate(errp, cco.err);
+} else {
+error_setg_errno(errp, -ret, "Could not delete image");
+}
+}
+
+out:
+g_free(cco.filename);
+return ret;
+}
+
/**
* Try to get @bs's logical and physical block size.
* On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index efb77daf9f..9b66cf00cb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -366,6 +366,9 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base);
bool bdrv_path_is_regular_file(const char *path);
int coroutine_fn bdrv_co_delete_file_generic(const char *filename,
Error **errp);
+int bdrv_delete_file(const char *filename, const char *fmt,
+ Error **errp);
+
typedef struct BdrvCheckResult {
int corruptions;
--
2.20.1
[Qemu-block] [PATCH v2 3/3] qemu-img.c: clean up created file on img_create failure
When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:
$ qemu-img create -f luks --object
secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o
key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760
key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not
valid UTF-8
However, the created file /var/tmp/pool_target/vol_1 is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.
The volume file is created inside block_crypto_co_create_opts_luks, in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.
This patch changes img_create to check if @filename is an existing file
before bdrv_img_create is called. In case of failure, if @filename didn't
exist before, check again for its existence and, if affirmative, erase it
by calling bdrv_delete_file.
* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.
Reported-by: Srikanth Aithal
Suggested-by: Daniel P. Berrangé
Signed-off-by: Daniel Henrique Barboza
---
qemu-img.c | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index 5fac840742..03b139b4ac 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -422,11 +422,12 @@ static int img_create(int argc, char **argv)
uint64_t img_size = -1;
const char *fmt = "raw";
const char *base_fmt = NULL;
-const char *filename;
+const char *filename, *path;
const char *base_filename = NULL;
char *options = NULL;
Error *local_err = NULL;
bool quiet = false;
+bool file_already_existed = false;
int flags = 0;
for(;;) {
@@ -529,6 +530,15 @@ static int img_create(int argc, char **argv)
error_exit("Unexpected argument: %s", argv[optind]);
}
+/*
+ * Check if 'filename' represents a local file that already
+ * exists in the file system prior to bdrv_img_create. Strip
+ * the leading 'file:' from the filename if it exists.
+ */
+path = filename;
+strstart(path, "file:", );
+file_already_existed = bdrv_path_is_regular_file(path);
+
bdrv_img_create(filename, fmt, base_filename, base_fmt,
options, img_size, flags, quiet, _err);
if (local_err) {
@@ -541,6 +551,23 @@ static int img_create(int argc, char **argv)
fail:
g_free(options);
+/*
+ * If an error occurred and we ended up creating a bogus
+ * 'filename' file, delete it
+ */
+if (!file_already_existed && bdrv_path_is_regular_file(path)) {
+
+int ret = bdrv_delete_file(path, fmt, _err);
+/*
+ * ENOTSUP will happen if the block driver doesn't support
+ * 'bdrv_co_delete_file'. ENOENT will happen if the file
+ * doesn't exist. Both are predictable and shouldn't be
+ * reported back to the user.
+ */
+if ((ret < 0) && (ret != -ENOTSUP) && (ret != -ENOENT)) {
+error_reportf_err(local_err, "%s: ", filename);
+}
+}
return 1;
}
--
2.20.1
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] block_crypto_co_create_generic: clean up created file on failure
On 3/20/19 6:52 AM, Daniel P. Berrangé wrote:
On Tue, Mar 19, 2019 at 07:03:30PM -0300, Daniel Henrique Barboza wrote:
When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:
$ qemu-img create -f luks --object
secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o
key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760
key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not
valid UTF-8
However, the created file /var/tmp/pool_target/vol_1 is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.
The volume file is created inside block_crypto_co_create_opts_luks, in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.
This patch fixes it by adding a flag to indicate whether bdrv_create_file()
created the file, and then unlinking it in the 'fail' label if
necessary.
* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.
Reported-by: Srikanth Aithal
Signed-off-by: Daniel Henrique Barboza
---
block/crypto.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index fd8c7cfac6..5b43424845 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -533,6 +533,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
BlockDriverState *bs = NULL;
QDict *cryptoopts;
int64_t size;
+bool filecreated = false;
int ret;
/* Parse options */
@@ -554,6 +555,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
if (ret < 0) {
goto fail;
}
+filecreated = true;
bs = bdrv_open(filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
@@ -569,7 +571,14 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
}
ret = 0;
+goto cleanup;
+
fail:
+if (filecreated) {
+unlink(filename);
You can't do this I'm afraid.
"filename" cannot be assumed to be a local file - it can be an arbitrary
block device backend string.
Contrary to what you might expect, given the call to bdrv_create_file,
we can't actually assume this method created the file either. The above
bdrv_create_file method may have merely formatted a pre-existing volume.
To fix this requires a "bdrv_delete" callback in the BlockDriver code,
and the actual deletion would need to be done in the "qemu-img create"
code.
Thanks for the pointers. I'll see if I can make this happen in v2.
DHB
Regards,
Daniel
[Qemu-block] [PATCH 1/1] block_crypto_co_create_generic: clean up created file on failure
When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:
$ qemu-img create -f luks --object
secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o
key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760
key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not
valid UTF-8
However, the created file /var/tmp/pool_target/vol_1 is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.
The volume file is created inside block_crypto_co_create_opts_luks, in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.
This patch fixes it by adding a flag to indicate whether bdrv_create_file()
created the file, and then unlinking it in the 'fail' label if
necessary.
* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.
Reported-by: Srikanth Aithal
Signed-off-by: Daniel Henrique Barboza
---
block/crypto.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index fd8c7cfac6..5b43424845 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -533,6 +533,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
BlockDriverState *bs = NULL;
QDict *cryptoopts;
int64_t size;
+bool filecreated = false;
int ret;
/* Parse options */
@@ -554,6 +555,7 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
if (ret < 0) {
goto fail;
}
+filecreated = true;
bs = bdrv_open(filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
@@ -569,7 +571,14 @@ static int coroutine_fn
block_crypto_co_create_opts_luks(const char *filename,
}
ret = 0;
+goto cleanup;
+
fail:
+if (filecreated) {
+unlink(filename);
+}
+
+cleanup:
bdrv_unref(bs);
qapi_free_QCryptoBlockCreateOptions(create_opts);
qobject_unref(cryptoopts);
--
2.20.1
Re: [Qemu-block] Problem with data miscompare using scsi-hd, cache=none and io=threads
On 05/24/2018 11:04 AM, Stefan Hajnoczi wrote: On Tue, May 15, 2018 at 06:25:46PM -0300, Daniel Henrique Barboza wrote: This means that the test executed a write at LBA 0x94fa and, after confirming that the write was completed, issue 2 reads in the same LBA to assert the written contents and found out a mismatch. Have you confirmed this pattern at various levels in the stack: 1. Application inside the guest (strace) 2. Guest kernel block layer (blktrace) 3. QEMU (strace) 4. Host kernel block layer (blktrace) Tested (3). In this case the bug stop reproducing. Not sure if there is anything related with strace adding a delay back and forth the preadv/pwritev calls, but the act of attaching strace to the QEMU process changed the behavior. Haven't tried the other 3 scenarios. (2) and (4) are definitely worth trying it out, specially (4). The key thing is that the write completes before the 2 reads are submitted. Have you tried running the test on bare metal? Yes. The stress test does not reproduce the miscompare issue when running on bare metal. Thanks, Daniel Stefan
Re: [Qemu-block] [Qemu-devel] Problem with data miscompare using scsi-hd, cache=none and io=threads
On 05/16/2018 06:35 PM, Daniel Henrique Barboza wrote: On 05/16/2018 04:47 AM, Paolo Bonzini wrote: On 15/05/2018 23:25, Daniel Henrique Barboza wrote: This is the current status of this investigation. I decided to start a discussion here, see if someone can point me something that I overlooked or got it wrong, before I started changing the POSIX thread pool behavior to see if I can enforce one specific POSIX thread to do a read() if we had a write() done in the same fd. Any suggestions? Copying from the bug: Unless we learn something new, my understanding is that we're dealing with a host side limitation/bug when calling pwritev() in a different thread than a following preadv(), using the same file descriptor opened with O_DIRECT and no WCE in the host side, the kernel can't grant data coherency, e.g: - thread A executes a pwritev() writing dataA in the disk - thread B executes a preadv() call to read the data, but this preadv() call isn't aware of the previous pwritev() call done in thread A, thus the guarantee of the preadv() call reading dataA isn't assured (as opposed to what is described in man 3 write) - the physical disk, due to the heavy load of the stress test, didn't finish writing up dataA. Since the disk itself doesn't have any internal cache to rely on, the preadv() call goes in and read an old data that differs from dataA. There is a problem in the reasoning of the third point: if the physical disk hasn't yet finished writing up dataA, pwritev() shouldn't have returned. This could be a bug in the kernel, or even in the disk. I suspect the kernel because SCSI passthrough doesn't show the bug; SCSI passthrough uses ioctl() which completes exactly when the disk tells QEMU that the command is done---it cannot report completion too early. (Another small problem in the third point is that the disk actually does have a cache. But the cache should be transparent, if it weren't the bug would be in the disk firmware). It has to be debugged and fixed in the kernel. The thread pool is just... a thread pool, and shouldn't be working around bugs, especially as serious as these. Fixing in the thread pool would only make sense if we were sure that the kernel was working as intended. I think the next step would be to look it in the kernel level and see what is not working there. A more likely possibility: maybe the disk has 4K sectors and QEMU is doing read-modify-write cycles to emulate 512 byte sectors? In this case, mismatches are not expected, since QEMU serializes RMW cycles, but at least we would know that the bug would be in QEMU, and where. Haven't considered this possibility. I'll look it up if the disk has 4k sectors and whether QEMU is emulating 512 bytes sectors. There are several differences between the guest and the host device regarding the kernel parameters. This is how the guest configured the SATA disk: # grep . /sys/block/sdb/queue/* /sys/block/sdb/queue/add_random:1 /sys/block/sdb/queue/chunk_sectors:0 /sys/block/sdb/queue/dax:0 /sys/block/sdb/queue/discard_granularity:4096 /sys/block/sdb/queue/discard_max_bytes:1073741824 /sys/block/sdb/queue/discard_max_hw_bytes:1073741824 /sys/block/sdb/queue/discard_zeroes_data:0 /sys/block/sdb/queue/hw_sector_size:512 /sys/block/sdb/queue/io_poll:0 /sys/block/sdb/queue/io_poll_delay:0 grep: /sys/block/sdb/queue/iosched: Is a directory /sys/block/sdb/queue/iostats:1 /sys/block/sdb/queue/logical_block_size:512 /sys/block/sdb/queue/max_discard_segments:1 /sys/block/sdb/queue/max_hw_sectors_kb:32767 /sys/block/sdb/queue/max_integrity_segments:0 /sys/block/sdb/queue/max_sectors_kb:256 /sys/block/sdb/queue/max_segments:126 /sys/block/sdb/queue/max_segment_size:65536 /sys/block/sdb/queue/minimum_io_size:262144 /sys/block/sdb/queue/nomerges:0 /sys/block/sdb/queue/nr_requests:128 /sys/block/sdb/queue/optimal_io_size:262144 /sys/block/sdb/queue/physical_block_size:512 /sys/block/sdb/queue/read_ahead_kb:4096 /sys/block/sdb/queue/rotational:1 /sys/block/sdb/queue/rq_affinity:1 /sys/block/sdb/queue/scheduler:noop [deadline] cfq /sys/block/sdb/queue/unpriv_sgio:0 grep: /sys/block/sdb/queue/wbt_lat_usec: Invalid argument /sys/block/sdb/queue/write_cache:write back /sys/block/sdb/queue/write_same_max_bytes:262144 /sys/block/sdb/queue/write_zeroes_max_bytes:262144 /sys/block/sdb/queue/zoned:none The same device in the host: $ grep . /sys/block/sdc/queue/* /sys/block/sdc/queue/add_random:1 /sys/block/sdc/queue/chunk_sectors:0 /sys/block/sdc/queue/dax:0 /sys/block/sdc/queue/discard_granularity:0 /sys/block/sdc/queue/discard_max_bytes:0 /sys/block/sdc/queue/discard_max_hw_bytes:0 /sys/block/sdc/queue/discard_zeroes_data:0 /sys/block/sdc/queue/hw_sector_size:512 /sys/block/sdc/queue/io_poll:0 /sys/block/sdc/queue/io_poll_delay:0 grep: /sys/block/sdc/queue/iosched: Is a directory /sys/block/sdc/queue/iostats:1 /sys/block/sdc/queue/logical_block_size:512 /sys/block/sdc/queue/max_discard_segments:1 /sys/block/sdc/queue
Re: [Qemu-block] [Qemu-devel] Problem with data miscompare using scsi-hd, cache=none and io=threads
On 05/16/2018 06:47 AM, Dr. David Alan Gilbert wrote: * Daniel Henrique Barboza (danie...@linux.ibm.com) wrote: Hi, I've been working in the last two months in a miscompare issue that happens when using a raid device and a SATA as scsi-hd (emulated SCSI) with cache=none and io=threads during a hardware stress test. I'll summarize it here as best as I can without creating a great wall of text - Red Hat folks can check [1] for all the details. Using the following setup: - Host is a POWER9 RHEL 7.5-alt: kernel 4.14.0-49.1.1.el7a.ppc64le, qemu-kvm-ma 2.10.0-20.el7 (also reproducible with upstream QEMU) - Guest is RHEL 7.5-alt using the same kernel as the host, using two storage disks (a 1.8 Tb raid and a 446Gb SATA drive) as follows: Both block devices have WCE off in the host. With this env, we found problems when running a stress test called HTX [2]. At a given time (usually after 24+ hours of test) HTX finds a data miscompare in one of the devices. This is an example: --- Device name: /dev/sdb Total blocks: 0x74706daf, Block size: 0x200 Rule file name: /usr/lpp/htx/rules/reg/hxestorage/default.hdd Number of Rulefile passes (cycle) completed: 0 Stanza running: rule_6, Thread no.: 8 Oper performed: wrc, Current seek type: SEQ LBA no. where IO started: 0x94fa Transfer size: 0x8400 Miscompare Summary: === LBA no. where miscomapre started: 0x94fa LBA no. where miscomapre ended: 0x94ff Miscompare start offset (in bytes): 0x8 Miscomapre end offset (in bytes): 0xbff Miscompare size (in bytes): 0xbf8 Expected data (at miscomapre offset): 8c9aea5a736462007275 Actual data (at miscomapre offset): 889aea5a736462007275 Are all the miscompares single bit errors like that one? The miscompares differs in size. What it is displayed here is the first snippet of the miscompare data, but in this case the miscompare has 0xbf8 bytes of size. I've seen cases where the miscompare has the same size of the data written - the test initialize the disk with a known pattern (bbb for example), then a miscompare happens and it founds out that the disk had the starting pattern. Is the test doing single bit manipulation or is that coming out of the blue? As far as I've read in the test suite code, it is writing several sectors at once then asserting that the contents were written. Dave - This means that the test executed a write at LBA 0x94fa and, after confirming that the write was completed, issue 2 reads in the same LBA to assert the written contents and found out a mismatch. I've tested all sort of configurations between disk vs LUN, cache modes and AIO. My findings are: - using device='lun' instead of device='disk', I can't reproduce the issue doesn't matter what other configurations are; - using device='disk' but with cache='writethrough', issue doesn't happen (haven't checked other cache modes); - using device='disk', cache='none' and io='native', issue doesn't happen. The issue seems to be tied with the combination device=disk + cache=none + io=threads. I've started digging into the SCSI layer all the way down to the block backend. With a shameful amount of logs I've discovered that, in the write that the test finds a miscompare, in block/file-posix.c: - when doing the write, handle_aiocb_rw_vector() returns success, pwritev() reports that all bytes were written - in both reads after the write, handle_aiocb_rw_vector returns success, all bytes read by preadv(). In both reads, the data read is different from the data written by the pwritev() that happened before In the discussions at [1], Fam Zheng suggested a test in which we would take down the number of threads created in the POSIX thread pool from 64 to 1. The idea is to ensure that we're using the same thread to write and read. There was a suspicion that the kernel can't guarantee data coherency between different threads, even if using the same fd, when using pwritev() and preadv(). This would explain why the following reads in the same fd would fail to retrieve the same data that was written before. After doing this modification, the miscompare didn't reproduce. After reverting the thread pool number change, I've made a couple of attempts trying to flush before read() and flushing after write(). Both attempts failed - the miscompare appears in both scenarios. This enforces the suspicion we have above - if data coherency can't be granted between different threads, flushing in different threads wouldn't make a difference too. I've also tested a suggestion from Fam where I started the disks with "cache.direct=on,cache.no-flush=off" - bug still reproduces. This is the current status of this investigation. I decided to start a discussion here, see if someone can point me something that I overlooked or got it wrong, before I started changing the POSIX thread pool behavior to see if I can e
Re: [Qemu-block] [Qemu-devel] Problem with data miscompare using scsi-hd, cache=none and io=threads
On 05/16/2018 04:47 AM, Paolo Bonzini wrote: On 15/05/2018 23:25, Daniel Henrique Barboza wrote: This is the current status of this investigation. I decided to start a discussion here, see if someone can point me something that I overlooked or got it wrong, before I started changing the POSIX thread pool behavior to see if I can enforce one specific POSIX thread to do a read() if we had a write() done in the same fd. Any suggestions? Copying from the bug: Unless we learn something new, my understanding is that we're dealing with a host side limitation/bug when calling pwritev() in a different thread than a following preadv(), using the same file descriptor opened with O_DIRECT and no WCE in the host side, the kernel can't grant data coherency, e.g: - thread A executes a pwritev() writing dataA in the disk - thread B executes a preadv() call to read the data, but this preadv() call isn't aware of the previous pwritev() call done in thread A, thus the guarantee of the preadv() call reading dataA isn't assured (as opposed to what is described in man 3 write) - the physical disk, due to the heavy load of the stress test, didn't finish writing up dataA. Since the disk itself doesn't have any internal cache to rely on, the preadv() call goes in and read an old data that differs from dataA. There is a problem in the reasoning of the third point: if the physical disk hasn't yet finished writing up dataA, pwritev() shouldn't have returned. This could be a bug in the kernel, or even in the disk. I suspect the kernel because SCSI passthrough doesn't show the bug; SCSI passthrough uses ioctl() which completes exactly when the disk tells QEMU that the command is done---it cannot report completion too early. (Another small problem in the third point is that the disk actually does have a cache. But the cache should be transparent, if it weren't the bug would be in the disk firmware). It has to be debugged and fixed in the kernel. The thread pool is just... a thread pool, and shouldn't be working around bugs, especially as serious as these. Fixing in the thread pool would only make sense if we were sure that the kernel was working as intended. I think the next step would be to look it in the kernel level and see what is not working there. A more likely possibility: maybe the disk has 4K sectors and QEMU is doing read-modify-write cycles to emulate 512 byte sectors? In this case, mismatches are not expected, since QEMU serializes RMW cycles, but at least we would know that the bug would be in QEMU, and where. Haven't considered this possibility. I'll look it up if the disk has 4k sectors and whether QEMU is emulating 512 bytes sectors. Paolo
[Qemu-block] Problem with data miscompare using scsi-hd, cache=none and io=threads
Hi, I've been working in the last two months in a miscompare issue that happens when using a raid device and a SATA as scsi-hd (emulated SCSI) with cache=none and io=threads during a hardware stress test. I'll summarize it here as best as I can without creating a great wall of text - Red Hat folks can check [1] for all the details. Using the following setup: - Host is a POWER9 RHEL 7.5-alt: kernel 4.14.0-49.1.1.el7a.ppc64le, qemu-kvm-ma 2.10.0-20.el7 (also reproducible with upstream QEMU) - Guest is RHEL 7.5-alt using the same kernel as the host, using two storage disks (a 1.8 Tb raid and a 446Gb SATA drive) as follows: dev='/dev/disk/by-id/scsi-3600605b000a2c110ff0004053d84a61b'/> Both block devices have WCE off in the host. With this env, we found problems when running a stress test called HTX [2]. At a given time (usually after 24+ hours of test) HTX finds a data miscompare in one of the devices. This is an example: --- Device name: /dev/sdb Total blocks: 0x74706daf, Block size: 0x200 Rule file name: /usr/lpp/htx/rules/reg/hxestorage/default.hdd Number of Rulefile passes (cycle) completed: 0 Stanza running: rule_6, Thread no.: 8 Oper performed: wrc, Current seek type: SEQ LBA no. where IO started: 0x94fa Transfer size: 0x8400 Miscompare Summary: === LBA no. where miscomapre started: 0x94fa LBA no. where miscomapre ended: 0x94ff Miscompare start offset (in bytes): 0x8 Miscomapre end offset (in bytes): 0xbff Miscompare size (in bytes): 0xbf8 Expected data (at miscomapre offset): 8c9aea5a736462007275 Actual data (at miscomapre offset): 889aea5a736462007275 - This means that the test executed a write at LBA 0x94fa and, after confirming that the write was completed, issue 2 reads in the same LBA to assert the written contents and found out a mismatch. I've tested all sort of configurations between disk vs LUN, cache modes and AIO. My findings are: - using device='lun' instead of device='disk', I can't reproduce the issue doesn't matter what other configurations are; - using device='disk' but with cache='writethrough', issue doesn't happen (haven't checked other cache modes); - using device='disk', cache='none' and io='native', issue doesn't happen. The issue seems to be tied with the combination device=disk + cache=none + io=threads. I've started digging into the SCSI layer all the way down to the block backend. With a shameful amount of logs I've discovered that, in the write that the test finds a miscompare, in block/file-posix.c: - when doing the write, handle_aiocb_rw_vector() returns success, pwritev() reports that all bytes were written - in both reads after the write, handle_aiocb_rw_vector returns success, all bytes read by preadv(). In both reads, the data read is different from the data written by the pwritev() that happened before In the discussions at [1], Fam Zheng suggested a test in which we would take down the number of threads created in the POSIX thread pool from 64 to 1. The idea is to ensure that we're using the same thread to write and read. There was a suspicion that the kernel can't guarantee data coherency between different threads, even if using the same fd, when using pwritev() and preadv(). This would explain why the following reads in the same fd would fail to retrieve the same data that was written before. After doing this modification, the miscompare didn't reproduce. After reverting the thread pool number change, I've made a couple of attempts trying to flush before read() and flushing after write(). Both attempts failed - the miscompare appears in both scenarios. This enforces the suspicion we have above - if data coherency can't be granted between different threads, flushing in different threads wouldn't make a difference too. I've also tested a suggestion from Fam where I started the disks with "cache.direct=on,cache.no-flush=off" - bug still reproduces. This is the current status of this investigation. I decided to start a discussion here, see if someone can point me something that I overlooked or got it wrong, before I started changing the POSIX thread pool behavior to see if I can enforce one specific POSIX thread to do a read() if we had a write() done in the same fd. Any suggestions? ps: it is worth mentioning that I was able to reproduce this same bug in a POWER8 system running Ubuntu 18.04. Given that the code we're dealing with doesn't have any arch-specific behavior I wouldn't be surprised if this bug is also reproducible in other archs like x86. Thanks, Daniel [1] https://bugzilla.redhat.com/show_bug.cgi?id=1561017 [2] https://github.com/open-power/HTX
