[Qemu-devel] [Bug 1790018] Re: Assertion failure (or segmentation fault) running 32-bit x86 Linux guest on 64-bit PowerPC host
This appears to be fixed by 9f754620651d3432114f4bb89c7f12cbea814b3e and present in 3.0.0. Closing. ** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1790018 Title: Assertion failure (or segmentation fault) running 32-bit x86 Linux guest on 64-bit PowerPC host Status in QEMU: Fix Released Bug description: Qemu 2.12.1 (also tried 2.12.0) Linux gwyn 4.14.48-mc8-easy #1 SMP Sat Jun 30 23:29:01 CDT 2018 ppc64 GNU/Linux gcc (Adelie 6.4.0-r9) 6.4.0 GNU assembler (GNU Binutils) 2.30 musl libc (powerpc64) Version 1.1.19 64-bit, 64-thread (16-core) POWER9 server in Big endian mode: processor : 0 cpu : POWER9, altivec supported clock : 3000.00MHz revision: 2.2 (pvr 004e 1202) Scenario: Attempting to install Adélie Linux 32-bit x86 guest on 64-bit PowerPC host using qemu-system-i386. Command line: /usr/bin/qemu-system-i386 -cdrom adelie-live- pmmx-1.0-beta1-20180807.iso -hda /dev/gwyn/x86 -m 512 -cpu pentium3 Environment reproduction: CD image can be obtained at https://distfiles.adelielinux.org/adelie/1.0-beta1/iso/adelie-live-pmmx-1.0-beta1-20180807.iso /dev/gwyn/x86 is an LVM2 logical volume, 4 GB in size, on NVMe storage Qemu was built from sources on this machine, with some distribution patches applied for musl support (does not affect tcg/ppc/* code); patches and build recipe (which was modified: https://bpaste.net/show/1bbb1d07d7f2 for recipe patch) can be found at: https://code.foxkit.us/adelie/packages/blob/master/user/qemu/APKBUILD Without --enable-debug-tcg: Thread 5 "qemu-system-i38" received signal SIGSEGV, Segmentation fault. [Switching to LWP 14090] 0x39fb04787f63db78 in ?? () (gdb) (gdb) bt #0 0x39fb04787f63db78 in () #1 0x31cdb160 in code_gen_buffer () #2 0x000100362048 in cpu_tb_exec (itb=, cpu=) at /usr/src/packages/user/qemu/src/qemu-2.12.1/accel/tcg/cpu-exec.c:169 #3 0x000100362048 in cpu_loop_exec_tb (tb_exit=, last_tb=, tb=, cpu=) at /usr/src/packages/user/qemu/src/qemu-2.12.1/accel/tcg/cpu-exec.c:626 #4 0x000100362048 in cpu_exec (cpu=) at /usr/src/packages/user/qemu/src/qemu-2.12.1/accel/tcg/cpu-exec.c:734 #5 0x0001003211b4 in tcg_cpu_exec (cpu=) at /usr/src/packages/user/qemu/src/qemu-2.12.1/cpus.c:1362 #6 0x0001003211b4 in qemu_tcg_rr_cpu_thread_fn (arg=) at /usr/src/packages/user/qemu/src/qemu-2.12.1/cpus.c:1461 #7 0x37fa275c in start (p=0x3fffedb6a810) at src/thread/pthread_create.c:147 #8 0x37fae4c8 in __clone () at src/thread/powerpc64/clone.s:43 With --enable-debug-tcg: Assertion failed: disp == (int16_t) disp (/usr/src/packages/user/qemu/src/qemu-2.12.1/tcg/ppc/tcg-target.inc.c: reloc_pc14_val: 204) zsh: abort qemu-system-i386 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1790018/+subscriptions
[Qemu-devel] [PATCH v3] intel_iommu: better handling of dmar state switch
QEMU is not handling the global DMAR switch well, especially when from
"on" to "off".
Let's first take the example of system reset.
Assuming that a guest has IOMMU enabled. When it reboots, we will drop
all the existing DMAR mappings to handle the system reset, however we'll
still keep the existing memory layouts which has the IOMMU memory region
enabled. So after the reboot and before the kernel reloads again, there
will be no mapping at all for the host device. That's problematic since
any software (for example, SeaBIOS) that runs earlier than the kernel
after the reboot will assume the IOMMU is disabled, so any DMA from the
software will fail.
For example, a guest that boots on an assigned NVMe device might fail to
find the boot device after a system reboot/reset and we'll be able to
observe SeaBIOS errors if we capture the debugging log:
WARNING - Timeout at nvme_wait:144!
Meanwhile, we should see DMAR errors on the host of that NVMe device.
It's the DMA fault that caused a NVMe driver timeout.
The correct fix should be that we do proper switching of device DMA
address spaces when system resets, which will setup correct memory
regions and notify the backend of the devices. This might not affect
much on non-assigned devices since QEMU VT-d emulation will assume a
default passthrough mapping if DMAR is not enabled in the GCMD
register (please refer to vtd_iommu_translate). However that's required
for an assigned devices, since that'll rebuild the correct GPA to HPA
mapping that is needed for any DMA operation during guest bootstrap.
Besides the system reset, we have some other places that might change
the global DMAR status and we'd better do the same thing there. For
example, when we change the state of GCMD register, or the DMAR root
pointer. Do the same refresh for all these places.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173
CC: QEMU Stable
Reported-by: Cong Li
Signed-off-by: Peter Xu
--
v2:
- do the same for GCMD write, or root pointer update [Alex]
- test is carried out by me this time, by observing the
vtd_switch_address_space tracepoint after system reboot
v3:
- rewrite commit message as suggested by Alex
---
hw/i386/intel_iommu.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 3dfada19a6..59dc155911 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -37,6 +37,8 @@
#include "kvm_i386.h"
#include "trace.h"
+static void vtd_address_space_refresh_all(IntelIOMMUState *s);
+
static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
uint64_t wmask, uint64_t w1cmask)
{
@@ -1428,7 +1430,7 @@ static void vtd_context_global_invalidate(IntelIOMMUState
*s)
vtd_reset_context_cache_locked(s);
}
vtd_iommu_unlock(s);
-vtd_switch_address_space_all(s);
+vtd_address_space_refresh_all(s);
/*
* From VT-d spec 6.5.2.1, a global context entry invalidation
* should be followed by a IOTLB global invalidation, so we should
@@ -1719,6 +1721,7 @@ static void vtd_handle_gcmd_srtp(IntelIOMMUState *s)
vtd_root_table_setup(s);
/* Ok - report back to driver */
vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
+vtd_address_space_refresh_all(s);
}
/* Set Interrupt Remap Table Pointer */
@@ -1751,7 +1754,7 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool
en)
vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
}
-vtd_switch_address_space_all(s);
+vtd_address_space_refresh_all(s);
}
/* Handle Interrupt Remap Enable/Disable */
@@ -3051,6 +3054,12 @@ static void vtd_address_space_unmap_all(IntelIOMMUState
*s)
}
}
+static void vtd_address_space_refresh_all(IntelIOMMUState *s)
+{
+vtd_address_space_unmap_all(s);
+vtd_switch_address_space_all(s);
+}
+
static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private)
{
memory_region_notify_one((IOMMUNotifier *)private, entry);
@@ -3226,11 +3235,7 @@ static void vtd_reset(DeviceState *dev)
IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
vtd_init(s);
-
-/*
- * When device reset, throw away all mappings and external caches
- */
-vtd_address_space_unmap_all(s);
+vtd_address_space_refresh_all(s);
}
static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
--
2.17.1
Re: [Qemu-devel] [PATCH] intel_iommu: do address space switching when reset
On Thu, Sep 06, 2018 at 07:56:49PM -0600, Alex Williamson wrote: > On Fri, 7 Sep 2018 09:00:31 +0800 > Peter Xu wrote: > > > On Thu, Sep 06, 2018 at 12:41:36PM -0600, Alex Williamson wrote: > > > On Thu, 6 Sep 2018 14:53:12 +0800 > > > Peter Xu wrote: > > > > > > > On Wed, Sep 05, 2018 at 08:55:50AM -0600, Alex Williamson wrote: > > > > > On Wed, 5 Sep 2018 19:31:58 +0800 > > > > > Peter Xu wrote: > > > > > > > > > > > We will drop all the mappings when system reset, however we'll still > > > > > > keep the existing memory layouts. That'll be problematic since if > > > > > > IOMMU > > > > > > is enabled in the guest and then reboot the guest, SeaBIOS will try > > > > > > to > > > > > > drive a device that with no page mapped there. What we need to do > > > > > > is to > > > > > > rebuild the GPA->HPA mapping when system resets, hence ease SeaBIOS. > > > > > > > > > > > > Without this patch, a guest that boots on an assigned NVMe device > > > > > > might > > > > > > fail to find the boot device after a system reboot/reset and we'll > > > > > > be > > > > > > able to observe SeaBIOS errors if turned on: > > > > > > > > > > > > WARNING - Timeout at nvme_wait:144! > > > > > > > > > > > > With the patch applied, the guest will be able to find the NVMe > > > > > > drive > > > > > > and bootstrap there even after multiple reboots or system resets. > > > > > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173 > > > > > > CC: QEMU Stable > > > > > > Tested-by: Cong Li > > > > > > Signed-off-by: Peter Xu > > > > > > --- > > > > > > hw/i386/intel_iommu.c | 8 > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > > > index 3dfada19a6..d3eb068d43 100644 > > > > > > --- a/hw/i386/intel_iommu.c > > > > > > +++ b/hw/i386/intel_iommu.c > > > > > > @@ -3231,6 +3231,14 @@ static void vtd_reset(DeviceState *dev) > > > > > > * When device reset, throw away all mappings and external > > > > > > caches > > > > > > */ > > > > > > vtd_address_space_unmap_all(s); > > > > > > + > > > > > > +/* > > > > > > + * Switch address spaces if needed (e.g., when reboot from a > > > > > > + * kernel that has IOMMU enabled, we should switch address > > > > > > spaces > > > > > > + * to rebuild the GPA->HPA mappings otherwise SeaBIOS might > > > > > > + * encounter DMA errors when running with e.g. a NVMe card). > > > > > > + */ > > > > > > +vtd_switch_address_space_all(s); > > > > > > } > > > > > > > > > > > > static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, > > > > > > int devfn) > > > > > > > > > > I'm curious why these aren't part of vtd_init(). vtd_init is where > > > > > GCMD is set back to it's power-on state, which disables translation, > > > > > so > > > > > logically we should reset the address space at that point. Similarly, > > > > > the root entry is reset, so it would make sense to throw away all the > > > > > mappings there too. Thanks, > > > > > > > > vtd_init() is only called when realize() or reset, and AFAIU it's not > > > > called by GCMD operations. However I think I get the point that > > > > logically we should do similar things in e.g. vtd_handle_gcmd_srtp() > > > > when the enable bit switches. > > > > > > > > My understanding is that if other things happened rather than the > > > > system reboot (e.g., when root pointer is replaced, or during the > > > > guest running the guest driver turns DMAR from on to off) the guest > > > > will be responsible to do the rest of invalidations first before doing > > > > that switch, so we'll possibly do the unmap_all() and address space > > > > switches in other places (e.g., in vtd_context_global_invalidate, or > > > > per device invalidations). > > > > > > AIUI, the entire global command register is write-once, so the guest > > > cannot disable the IOMMU or change the root pointer after it's been > > > initialized, except through a system reset. > > > > I'm not sure about this one. The spec has this though (chap 10.4.4, > > Global Command Register): > > > > Register to control remapping hardware. If multiple control > > fields in this register need to be modified, software must > > serialize the modifications through multiple writes to this > > register. > > > > So AFAIU it's at least not write-once register since after all the > > guest software need to update the register in a per-bit fashion. And > > AFAIU there's no restriction too on turning the global DMAR off after > > it's turned on (though Linux won't do that). > > I'm sorry, my brain was elsewhere, WO = Write Only, not sure where I > came up with Write Once. I do have an impression that there's > something one-way about enabling the IOMMU, but I'm not sure where it > is. No problem. My impression was that I kept that idea until someday someone told
Re: [Qemu-devel] [PATCH] intel_iommu: do address space switching when reset
On Fri, 7 Sep 2018 09:00:31 +0800 Peter Xu wrote: > On Thu, Sep 06, 2018 at 12:41:36PM -0600, Alex Williamson wrote: > > On Thu, 6 Sep 2018 14:53:12 +0800 > > Peter Xu wrote: > > > > > On Wed, Sep 05, 2018 at 08:55:50AM -0600, Alex Williamson wrote: > > > > On Wed, 5 Sep 2018 19:31:58 +0800 > > > > Peter Xu wrote: > > > > > > > > > We will drop all the mappings when system reset, however we'll still > > > > > keep the existing memory layouts. That'll be problematic since if > > > > > IOMMU > > > > > is enabled in the guest and then reboot the guest, SeaBIOS will try to > > > > > drive a device that with no page mapped there. What we need to do is > > > > > to > > > > > rebuild the GPA->HPA mapping when system resets, hence ease SeaBIOS. > > > > > > > > > > Without this patch, a guest that boots on an assigned NVMe device > > > > > might > > > > > fail to find the boot device after a system reboot/reset and we'll be > > > > > able to observe SeaBIOS errors if turned on: > > > > > > > > > > WARNING - Timeout at nvme_wait:144! > > > > > > > > > > With the patch applied, the guest will be able to find the NVMe drive > > > > > and bootstrap there even after multiple reboots or system resets. > > > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173 > > > > > CC: QEMU Stable > > > > > Tested-by: Cong Li > > > > > Signed-off-by: Peter Xu > > > > > --- > > > > > hw/i386/intel_iommu.c | 8 > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > > index 3dfada19a6..d3eb068d43 100644 > > > > > --- a/hw/i386/intel_iommu.c > > > > > +++ b/hw/i386/intel_iommu.c > > > > > @@ -3231,6 +3231,14 @@ static void vtd_reset(DeviceState *dev) > > > > > * When device reset, throw away all mappings and external caches > > > > > */ > > > > > vtd_address_space_unmap_all(s); > > > > > + > > > > > +/* > > > > > + * Switch address spaces if needed (e.g., when reboot from a > > > > > + * kernel that has IOMMU enabled, we should switch address spaces > > > > > + * to rebuild the GPA->HPA mappings otherwise SeaBIOS might > > > > > + * encounter DMA errors when running with e.g. a NVMe card). > > > > > + */ > > > > > +vtd_switch_address_space_all(s); > > > > > } > > > > > > > > > > static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, > > > > > int devfn) > > > > > > > > I'm curious why these aren't part of vtd_init(). vtd_init is where > > > > GCMD is set back to it's power-on state, which disables translation, so > > > > logically we should reset the address space at that point. Similarly, > > > > the root entry is reset, so it would make sense to throw away all the > > > > mappings there too. Thanks, > > > > > > vtd_init() is only called when realize() or reset, and AFAIU it's not > > > called by GCMD operations. However I think I get the point that > > > logically we should do similar things in e.g. vtd_handle_gcmd_srtp() > > > when the enable bit switches. > > > > > > My understanding is that if other things happened rather than the > > > system reboot (e.g., when root pointer is replaced, or during the > > > guest running the guest driver turns DMAR from on to off) the guest > > > will be responsible to do the rest of invalidations first before doing > > > that switch, so we'll possibly do the unmap_all() and address space > > > switches in other places (e.g., in vtd_context_global_invalidate, or > > > per device invalidations). > > > > AIUI, the entire global command register is write-once, so the guest > > cannot disable the IOMMU or change the root pointer after it's been > > initialized, except through a system reset. > > I'm not sure about this one. The spec has this though (chap 10.4.4, > Global Command Register): > > Register to control remapping hardware. If multiple control > fields in this register need to be modified, software must > serialize the modifications through multiple writes to this > register. > > So AFAIU it's at least not write-once register since after all the > guest software need to update the register in a per-bit fashion. And > AFAIU there's no restriction too on turning the global DMAR off after > it's turned on (though Linux won't do that). I'm sorry, my brain was elsewhere, WO = Write Only, not sure where I came up with Write Once. I do have an impression that there's something one-way about enabling the IOMMU, but I'm not sure where it is. > > I think that means the > > guest can only operate through the invalidation queue at runtime. The > > bug being fixed here is that the IOMMU has been reset to its power-on > > state where translation is disabled, but the emulation of that disabled > > state also needs to return the per-device address space to that of > > system memory, or identity map thereof. The commit log seems to imply > > that there's
Re: [Qemu-devel] [PATCH v2] hw/ppc: on 40p machine, change default firmware to OpenBIOS
On Thu, Sep 06, 2018 at 05:38:26AM +0100, Mark Cave-Ayland wrote:
> On 05/09/18 01:13, David Gibson wrote:
>
> > On Tue, Sep 04, 2018 at 09:49:03PM +0200, Hervé Poussineau wrote:
> >> OpenBIOS gained 40p support in 5b20e4cacecb62fb2bdc6867c11d44cddd77c4ff
> >> Use it, instead of relying on an unmaintained and very limited firmware.
> >>
> >> Signed-off-by: Hervé Poussineau
> >
> > Uh.. against current ppc-for-3.1, plase.
>
> I was a bit confused as to why this failed to apply since the original
> had been part of a local branch for a while, but just noticed it was
> because of this change to Hervé's original which I had missed:
>
> [dwg: Drop prep from boot-serial test to avoid deprecation warnings]
>
> Included below is the new diff against ppc-for-3.1: David, is this
> enough for you to be able to fix up manually without a v3?
Well, I could have fixed it up manually from v2 - but I'm pushing that
busy work back on you as a contributor, because I'm having trouble
enough finding dtc maintenance time as it is.
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 162b27a3b8..baca1d7c04 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -651,7 +651,7 @@ static void ibm_40p_init(MachineState *machine)
> /* PCI host */
> dev = qdev_create(NULL, "raven-pcihost");
> if (!bios_name) {
> -bios_name = BIOS_FILENAME;
> +bios_name = "openbios-ppc";
> }
> qdev_prop_set_string(dev, "bios-name", bios_name);
> qdev_prop_set_uint32(dev, "elf-machine", PPC_ELF_MACHINE);
> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> index f123b15e3e..f865822e32 100644
> --- a/tests/boot-serial-test.c
> +++ b/tests/boot-serial-test.c
> @@ -75,12 +75,12 @@ typedef struct testdef {
> static testdef_t tests[] = {
> { "alpha", "clipper", "", "PCI:" },
> { "ppc", "ppce500", "", "U-Boot" },
> -{ "ppc", "40p", "-boot d", "Booting from device d" },
> +{ "ppc", "40p", "-vga none -boot d", "Trying cd:," },
> { "ppc", "g3beige", "", "PowerPC,750" },
> { "ppc", "mac99", "", "PowerPC,G4" },
> { "ppc", "sam460ex", "-m 256", "DRAM: 256 MiB" },
> { "ppc64", "ppce500", "", "U-Boot" },
> -{ "ppc64", "40p", "-m 192", "Memory size: 192 MB" },
> +{ "ppc64", "40p", "-m 192", "Memory: 192M" },
> { "ppc64", "mac99", "", "PowerPC,970FX" },
> { "ppc64", "pseries", "", "Open Firmware" },
> { "ppc64", "powernv", "-cpu POWER8", "OPAL" },
>
>
> ATB,
>
> Mark.
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] target-ppc: Extend HWCAP2 bits for ISA 3.0
On Thu, Sep 06, 2018 at 12:27:29PM +0530, Sandipan Das wrote:
> This adds the HWCAP2 bit to detect if a linux user process is
> running on an ISA 3.0 compliant cpu like POWER9. This can be
> verified using a simple test program that prints the value in
> the auxiliary vector for AT_HWCAP2 as shown below.
>
> Before:
> $ qemu-ppc64le -cpu power8 test
> 0x8c00
>
> $ qemu-ppc64le -cpu power9 test
> 0x8c00
>
> After:
> $ qemu-ppc64le -cpu power8 test
> 0x8c00
>
> $ qemu-ppc64le -cpu power9 test
> 0x8c80
>
> Signed-off-by: Sandipan Das
Applied, thanks.
> ---
> linux-user/elfload.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 8638612aec..e97c4cde49 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -710,6 +710,7 @@ enum {
> QEMU_PPC_FEATURE2_HAS_EBB = 0x1000, /* Event Base Branching */
> QEMU_PPC_FEATURE2_HAS_ISEL = 0x0800, /* Integer Select */
> QEMU_PPC_FEATURE2_HAS_TAR = 0x0400, /* Target Address Register */
> +QEMU_PPC_FEATURE2_ARCH_3_00 = 0x0080, /* ISA 3.00 */
> };
>
> #define ELF_HWCAP get_elf_hwcap()
> @@ -764,6 +765,7 @@ static uint32_t get_elf_hwcap2(void)
> GET_FEATURE2(PPC2_BCTAR_ISA207, QEMU_PPC_FEATURE2_HAS_TAR);
> GET_FEATURE2((PPC2_BCTAR_ISA207 | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 |
>PPC2_ISA207S), QEMU_PPC_FEATURE2_ARCH_2_07);
> +GET_FEATURE2(PPC2_ISA300, QEMU_PPC_FEATURE2_ARCH_3_00);
>
> #undef GET_FEATURE
> #undef GET_FEATURE2
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] intel_iommu: do address space switching when reset
On Thu, Sep 06, 2018 at 12:41:36PM -0600, Alex Williamson wrote: > On Thu, 6 Sep 2018 14:53:12 +0800 > Peter Xu wrote: > > > On Wed, Sep 05, 2018 at 08:55:50AM -0600, Alex Williamson wrote: > > > On Wed, 5 Sep 2018 19:31:58 +0800 > > > Peter Xu wrote: > > > > > > > We will drop all the mappings when system reset, however we'll still > > > > keep the existing memory layouts. That'll be problematic since if IOMMU > > > > is enabled in the guest and then reboot the guest, SeaBIOS will try to > > > > drive a device that with no page mapped there. What we need to do is to > > > > rebuild the GPA->HPA mapping when system resets, hence ease SeaBIOS. > > > > > > > > Without this patch, a guest that boots on an assigned NVMe device might > > > > fail to find the boot device after a system reboot/reset and we'll be > > > > able to observe SeaBIOS errors if turned on: > > > > > > > > WARNING - Timeout at nvme_wait:144! > > > > > > > > With the patch applied, the guest will be able to find the NVMe drive > > > > and bootstrap there even after multiple reboots or system resets. > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173 > > > > CC: QEMU Stable > > > > Tested-by: Cong Li > > > > Signed-off-by: Peter Xu > > > > --- > > > > hw/i386/intel_iommu.c | 8 > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > index 3dfada19a6..d3eb068d43 100644 > > > > --- a/hw/i386/intel_iommu.c > > > > +++ b/hw/i386/intel_iommu.c > > > > @@ -3231,6 +3231,14 @@ static void vtd_reset(DeviceState *dev) > > > > * When device reset, throw away all mappings and external caches > > > > */ > > > > vtd_address_space_unmap_all(s); > > > > + > > > > +/* > > > > + * Switch address spaces if needed (e.g., when reboot from a > > > > + * kernel that has IOMMU enabled, we should switch address spaces > > > > + * to rebuild the GPA->HPA mappings otherwise SeaBIOS might > > > > + * encounter DMA errors when running with e.g. a NVMe card). > > > > + */ > > > > +vtd_switch_address_space_all(s); > > > > } > > > > > > > > static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int > > > > devfn) > > > > > > I'm curious why these aren't part of vtd_init(). vtd_init is where > > > GCMD is set back to it's power-on state, which disables translation, so > > > logically we should reset the address space at that point. Similarly, > > > the root entry is reset, so it would make sense to throw away all the > > > mappings there too. Thanks, > > > > vtd_init() is only called when realize() or reset, and AFAIU it's not > > called by GCMD operations. However I think I get the point that > > logically we should do similar things in e.g. vtd_handle_gcmd_srtp() > > when the enable bit switches. > > > > My understanding is that if other things happened rather than the > > system reboot (e.g., when root pointer is replaced, or during the > > guest running the guest driver turns DMAR from on to off) the guest > > will be responsible to do the rest of invalidations first before doing > > that switch, so we'll possibly do the unmap_all() and address space > > switches in other places (e.g., in vtd_context_global_invalidate, or > > per device invalidations). > > AIUI, the entire global command register is write-once, so the guest > cannot disable the IOMMU or change the root pointer after it's been > initialized, except through a system reset. I'm not sure about this one. The spec has this though (chap 10.4.4, Global Command Register): Register to control remapping hardware. If multiple control fields in this register need to be modified, software must serialize the modifications through multiple writes to this register. So AFAIU it's at least not write-once register since after all the guest software need to update the register in a per-bit fashion. And AFAIU there's no restriction too on turning the global DMAR off after it's turned on (though Linux won't do that). > I think that means the > guest can only operate through the invalidation queue at runtime. The > bug being fixed here is that the IOMMU has been reset to its power-on > state where translation is disabled, but the emulation of that disabled > state also needs to return the per-device address space to that of > system memory, or identity map thereof. The commit log seems to imply > that there's some sort of SeaBIOS issue and we're just doing this to > help the BIOS, when in reality, we just forgot to reset the per device > address space and a subsequent boot of a guest that didn't enable the > IOMMU would have the same sort of issues. In fact any usage of an > assigned device prior to re-enabling the IOMMU would fail, there's > nothing unique to NVMe here. Thanks, Sorry for the confusion on the warning line. I pasted that line to mainly help people when looking for solutions of
Re: [Qemu-devel] [PATCH] RISC-V - Dynamic parameterization of RISC-V memory map
On 09/06/2018 08:44 AM, Peter Maydell wrote: On 6 September 2018 at 16:07, Michael Eager wrote: Any comments? I'd quite like to hear from somebody more familiar with the readconfig/writeconfig stuff than me about whether this very riscv-centric approach makes sense and fits with how the config file is used by other parts of QEMU. Hi Peter -- This patch is specific to RISC-V, but it would be easy to generalize it to any target. It could also be extended to describe other processor features, for example, the number of cores. Generalizing it would involve refactoring the changes to create a generic routine which could be used in any target. I'm not sure who would be best to do that review, though. Paolo, any suggestions for who knows that bit of the code? thanks -- PMM On 08/30/2018 09:22 AM, Michael Eager wrote: Corrected patch attached. On 08/29/2018 05:48 PM, Michael Eager wrote: Whoops. I just noticed that this patch is against the riscv-qemu repo on github, not the qemu.org repo. I will rework it for the qemu.org repo. Meanwhile, I welcome any comments. On 08/29/2018 05:21 PM, Michael Eager wrote: Memory parameters for RISC-V boards can be read from a configuration file using the -readconfig command line option. The configuration file should have a section for the board and memory. The configuration for the VirtIO board has the following configuration variables: [riscv-virt-mem] debug-base = "0x0" debug-size = "0x100" mrom-base= "0x1000" mrom-size= "0x11000" test-base= "0x10" test-size= "0x1000" clint-base = "0x200" clint-size = "0x1" plic-base= "0xc00" plic-size= "0x400" uart0-base = "0x1000" uart0-size = "0x100" virtio-base = "0x10001000" virtio-size = "0x1000" dram-base= "0x8000" dram-size= "0x0" Values must be enclosed within quotes. Signed-off-by: Michael Eager -- Michael Eagerea...@eagerm.com 1960 Park Blvd., Palo Alto, CA 94306 -- Michael Eagerea...@eagerm.com 1960 Park Blvd., Palo Alto, CA 94306
Re: [Qemu-devel] [PATCH v2] monitor: print message when using 'help' with an unknown command
On 09/06/2018 03:28 PM, Dr. David Alan Gilbert wrote: > * Collin Walling (wall...@linux.ibm.com) wrote: >> On 08/08/2018 03:00 PM, Dr. David Alan Gilbert wrote: >>> * Collin Walling (wall...@linux.ibm.com) wrote: When typing 'help' followed by an unknown command, QEMU will not print anything to the command line to let the user know they typed a bad command. Let's fix this by printing a message to the monitor when this happens. For example: (qemu) help xyz unknown command: 'xyz' Reported-by: Stefan Zimmermann Signed-off-by: Collin Walling >>> >>> Reviewed-by: Dr. David Alan Gilbert >>> >> >> Thank you for the R-b. >> >> And pardon my impatience, but any chance this patch might get picked up, or >> does it require some more review first? > > It will on my next HMP pull; it's just not many pulls are happening > at the moment; it should be in by the end of the month. > > Dave Fair enough. Thank you for the update. > >> [...] >> >> >> -- >> Respectfully, >> - Collin Walling >> > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Respectfully, - Collin Walling
Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor
On 09/06/2018 12:57 PM, Jeff Cody wrote:
> On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
>> For purposes of minimum code movement, refactor the mirror_exit
>> callback to use the post-finalization callbacks in a trivial way.
>>
>> Signed-off-by: John Snow
>> ---
>> block/mirror.c | 39 ---
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index bd3e908710..a92b4702c5 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>> int max_iov;
>> bool initial_zeroing_ongoing;
>> int in_active_write_counter;
>> +bool prepared;
>> } MirrorBlockJob;
>>
>> typedef struct MirrorBDSOpaque {
>> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>> }
>> }
>>
>> -static void mirror_exit(Job *job)
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
* for .abort cases, denoted by abort = true, MUST return 0.
*/
>> +static int mirror_exit_common(Job *job)
>> {
>> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>> BlockJob *bjob = >common;
>> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>> BlockDriverState *target_bs = blk_bs(s->target);
>> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>> Error *local_err = NULL;
>> -int ret = job->ret;
>> +bool abort = job->ret < 0;
>> +int ret = 0;
>> +
>> +if (s->prepared) {
>> +return 0;
>> +}
>> +s->prepared = true;
>>
>> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>
>> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
>> * required before it could become a backing file of target_bs. */
>> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>> _abort);
>> -if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> +if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> BlockDriverState *backing = s->is_none_mode ? src : s->base;
>> if (backing_bs(target_bs) != backing) {
>> bdrv_set_backing_hd(target_bs, backing, _err);
>> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
>> aio_context_acquire(replace_aio_context);
>> }
>>
>> -if (s->should_complete && ret == 0) {
>> -BlockDriverState *to_replace = src;
>> -if (s->to_replace) {
>> -to_replace = s->to_replace;
>> -}
>> +if (s->should_complete && !abort) {
>> +BlockDriverState *to_replace = s->to_replace ?: src;
>>
>> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>> bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
>> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
>> bdrv_unref(mirror_top_bs);
>> bdrv_unref(src);
>>
>> -job->ret = ret;
>> +return ret;
>> +}
>> +
>> +static int mirror_prepare(Job *job)
>> +{
>> +return mirror_exit_common(job);
>> +}
>> +
>> +static void mirror_abort(Job *job)
>> +{
>> +int ret = mirror_exit_common(job);
>> +assert(ret == 0);
>
> Not something to hold the series up, but in case a v6 is called for due to
> other changes: I think it may be worth a comment in mirror_exit_common()
> that if abort is true, and we don't return success, QEMU will hit an assert.
> Mainly to prevent someone from including a call with a potential error
> return in the abort path in the future.
>
> Reviewed-by: Jeff Cody
>
Yeah, I should have added a comment, like the one you read before you
got to this comment, by the very nature of how email works.
--js
Re: [Qemu-devel] [PATCH v11 6/6] tpm: add ACPI memory clear interface
On 09/05/2018 11:29 PM, Marc-André Lureau wrote:
This allows to pass the last failing test from the Windows HLK TPM 2.0
TCG PPI 1.3 tests.
The interface is described in the "TCG Platform Reset Attack
Mitigation Specification", chapter 6 "ACPI _DSM Function". According
to Laszlo, it's not so easy to implement in OVMF, he suggested to do
it in qemu instead.
Signed-off-by: Marc-André Lureau
---
hw/tpm/tpm_ppi.h | 2 ++
hw/i386/acpi-build.c | 46
hw/tpm/tpm_crb.c | 1 +
hw/tpm/tpm_ppi.c | 23 ++
hw/tpm/tpm_tis.c | 1 +
docs/specs/tpm.txt | 2 ++
hw/tpm/trace-events | 3 +++
7 files changed, 78 insertions(+)
diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
index c2ab2ed300..b8f67962c7 100644
--- a/hw/tpm/tpm_ppi.h
+++ b/hw/tpm/tpm_ppi.h
@@ -23,4 +23,6 @@ typedef struct TPMPPI {
bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
hwaddr addr, Object *obj, Error **errp);
+void tpm_ppi_reset(TPMPPI *tpmppi);
+
#endif /* TPM_TPM_PPI_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c5e9a6e11d..2ab3e8fae7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
pprq = aml_name("PPRQ");
pprm = aml_name("PPRM");
+aml_append(dev,
+ aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
+aml_int(TPM_PPI_ADDR_BASE + 0x15a),
+0x1));
+field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
+aml_append(field, aml_named_field("MOVV", 8));
+aml_append(dev, field);
/*
* DerefOf in Windows is broken with SYSTEM_MEMORY. Use a dynamic
* operation region inside of a method for getting FUNC[op].
@@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
}
aml_append(method, ifctx);
+
+ifctx = aml_if(
+aml_equal(uuid,
+ aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
+{
+/* standard DSM query function */
+ifctx2 = aml_if(aml_equal(function, zero));
+{
+uint8_t byte_list[1] = { 0x03 };
+aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
+}
+aml_append(ifctx, ifctx2);
+
+/*
+ * TCG Platform Reset Attack Mitigation Specification 1.0 Ch.6
+ *
+ * Arg 2 (Integer): Function Index = 1
+ * Arg 3 (Package): Arguments = Package: Type: Integer
+ * Operation Value of the Request
+ * Returns: Type: Integer
+ * 0: Success
+ * 1: General Failure
+ */
+ifctx2 = aml_if(aml_equal(function, one));
+{
+aml_append(ifctx2,
+ aml_store(aml_derefof(aml_index(arguments, zero)),
+ op));
+{
+aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
+
+/* 0: success */
+aml_append(ifctx2, aml_return(zero));
+}
+}
+aml_append(ifctx, ifctx2);
+}
+aml_append(method, ifctx);
}
+
aml_append(dev, method);
}
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index b243222fd6..48f6a716ad 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
{
CRBState *s = CRB(dev);
+tpm_ppi_reset(>ppi);
tpm_backend_reset(s->tpmbe);
memset(s->regs, 0, sizeof(s->regs));
diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
index f2f07f895e..46ca8ea3ea 100644
--- a/hw/tpm/tpm_ppi.c
+++ b/hw/tpm/tpm_ppi.c
@@ -16,8 +16,30 @@
#include "qapi/error.h"
#include "cpu.h"
#include "sysemu/memory_mapping.h"
+#include "sysemu/reset.h"
#include "migration/vmstate.h"
#include "tpm_ppi.h"
+#include "trace.h"
+
+void tpm_ppi_reset(TPMPPI *tpmppi)
+{
+char *ptr = memory_region_get_ram_ptr(>ram);
+
+if (ptr[0x15a] & 0x1) {
+GuestPhysBlockList guest_phys_blocks;
+GuestPhysBlock *block;
+
+guest_phys_blocks_init(_phys_blocks);
+guest_phys_blocks_append(_phys_blocks);
+QTAILQ_FOREACH(block, _phys_blocks.head, next) {
+trace_tpm_ppi_memset(block->host_addr,
+ block->target_end - block->target_start);
+memset(block->host_addr, 0,
+ block->target_end - block->target_start);
Does this also clear the PPI memory? If so, could we create a backup of
the few relevant bytes the firmware will look at and restore them after
the loop? The PPI device likely contains the only data that may need to
be preserved across a
Re: [Qemu-devel] [PATCH v2] monitor: print message when using 'help' with an unknown command
* Collin Walling (wall...@linux.ibm.com) wrote: > On 08/08/2018 03:00 PM, Dr. David Alan Gilbert wrote: > > * Collin Walling (wall...@linux.ibm.com) wrote: > >> When typing 'help' followed by an unknown command, QEMU will > >> not print anything to the command line to let the user know > >> they typed a bad command. Let's fix this by printing a message > >> to the monitor when this happens. For example: > >> > >> (qemu) help xyz > >> unknown command: 'xyz' > >> > >> Reported-by: Stefan Zimmermann > >> Signed-off-by: Collin Walling > > > > Reviewed-by: Dr. David Alan Gilbert > > > > Thank you for the R-b. > > And pardon my impatience, but any chance this patch might get picked up, or > does it require some more review first? It will on my next HMP pull; it's just not many pulls are happening at the moment; it should be in by the end of the month. Dave > [...] > > > -- > Respectfully, > - Collin Walling > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface
On 09/06/18 19:23, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> Hi
>>
>> On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
>> wrote:
>>>
>>> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
Hi
On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
wrote:
>
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> Hi
>>
>> On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov
>> wrote:
>>>
>>> On Thu, 6 Sep 2018 07:50:09 +0400
>>> Marc-André Lureau wrote:
>>>
Hi
On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov
wrote:
>
> On Fri, 31 Aug 2018 19:24:24 +0200
> Marc-André Lureau wrote:
>
>> This allows to pass the last failing test from the Windows HLK TPM
>> 2.0
>> TCG PPI 1.3 tests.
>>
>> The interface is described in the "TCG Platform Reset Attack
>> Mitigation Specification", chapter 6 "ACPI _DSM Function". According
>> to Laszlo, it's not so easy to implement in OVMF, he suggested to do
>> it in qemu instead.
>>
>> Signed-off-by: Marc-André Lureau
>> ---
>> hw/tpm/tpm_ppi.h | 2 ++
>> hw/i386/acpi-build.c | 46
>>
>> hw/tpm/tpm_crb.c | 1 +
>> hw/tpm/tpm_ppi.c | 23 ++
>> hw/tpm/tpm_tis.c | 1 +
>> docs/specs/tpm.txt | 2 ++
>> hw/tpm/trace-events | 3 +++
>> 7 files changed, 78 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
>> index f6458bf87e..3239751e9f 100644
>> --- a/hw/tpm/tpm_ppi.h
>> +++ b/hw/tpm/tpm_ppi.h
>> @@ -23,4 +23,6 @@ typedef struct TPMPPI {
>> bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>>hwaddr addr, Object *obj, Error **errp);
>>
>> +void tpm_ppi_reset(TPMPPI *tpmppi);
>> +
>> #endif /* TPM_TPM_PPI_H */
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index c5e9a6e11d..2ab3e8fae7 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>> pprq = aml_name("PPRQ");
>> pprm = aml_name("PPRM");
>>
>> +aml_append(dev,
>> + aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
>> +aml_int(TPM_PPI_ADDR_BASE +
>> 0x15a),
>> +0x1));
>> +field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK,
>> AML_PRESERVE);
>> +aml_append(field, aml_named_field("MOVV", 8));
>> +aml_append(dev, field);
>> /*
>> * DerefOf in Windows is broken with SYSTEM_MEMORY. Use a
>> dynamic
>> * operation region inside of a method for getting FUNC[op].
>> @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>> aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>> }
>> aml_append(method, ifctx);
>> +
>> +ifctx = aml_if(
>> +aml_equal(uuid,
>> +
>> aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>> +{
>> +/* standard DSM query function */
>> +ifctx2 = aml_if(aml_equal(function, zero));
>> +{
>> +uint8_t byte_list[1] = { 0x03 };
>> +aml_append(ifctx2, aml_return(aml_buffer(1,
>> byte_list)));
>> +}
>> +aml_append(ifctx, ifctx2);
>> +
>> +/*
>> + * TCG Platform Reset Attack Mitigation Specification
>> 1.0 Ch.6
>> + *
>> + * Arg 2 (Integer): Function Index = 1
>> + * Arg 3 (Package): Arguments = Package: Type: Integer
>> + * Operation Value of the Request
>> + * Returns: Type: Integer
>> + * 0: Success
>> + * 1: General Failure
>> + */
>> +ifctx2 = aml_if(aml_equal(function, one));
>> +{
>> +aml_append(ifctx2,
>> +
>> aml_store(aml_derefof(aml_index(arguments, zero)),
>> + op));
>> +{
>> +aml_append(ifctx2, aml_store(op,
>> aml_name("MOVV")));
>> +
>> +/* 0:
Re: [Qemu-devel] [PATCH] intel_iommu: do address space switching when reset
On Thu, 6 Sep 2018 14:53:12 +0800 Peter Xu wrote: > On Wed, Sep 05, 2018 at 08:55:50AM -0600, Alex Williamson wrote: > > On Wed, 5 Sep 2018 19:31:58 +0800 > > Peter Xu wrote: > > > > > We will drop all the mappings when system reset, however we'll still > > > keep the existing memory layouts. That'll be problematic since if IOMMU > > > is enabled in the guest and then reboot the guest, SeaBIOS will try to > > > drive a device that with no page mapped there. What we need to do is to > > > rebuild the GPA->HPA mapping when system resets, hence ease SeaBIOS. > > > > > > Without this patch, a guest that boots on an assigned NVMe device might > > > fail to find the boot device after a system reboot/reset and we'll be > > > able to observe SeaBIOS errors if turned on: > > > > > > WARNING - Timeout at nvme_wait:144! > > > > > > With the patch applied, the guest will be able to find the NVMe drive > > > and bootstrap there even after multiple reboots or system resets. > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173 > > > CC: QEMU Stable > > > Tested-by: Cong Li > > > Signed-off-by: Peter Xu > > > --- > > > hw/i386/intel_iommu.c | 8 > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index 3dfada19a6..d3eb068d43 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -3231,6 +3231,14 @@ static void vtd_reset(DeviceState *dev) > > > * When device reset, throw away all mappings and external caches > > > */ > > > vtd_address_space_unmap_all(s); > > > + > > > +/* > > > + * Switch address spaces if needed (e.g., when reboot from a > > > + * kernel that has IOMMU enabled, we should switch address spaces > > > + * to rebuild the GPA->HPA mappings otherwise SeaBIOS might > > > + * encounter DMA errors when running with e.g. a NVMe card). > > > + */ > > > +vtd_switch_address_space_all(s); > > > } > > > > > > static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int > > > devfn) > > > > I'm curious why these aren't part of vtd_init(). vtd_init is where > > GCMD is set back to it's power-on state, which disables translation, so > > logically we should reset the address space at that point. Similarly, > > the root entry is reset, so it would make sense to throw away all the > > mappings there too. Thanks, > > vtd_init() is only called when realize() or reset, and AFAIU it's not > called by GCMD operations. However I think I get the point that > logically we should do similar things in e.g. vtd_handle_gcmd_srtp() > when the enable bit switches. > > My understanding is that if other things happened rather than the > system reboot (e.g., when root pointer is replaced, or during the > guest running the guest driver turns DMAR from on to off) the guest > will be responsible to do the rest of invalidations first before doing > that switch, so we'll possibly do the unmap_all() and address space > switches in other places (e.g., in vtd_context_global_invalidate, or > per device invalidations). AIUI, the entire global command register is write-once, so the guest cannot disable the IOMMU or change the root pointer after it's been initialized, except through a system reset. I think that means the guest can only operate through the invalidation queue at runtime. The bug being fixed here is that the IOMMU has been reset to its power-on state where translation is disabled, but the emulation of that disabled state also needs to return the per-device address space to that of system memory, or identity map thereof. The commit log seems to imply that there's some sort of SeaBIOS issue and we're just doing this to help the BIOS, when in reality, we just forgot to reset the per device address space and a subsequent boot of a guest that didn't enable the IOMMU would have the same sort of issues. In fact any usage of an assigned device prior to re-enabling the IOMMU would fail, there's nothing unique to NVMe here. Thanks, Alex
[Qemu-devel] [Bug 1790975] Re: Default arm virt machine broken
** Summary changed: - arm virt ecam pcie conflict + Default arm virt machine broken ** Description changed: This occurs on qemu_v3.0.0 but not on qemu_v2.12.2 (built from qemu_v3.0.0 tag on github) Symptom: You'll see something like this in the kernel output: [1.285210] OF: PCI: host bridge /pcie@1000 ranges: [1.286246] OF: PCI:IO 0x3eff..0x3eff -> 0x [1.287061] OF: PCI: MEM 0x1000..0x3efe -> 0x1000 [1.287820] OF: PCI: MEM 0x80..0xff -> 0x80 [1.289312] pci-host-generic 401000.pcie: can't claim ECAM area [mem 0x1000-0x1fff]: address conflict with /pcie@1000 [mem 0x1000-0x3efe] [1.290984] pci-host-generic: probe of 401000.pcie failed with error -16 - - Qemu Command Line: qemu-system-arm -machine virt -m 1024M -kernel zImage -serial stdio + Qemu Command Line: qemu-system-arm -machine virt -m 1024M -kernel zImage + -serial stdio I can post my zImage if anyone has problems reproducing with their own zImage. + + Note that this problem breaks pci making the machine unusable. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1790975 Title: Default arm virt machine broken Status in QEMU: New Bug description: This occurs on qemu_v3.0.0 but not on qemu_v2.12.2 (built from qemu_v3.0.0 tag on github) Symptom: You'll see something like this in the kernel output: [1.285210] OF: PCI: host bridge /pcie@1000 ranges: [1.286246] OF: PCI:IO 0x3eff..0x3eff -> 0x [1.287061] OF: PCI: MEM 0x1000..0x3efe -> 0x1000 [1.287820] OF: PCI: MEM 0x80..0xff -> 0x80 [1.289312] pci-host-generic 401000.pcie: can't claim ECAM area [mem 0x1000-0x1fff]: address conflict with /pcie@1000 [mem 0x1000-0x3efe] [1.290984] pci-host-generic: probe of 401000.pcie failed with error -16 Qemu Command Line: qemu-system-arm -machine virt -m 1024M -kernel zImage -serial stdio I can post my zImage if anyone has problems reproducing with their own zImage. Note that this problem breaks pci making the machine unusable. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1790975/+subscriptions
Re: [Qemu-devel] [PATCH v2] monitor: print message when using 'help' with an unknown command
On 08/08/2018 03:00 PM, Dr. David Alan Gilbert wrote: > * Collin Walling (wall...@linux.ibm.com) wrote: >> When typing 'help' followed by an unknown command, QEMU will >> not print anything to the command line to let the user know >> they typed a bad command. Let's fix this by printing a message >> to the monitor when this happens. For example: >> >> (qemu) help xyz >> unknown command: 'xyz' >> >> Reported-by: Stefan Zimmermann >> Signed-off-by: Collin Walling > > Reviewed-by: Dr. David Alan Gilbert > Thank you for the R-b. And pardon my impatience, but any chance this patch might get picked up, or does it require some more review first? [...] -- Respectfully, - Collin Walling
[Qemu-devel] [Bug 1790975] Re: arm virt ecam pcie conflict
I tried to triage this a bit today. I'm running a 32-bit linux kernel and I think that's the problem. The ECAM address base is at 0x401000, but it gets truncated to 0x1000 because it's only a 32-bit kernel, but since it's truncated, it conflicts with VIRT_PCIE_MMIO (see hw/arm/virt.c) whose range is from 0x1000 to 0x3efe which matches what we see in the error message. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1790975 Title: arm virt ecam pcie conflict Status in QEMU: New Bug description: This occurs on qemu_v3.0.0 but not on qemu_v2.12.2 (built from qemu_v3.0.0 tag on github) Symptom: You'll see something like this in the kernel output: [1.285210] OF: PCI: host bridge /pcie@1000 ranges: [1.286246] OF: PCI:IO 0x3eff..0x3eff -> 0x [1.287061] OF: PCI: MEM 0x1000..0x3efe -> 0x1000 [1.287820] OF: PCI: MEM 0x80..0xff -> 0x80 [1.289312] pci-host-generic 401000.pcie: can't claim ECAM area [mem 0x1000-0x1fff]: address conflict with /pcie@1000 [mem 0x1000-0x3efe] [1.290984] pci-host-generic: probe of 401000.pcie failed with error -16 Qemu Command Line: qemu-system-arm -machine virt -m 1024M -kernel zImage -serial stdio I can post my zImage if anyone has problems reproducing with their own zImage. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1790975/+subscriptions
Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface
* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
>
> On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
> wrote:
> >
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > Hi
> > >
> > > On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> > > wrote:
> > > >
> > > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > > > Hi
> > > > >
> > > > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov
> > > > > wrote:
> > > > > >
> > > > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > > > Marc-André Lureau wrote:
> > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > > > Marc-André Lureau wrote:
> > > > > > > >
> > > > > > > > > This allows to pass the last failing test from the Windows
> > > > > > > > > HLK TPM 2.0
> > > > > > > > > TCG PPI 1.3 tests.
> > > > > > > > >
> > > > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function".
> > > > > > > > > According
> > > > > > > > > to Laszlo, it's not so easy to implement in OVMF, he
> > > > > > > > > suggested to do
> > > > > > > > > it in qemu instead.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Marc-André Lureau
> > > > > > > > > ---
> > > > > > > > > hw/tpm/tpm_ppi.h | 2 ++
> > > > > > > > > hw/i386/acpi-build.c | 46
> > > > > > > > >
> > > > > > > > > hw/tpm/tpm_crb.c | 1 +
> > > > > > > > > hw/tpm/tpm_ppi.c | 23 ++
> > > > > > > > > hw/tpm/tpm_tis.c | 1 +
> > > > > > > > > docs/specs/tpm.txt | 2 ++
> > > > > > > > > hw/tpm/trace-events | 3 +++
> > > > > > > > > 7 files changed, 78 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > > > > bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > > >hwaddr addr, Object *obj, Error **errp);
> > > > > > > > >
> > > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > > > +
> > > > > > > > > #endif /* TPM_TPM_PPI_H */
> > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > > pprq = aml_name("PPRQ");
> > > > > > > > > pprm = aml_name("PPRM");
> > > > > > > > >
> > > > > > > > > +aml_append(dev,
> > > > > > > > > + aml_operation_region("TPP3",
> > > > > > > > > AML_SYSTEM_MEMORY,
> > > > > > > > > +
> > > > > > > > > aml_int(TPM_PPI_ADDR_BASE + 0x15a),
> > > > > > > > > +0x1));
> > > > > > > > > +field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK,
> > > > > > > > > AML_PRESERVE);
> > > > > > > > > +aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > > > +aml_append(dev, field);
> > > > > > > > > /*
> > > > > > > > > * DerefOf in Windows is broken with SYSTEM_MEMORY. Use
> > > > > > > > > a dynamic
> > > > > > > > > * operation region inside of a method for getting
> > > > > > > > > FUNC[op].
> > > > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > > aml_append(ifctx, aml_return(aml_buffer(1,
> > > > > > > > > zerobyte)));
> > > > > > > > > }
> > > > > > > > > aml_append(method, ifctx);
> > > > > > > > > +
> > > > > > > > > +ifctx = aml_if(
> > > > > > > > > +aml_equal(uuid,
> > > > > > > > > +
> > > > > > > > > aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > > > +{
> > > > > > > > > +/* standard DSM query function */
> > > > > > > > > +ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > > > +{
> > > > > > > > > +uint8_t byte_list[1] = { 0x03 };
> > > > > > > > > +aml_append(ifctx2, aml_return(aml_buffer(1,
> > > > > > > > > byte_list)));
> > > > > > > > > +}
> > > > > > > > > +aml_append(ifctx, ifctx2);
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * TCG Platform Reset Attack Mitigation
> > > > > > > > > Specification 1.0 Ch.6
> > > > > > > > > + *
> > > > > > > > > + * Arg 2 (Integer): Function Index = 1
> > > > > > > > > + * Arg 3 (Package): Arguments = Package: Type:
> > > > > > > > > Integer
> > > > > > > > > + *
Re: [Qemu-devel] [PATCH 10/10] vl: list user creatable propeties if '?' as argument
On 09/06/2018 11:27 AM, Marc-André Lureau wrote:
We use name=type (text) for devices properties, ex:
qemu-system-x86_64 -device tpm-tis,?
tpm-tis.tpmdev=str (ID of a tpm to use as a backend)
tpm-tis.irq=uint32
tpm-tis.tpm-tis-mmio[0]=child
But
qemu-img create -f qcow2 -o ?
size Virtual disk size
compat Compatibility level (0.10 or 1.1)
backing_file File name of a base image
I think I like more "name=type - text" form I introduced in "improve
qemu_opts_print_help() output". I guess I should change device
properties help for consistency then.
I don't have a strong preference for one form over the other, so much as
consistency in the various applications using the form.
+static gint
+pstrcmp(const char **a, const char **b)
+{
+return g_strcmp0(*a, *b);
+}
This is the second time your series has added this static helper. Should
it be a common helper instead?
as qemu_pstrcmp in cutils? inline in the header?
Does glib not already have such a helper? cutils sounds as good a place
as any, although inline may be at odds with typically using it as a
callback function (I don't know how well the compiler and linker handle
such a situation).
+g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
+for (i = 0; i < array->len; i++) {
+error_printf("%s\n", (char *)array->pdata[i]);
+}
+g_ptr_array_set_free_func(array, g_free);
+g_ptr_array_free(array, true);
+exit(0);
Again, printing to stderr then exiting with status 0 is awkward. Print
to stdout when successfully offering help text.
We use error_printf() for qdev list (qdev_device_help), which
redirects to monitor or stderr.
Should I also change ir for consistency? hopefully nobody relies on
the output going to stderr...
I don't worry too much about that. We've already fixed other places
where qemu binaries were antisocial, such as commit ac1307ab fixing
'qemu-img --help' to give status 0 instead of 1.
In general, when a user asks for help, the help text should go to
stdout, and the exit status should be 0 (unless you go to great lengths
to also detect write failures such as ENOSPC or EPIPE, in which case you
should ALSO attempt to write to stderr prior to exiting with nonzero
status that you couldn't output the help text - but since FILE* I/O can
cache things, detecting all possible write errors is not reliable unless
you check the result of fclose(stdout), which in turn has to be deferred
to the end of your program execution, generally via atexit(). The extra
complications and code maintenance for checking for --help output
failures is something that I'm personally okay with skipping).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v5 13/16] qapi/block-mirror: expose new job properties
On Thu, Sep 06, 2018 at 09:02:22AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 14 ++
> qapi/block-core.json | 30 --
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 98b91e75a7..429cdf9901 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3597,6 +3597,8 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> bool has_filter_node_name,
> const char *filter_node_name,
> bool has_copy_mode, MirrorCopyMode
> copy_mode,
> + bool has_auto_finalize, bool
> auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
> Error **errp)
> {
> int job_flags = JOB_DEFAULT;
> @@ -3625,6 +3627,12 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> if (!has_copy_mode) {
> copy_mode = MIRROR_COPY_MODE_BACKGROUND;
> }
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
>
> if (granularity != 0 && (granularity < 512 || granularity > 1048576 *
> 64)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> @@ -3802,6 +3810,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
> arg->has_unmap, arg->unmap,
> false, NULL,
> arg->has_copy_mode, arg->copy_mode,
> + arg->has_auto_finalize, arg->auto_finalize,
> + arg->has_auto_dismiss, arg->auto_dismiss,
> _err);
> bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> @@ -3823,6 +3833,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> bool has_filter_node_name,
> const char *filter_node_name,
> bool has_copy_mode, MirrorCopyMode copy_mode,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
> Error **errp)
> {
> BlockDriverState *bs;
> @@ -3856,6 +3868,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> true, true,
> has_filter_node_name, filter_node_name,
> has_copy_mode, copy_mode,
> + has_auto_finalize, auto_finalize,
> + has_auto_dismiss, auto_dismiss,
> _err);
> error_propagate(errp, local_err);
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d5b62e50d7..e785c2e9fe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1729,6 +1729,18 @@
> # @copy-mode: when to copy data to the destination; defaults to 'background'
> # (Since: 3.0)
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> # Since: 1.3
> ##
> { 'struct': 'DriveMirror',
> @@ -1738,7 +1750,8 @@
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> -'*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
> +'*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
> +'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
> # @BlockDirtyBitmap:
> @@ -2004,6 +2017,18 @@
> # @copy-mode: when to copy data to the destination; defaults to 'background'
> # (Since: 3.0)
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +#
Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions
On 06/09/18 17:40, Thomas Huth wrote: > On 2018-09-06 16:50, Peter Maydell wrote: >> On 6 September 2018 at 13:02, Thomas Huth wrote: >>> I somehow fail to see that something outside of lsi53c895a.c should >>> really need to access the internals of LSIState. If there is something >>> that needs to be configured from the outside, it should be done via QOM >>> properties instead, shouldn't it? So I think I'd rather prefer if you >>> could do it the other way round and change the lsi*_create() functions >>> to return a pointer to PCIDevice instead, if possible. >> >> Nothing typically does, but the "modern" style of having QOM objects which >> use other QOM objects do so by embedding the child object's struct into >> the struct of the parent requires that the struct definition is available. > > But in this case we don't have anything that "inherits" from LSIState, > so shouldn't we rather follow the "information hiding" principle and > keep the state local to the lsi53c895a.c file? If you want to use a > "LSIState *" from another .c file, you can still put an "anonymous" > > struct LSIState; > typedef struct LSIState LSIState; > > in a header somewhere without revealing the implementation. > > I'm fine with putting the whole LSIState into a header file if we really > need it, but in this case, I don't see the point. I completely agree with you that struct members used to configure the device initialisation should only be done via qdev properties, however having the struct information and the QOM defines available is very, very handy. In terms of information hiding we are a long way away from this, and in my experience having both the compile time and runtime checks on QOM macros means that just doing a FOO(pci_create_simple...) or strongly typing object links when wiring up a board catches just about all errors developers can make. Amusingly the main reason I need to expose the LSIState at all is to be able to call scsi_bus_legacy_handle_cmdline() on the SCSI bus object itself. I guess you could say that this is an argument in favour of the existing approach, but then you're effectively moving back to the equivalent of _init() functions for this one particular case which these days are considered to be bad. My feeling is that since the pattern of a separate header with struct and QOM macros (or "modern" style) is used throughout the rest of the codebase, why should we make an exception for this one particular case? I see Peter also mentions about marking members as public/private, but again in my opinion and experience, with the addition of runtime as well as compile time QOM casts a huge class of problems has now gone away - and as a result of this, there are now more urgent problems which is probably why no-one has really picked up Peter's patch in over 4 years. ATB, Mark.
Re: [Qemu-devel] [PATCH v5 15/16] block/backup: qapi documentation fixup
On Thu, Sep 06, 2018 at 09:02:24AM -0400, John Snow wrote: > Fix documentation to match the other jobs amended for 3.1. > > Signed-off-by: John Snow > Reviewed-by: Max Reitz Reviewed-by: Jeff Cody > --- > qapi/block-core.json | 18 ++ > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index f877e9e414..c0b3d33dbb 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1272,13 +1272,14 @@ > # a different block device than @device). > # > # @auto-finalize: When false, this job will wait in a PENDING state after it > has > -# finished its work, waiting for @block-job-finalize. > -# When true, this job will automatically perform its abort or > -# commit actions. > +# finished its work, waiting for @block-job-finalize before > +# making any block graph changes. > +# When true, this job will automatically > +# perform its abort or commit actions. > # Defaults to true. (Since 2.12) > # > # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it > -#has completed ceased all work, and wait for > @block-job-dismiss. > +#has completely ceased all work, and awaits > @block-job-dismiss. > #When true, this job will automatically disappear from the > query > #list without user intervention. > #Defaults to true. (Since 2.12) > @@ -1327,13 +1328,14 @@ > # a different block device than @device). > # > # @auto-finalize: When false, this job will wait in a PENDING state after it > has > -# finished its work, waiting for @block-job-finalize. > -# When true, this job will automatically perform its abort or > -# commit actions. > +# finished its work, waiting for @block-job-finalize before > +# making any block graph changes. > +# When true, this job will automatically > +# perform its abort or commit actions. > # Defaults to true. (Since 2.12) > # > # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it > -#has completed ceased all work, and wait for > @block-job-dismiss. > +#has completely ceased all work, and awaits > @block-job-dismiss. > #When true, this job will automatically disappear from the > query > #list without user intervention. > #Defaults to true. (Since 2.12) > -- > 2.14.4 >
Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions
On 6 September 2018 at 17:40, Thomas Huth wrote:
> On 2018-09-06 16:50, Peter Maydell wrote:
>> Nothing typically does, but the "modern" style of having QOM objects which
>> use other QOM objects do so by embedding the child object's struct into
>> the struct of the parent requires that the struct definition is available.
>
> But in this case we don't have anything that "inherits" from LSIState,
> so shouldn't we rather follow the "information hiding" principle and
> keep the state local to the lsi53c895a.c file? If you want to use a
> "LSIState *" from another .c file, you can still put an "anonymous"
>
> struct LSIState;
> typedef struct LSIState LSIState;
This doesn't work for
typedef struct MySoC {
MyUART uart;
LSIState scsi;
...
} MySoC;
This isn't inheritance, it's just use ("has-a", not "is-a").
> in a header somewhere without revealing the implementation.
>
> I'm fine with putting the whole LSIState into a header file if we really
> need it, but in this case, I don't see the point.
Looking at the rest of the series there doesn't seem to be
any code that wants to do use-by-embedding, so we can
certainly postpone moving the struct into the header file
until we need it.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v5 14/16] qapi/block-stream: expose new job properties
On Thu, Sep 06, 2018 at 09:02:23AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 9 +
> hmp.c| 5 +++--
> qapi/block-core.json | 16 +++-
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 429cdf9901..0cf8febe6c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3116,6 +3116,8 @@ void qmp_block_stream(bool has_job_id, const char
> *job_id, const char *device,
>bool has_backing_file, const char *backing_file,
>bool has_speed, int64_t speed,
>bool has_on_error, BlockdevOnError on_error,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
>Error **errp)
> {
> BlockDriverState *bs, *iter;
> @@ -3185,6 +3187,13 @@ void qmp_block_stream(bool has_job_id, const char
> *job_id, const char *device,
> /* backing_file string overrides base bs filename */
> base_name = has_backing_file ? backing_file : base_name;
>
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
> +
> stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> job_flags, has_speed ? speed : 0, on_error, _err);
> if (local_err) {
> diff --git a/hmp.c b/hmp.c
> index 4975fa56b0..868c1a049d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1905,8 +1905,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
> int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>
> qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> - false, NULL, qdict_haskey(qdict, "speed"), speed,
> - true, BLOCKDEV_ON_ERROR_REPORT, );
> + false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> + BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> + );
>
> hmp_handle_error(mon, );
> }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e785c2e9fe..f877e9e414 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2317,6 +2317,19 @@
> #'stop' and 'enospc' can only be used if the block device
> #supports io-status (see BlockInfo). Since 1.3.
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> +#
> # Returns: Nothing on success. If @device does not exist, DeviceNotFound.
> #
> # Since: 1.1
> @@ -2332,7 +2345,8 @@
> { 'command': 'block-stream',
>'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
> '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> -'*on-error': 'BlockdevOnError' } }
> +'*on-error': 'BlockdevOnError',
> +'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
> # @block-job-set-speed:
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings
On Thu, Sep 06, 2018 at 09:02:25AM -0400, John Snow wrote:
> Presently only the backup job really guarantees what one would consider
> transactional semantics. To guard against someone helpfully adding them
> in the future, document that there are shortcomings in the model that
> would need to be audited at that time.
>
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0cf8febe6c..d4b42403df 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2182,7 +2182,13 @@ static const BlkActionOps actions[] = {
> .instance_size = sizeof(BlockDirtyBitmapState),
> .prepare = block_dirty_bitmap_disable_prepare,
> .abort = block_dirty_bitmap_disable_abort,
> - }
> +},
> +/* Where are transactions for MIRROR, COMMIT and STREAM?
> + * Although these blockjobs use transaction callbacks like the backup
> job,
> + * these jobs do not necessarily adhere to transaction semantics.
> + * These jobs may not fully undo all of their actions on abort, nor do
> they
> + * necessarily work in transactions with more than one job in them.
> + */
> };
>
> /**
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 12/16] qapi/block-commit: expose new job properties
On Thu, Sep 06, 2018 at 09:02:21AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 8
> qapi/block-core.json | 16 +++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index ec90eb1cf9..98b91e75a7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3204,6 +3204,8 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
>bool has_backing_file, const char *backing_file,
>bool has_speed, int64_t speed,
>bool has_filter_node_name, const char
> *filter_node_name,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
>Error **errp)
> {
> BlockDriverState *bs;
> @@ -3223,6 +3225,12 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> if (!has_filter_node_name) {
> filter_node_name = NULL;
> }
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
>
> /* Important Note:
> * libvirt relies on the DeviceNotFound error class in order to probe
> for
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4c7a37afdc..d5b62e50d7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1498,6 +1498,19 @@
> #above @top. If this option is not given, a node name is
> #autogenerated. (Since: 2.9)
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> +#
> # Returns: Nothing on success
> # If @device does not exist, DeviceNotFound
> # Any other error returns a GenericError.
> @@ -1515,7 +1528,8 @@
> { 'command': 'block-commit',
>'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
> '*backing-file': 'str', '*speed': 'int',
> -'*filter-node-name': 'str' } }
> +'*filter-node-name': 'str',
> +'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
> # @drive-backup:
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 09/16] tests/test-blockjob: remove exit callback
On Thu, Sep 06, 2018 at 09:02:18AM -0400, John Snow wrote:
> We remove the exit callback and the completed boolean along with it.
> We can simulate it just fine by waiting for the job to defer to the
> main loop, and then giving it one final kick to get the main loop
> portion to run.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 8e8b680416..de4c1c20aa 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -160,15 +160,8 @@ typedef struct CancelJob {
> BlockBackend *blk;
> bool should_converge;
> bool should_complete;
> -bool completed;
> } CancelJob;
>
> -static void cancel_job_exit(Job *job)
> -{
> -CancelJob *s = container_of(job, CancelJob, common.job);
> -s->completed = true;
> -}
> -
> static void cancel_job_complete(Job *job, Error **errp)
> {
> CancelJob *s = container_of(job, CancelJob, common.job);
> @@ -201,7 +194,6 @@ static const BlockJobDriver test_cancel_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .run = cancel_job_run,
> -.exit = cancel_job_exit,
> .complete = cancel_job_complete,
> },
> };
> @@ -335,9 +327,11 @@ static void test_cancel_pending(void)
>
> job_complete(job, _abort);
> job_enter(job);
> -while (!s->completed) {
> +while (!job->deferred_to_main_loop) {
> aio_poll(qemu_get_aio_context(), true);
> }
> +assert(job->status == JOB_STATUS_READY);
> +aio_poll(qemu_get_aio_context(), true);
> assert(job->status == JOB_STATUS_PENDING);
>
> cancel_common(s);
> @@ -359,9 +353,11 @@ static void test_cancel_concluded(void)
>
> job_complete(job, _abort);
> job_enter(job);
> -while (!s->completed) {
> +while (!job->deferred_to_main_loop) {
> aio_poll(qemu_get_aio_context(), true);
> }
> +assert(job->status == JOB_STATUS_READY);
> +aio_poll(qemu_get_aio_context(), true);
> assert(job->status == JOB_STATUS_PENDING);
>
> job_finalize(job, _abort);
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 11/16] jobs: remove .exit callback
On Thu, Sep 06, 2018 at 09:02:20AM -0400, John Snow wrote:
> Now that all of the jobs use the component finalization callbacks,
> there's no use for the heavy-hammer .exit callback anymore.
>
> job_exit becomes a glorified type shim so that we can call
> job_completed from aio_bh_schedule_oneshot.
>
> Move these three functions down into job.c to eliminate a
> forward reference.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> include/qemu/job.h | 11
> job.c | 77
> --
> 2 files changed, 34 insertions(+), 54 deletions(-)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index e0cff702b7..5cb0681834 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -221,17 +221,6 @@ struct JobDriver {
> */
> void (*drain)(Job *job);
>
> -/**
> - * If the callback is not NULL, exit will be invoked from the main thread
> - * when the job's coroutine has finished, but before transactional
> - * convergence; before @prepare or @abort.
> - *
> - * FIXME TODO: This callback is only temporary to transition remaining
> jobs
> - * to prepare/commit/abort/clean callbacks and will be removed before
> 3.1.
> - * is released.
> - */
> -void (*exit)(Job *job);
> -
> /**
> * If the callback is not NULL, prepare will be invoked when all the jobs
> * belonging to the same transaction complete; or upon this job's
> completion
> diff --git a/job.c b/job.c
> index 01dd97fee3..72f7de1f36 100644
> --- a/job.c
> +++ b/job.c
> @@ -535,49 +535,6 @@ void job_drain(Job *job)
> }
> }
>
> -static void job_completed(Job *job);
> -
> -static void job_exit(void *opaque)
> -{
> -Job *job = (Job *)opaque;
> -AioContext *aio_context = job->aio_context;
> -
> -if (job->driver->exit) {
> -aio_context_acquire(aio_context);
> -job->driver->exit(job);
> -aio_context_release(aio_context);
> -}
> -job_completed(job);
> -}
> -
> -/**
> - * All jobs must allow a pause point before entering their job proper. This
> - * ensures that jobs can be paused prior to being started, then resumed
> later.
> - */
> -static void coroutine_fn job_co_entry(void *opaque)
> -{
> -Job *job = opaque;
> -
> -assert(job && job->driver && job->driver->run);
> -job_pause_point(job);
> -job->ret = job->driver->run(job, >err);
> -job->deferred_to_main_loop = true;
> -aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> -}
> -
> -
> -void job_start(Job *job)
> -{
> -assert(job && !job_started(job) && job->paused &&
> - job->driver && job->driver->run);
> -job->co = qemu_coroutine_create(job_co_entry, job);
> -job->pause_count--;
> -job->busy = true;
> -job->paused = false;
> -job_state_transition(job, JOB_STATUS_RUNNING);
> -aio_co_enter(job->aio_context, job->co);
> -}
> -
> /* Assumes the block_job_mutex is held */
> static bool job_timer_not_pending(Job *job)
> {
> @@ -894,6 +851,40 @@ static void job_completed(Job *job)
> }
> }
>
> +/** Useful only as a type shim for aio_bh_schedule_oneshot. */
> +static void job_exit(void *opaque)
> +{
> +Job *job = (Job *)opaque;
> +job_completed(job);
> +}
> +
> +/**
> + * All jobs must allow a pause point before entering their job proper. This
> + * ensures that jobs can be paused prior to being started, then resumed
> later.
> + */
> +static void coroutine_fn job_co_entry(void *opaque)
> +{
> +Job *job = opaque;
> +
> +assert(job && job->driver && job->driver->run);
> +job_pause_point(job);
> +job->ret = job->driver->run(job, >err);
> +job->deferred_to_main_loop = true;
> +aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> +}
> +
> +void job_start(Job *job)
> +{
> +assert(job && !job_started(job) && job->paused &&
> + job->driver && job->driver->run);
> +job->co = qemu_coroutine_create(job_co_entry, job);
> +job->pause_count--;
> +job->busy = true;
> +job->paused = false;
> +job_state_transition(job, JOB_STATUS_RUNNING);
> +aio_co_enter(job->aio_context, job->co);
> +}
> +
> void job_cancel(Job *job, bool force)
> {
> if (job->status == JOB_STATUS_CONCLUDED) {
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean
On Thu, Sep 06, 2018 at 09:02:19AM -0400, John Snow wrote:
> The exit callback in this test actually only performs cleanup.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob-txn.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index ef29f35e44..86606f92b3 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -24,7 +24,7 @@ typedef struct {
> int *result;
> } TestBlockJob;
>
> -static void test_block_job_exit(Job *job)
> +static void test_block_job_clean(Job *job)
> {
> BlockJob *bjob = container_of(job, BlockJob, job);
> BlockDriverState *bs = blk_bs(bjob->blk);
> @@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .run = test_block_job_run,
> -.exit = test_block_job_exit,
> +.clean = test_block_job_clean,
> },
> };
>
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 08/16] tests/blockjob: replace Blockjob with Job
On Thu, Sep 06, 2018 at 09:02:17AM -0400, John Snow wrote:
> These tests don't actually test blockjobs anymore, they test
> generic Job lifetimes. Change the types accordingly.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob.c | 98
> ++-
> 1 file changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index ad4a65bc78..8e8b680416 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -206,18 +206,20 @@ static const BlockJobDriver test_cancel_driver = {
> },
> };
>
> -static CancelJob *create_common(BlockJob **pjob)
> +static CancelJob *create_common(Job **pjob)
> {
> BlockBackend *blk;
> -BlockJob *job;
> +Job *job;
> +BlockJob *bjob;
> CancelJob *s;
>
> blk = create_blk(NULL);
> -job = mk_job(blk, "Steve", _cancel_driver, true,
> - JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> -job_ref(>job);
> -assert(job->job.status == JOB_STATUS_CREATED);
> -s = container_of(job, CancelJob, common);
> +bjob = mk_job(blk, "Steve", _cancel_driver, true,
> + JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> +job = >job;
> +job_ref(job);
> +assert(job->status == JOB_STATUS_CREATED);
> +s = container_of(bjob, CancelJob, common);
> s->blk = blk;
>
> *pjob = job;
> @@ -242,7 +244,7 @@ static void cancel_common(CancelJob *s)
>
> static void test_cancel_created(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
> @@ -251,119 +253,119 @@ static void test_cancel_created(void)
>
> static void test_cancel_running(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> cancel_common(s);
> }
>
> static void test_cancel_paused(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> -job_user_pause(>job, _abort);
> -job_enter(>job);
> -assert(job->job.status == JOB_STATUS_PAUSED);
> +job_user_pause(job, _abort);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_PAUSED);
>
> cancel_common(s);
> }
>
> static void test_cancel_ready(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> s->should_converge = true;
> -job_enter(>job);
> -assert(job->job.status == JOB_STATUS_READY);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_READY);
>
> cancel_common(s);
> }
>
> static void test_cancel_standby(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> s->should_converge = true;
> -job_enter(>job);
> -assert(job->job.status == JOB_STATUS_READY);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_READY);
>
> -job_user_pause(>job, _abort);
> -job_enter(>job);
> -assert(job->job.status == JOB_STATUS_STANDBY);
> +job_user_pause(job, _abort);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_STANDBY);
>
> cancel_common(s);
> }
>
> static void test_cancel_pending(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> s->should_converge = true;
> -job_enter(>job);
> -assert(job->job.status == JOB_STATUS_READY);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_READY);
>
> -job_complete(>job, _abort);
> -job_enter(>job);
> +job_complete(job, _abort);
> +job_enter(job);
> while (!s->completed) {
> aio_poll(qemu_get_aio_context(), true);
> }
> -assert(job->job.status == JOB_STATUS_PENDING);
> +assert(job->status == JOB_STATUS_PENDING);
>
> cancel_common(s);
> }
>
> static void test_cancel_concluded(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common();
>
> -job_start(>job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status ==
Re: [Qemu-devel] [PATCH v5 07/16] block/stream: refactor stream to use job callbacks
On Thu, Sep 06, 2018 at 09:02:16AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/stream.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 700eb239e4..81a7ec8ece 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
> return blk_co_preadv(blk, offset, qiov.size, ,
> BDRV_REQ_COPY_ON_READ);
> }
>
> -static void stream_exit(Job *job)
> +static int stream_prepare(Job *job)
> {
> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> BlockJob *bjob = >common;
> BlockDriverState *bs = blk_bs(bjob->blk);
> BlockDriverState *base = s->base;
> Error *local_err = NULL;
> -int ret = job->ret;
> +int ret = 0;
>
> -if (!job_is_cancelled(job) && bs->backing && ret == 0) {
> +if (bs->backing) {
> const char *base_id = NULL, *base_fmt = NULL;
> if (base) {
> base_id = s->backing_file_str;
> @@ -75,12 +75,19 @@ static void stream_exit(Job *job)
> bdrv_set_backing_hd(bs, base, _err);
> if (local_err) {
> error_report_err(local_err);
> -ret = -EPERM;
> -goto out;
> +return -EPERM;
> }
> }
>
> -out:
> +return ret;
> +}
> +
> +static void stream_clean(Job *job)
> +{
> +StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> +BlockJob *bjob = >common;
> +BlockDriverState *bs = blk_bs(bjob->blk);
> +
> /* Reopen the image back in read-only mode if necessary */
> if (s->bs_flags != bdrv_get_flags(bs)) {
> /* Give up write permissions before making it read-only */
> @@ -89,7 +96,6 @@ out:
> }
>
> g_free(s->backing_file_str);
> -job->ret = ret;
> }
>
> static int coroutine_fn stream_run(Job *job, Error **errp)
> @@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
> .job_type = JOB_TYPE_STREAM,
> .free = block_job_free,
> .run = stream_run,
> -.exit = stream_exit,
> +.prepare = stream_prepare,
> +.clean = stream_clean,
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> },
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor
On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
> For purposes of minimum code movement, refactor the mirror_exit
> callback to use the post-finalization callbacks in a trivial way.
>
> Signed-off-by: John Snow
> ---
> block/mirror.c | 39 ---
> 1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index bd3e908710..a92b4702c5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
> int max_iov;
> bool initial_zeroing_ongoing;
> int in_active_write_counter;
> +bool prepared;
> } MirrorBlockJob;
>
> typedef struct MirrorBDSOpaque {
> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
> }
> }
>
> -static void mirror_exit(Job *job)
> +static int mirror_exit_common(Job *job)
> {
> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
> BlockJob *bjob = >common;
> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
> BlockDriverState *target_bs = blk_bs(s->target);
> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> Error *local_err = NULL;
> -int ret = job->ret;
> +bool abort = job->ret < 0;
> +int ret = 0;
> +
> +if (s->prepared) {
> +return 0;
> +}
> +s->prepared = true;
>
> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>
> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
> * required before it could become a backing file of target_bs. */
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> _abort);
> -if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> BlockDriverState *backing = s->is_none_mode ? src : s->base;
> if (backing_bs(target_bs) != backing) {
> bdrv_set_backing_hd(target_bs, backing, _err);
> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
> aio_context_acquire(replace_aio_context);
> }
>
> -if (s->should_complete && ret == 0) {
> -BlockDriverState *to_replace = src;
> -if (s->to_replace) {
> -to_replace = s->to_replace;
> -}
> +if (s->should_complete && !abort) {
> +BlockDriverState *to_replace = s->to_replace ?: src;
>
> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
> bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
> bdrv_unref(mirror_top_bs);
> bdrv_unref(src);
>
> -job->ret = ret;
> +return ret;
> +}
> +
> +static int mirror_prepare(Job *job)
> +{
> +return mirror_exit_common(job);
> +}
> +
> +static void mirror_abort(Job *job)
> +{
> +int ret = mirror_exit_common(job);
> +assert(ret == 0);
Not something to hold the series up, but in case a v6 is called for due to
other changes: I think it may be worth a comment in mirror_exit_common()
that if abort is true, and we don't return success, QEMU will hit an assert.
Mainly to prevent someone from including a call with a potential error
return in the abort path in the future.
Reviewed-by: Jeff Cody
> }
>
> static void mirror_throttle(MirrorBlockJob *s)
> @@ -1132,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> -.exit = mirror_exit,
> +.prepare= mirror_prepare,
> +.abort = mirror_abort,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> @@ -1149,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> -.exit = mirror_exit,
> +.prepare= mirror_prepare,
> +.abort = mirror_abort,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v10 6/6] tpm: add ACPI memory clear interface
Hi
On Thu, Sep 6, 2018 at 1:42 PM Dr. David Alan Gilbert
wrote:
>
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > Hi
> >
> > On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> > wrote:
> > >
> > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > > Hi
> > > >
> > > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov
> > > > wrote:
> > > > >
> > > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > > Marc-André Lureau wrote:
> > > > >
> > > > > > Hi
> > > > > >
> > > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov
> > > > > > wrote:
> > > > > > >
> > > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > > Marc-André Lureau wrote:
> > > > > > >
> > > > > > > > This allows to pass the last failing test from the Windows HLK
> > > > > > > > TPM 2.0
> > > > > > > > TCG PPI 1.3 tests.
> > > > > > > >
> > > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function".
> > > > > > > > According
> > > > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested
> > > > > > > > to do
> > > > > > > > it in qemu instead.
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau
> > > > > > > > ---
> > > > > > > > hw/tpm/tpm_ppi.h | 2 ++
> > > > > > > > hw/i386/acpi-build.c | 46
> > > > > > > >
> > > > > > > > hw/tpm/tpm_crb.c | 1 +
> > > > > > > > hw/tpm/tpm_ppi.c | 23 ++
> > > > > > > > hw/tpm/tpm_tis.c | 1 +
> > > > > > > > docs/specs/tpm.txt | 2 ++
> > > > > > > > hw/tpm/trace-events | 3 +++
> > > > > > > > 7 files changed, 78 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > > > bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > >hwaddr addr, Object *obj, Error **errp);
> > > > > > > >
> > > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > > +
> > > > > > > > #endif /* TPM_TPM_PPI_H */
> > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > pprq = aml_name("PPRQ");
> > > > > > > > pprm = aml_name("PPRM");
> > > > > > > >
> > > > > > > > +aml_append(dev,
> > > > > > > > + aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > > > +aml_int(TPM_PPI_ADDR_BASE
> > > > > > > > + 0x15a),
> > > > > > > > +0x1));
> > > > > > > > +field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK,
> > > > > > > > AML_PRESERVE);
> > > > > > > > +aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > > +aml_append(dev, field);
> > > > > > > > /*
> > > > > > > > * DerefOf in Windows is broken with SYSTEM_MEMORY. Use a
> > > > > > > > dynamic
> > > > > > > > * operation region inside of a method for getting
> > > > > > > > FUNC[op].
> > > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > > aml_append(ifctx, aml_return(aml_buffer(1,
> > > > > > > > zerobyte)));
> > > > > > > > }
> > > > > > > > aml_append(method, ifctx);
> > > > > > > > +
> > > > > > > > +ifctx = aml_if(
> > > > > > > > +aml_equal(uuid,
> > > > > > > > +
> > > > > > > > aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > > +{
> > > > > > > > +/* standard DSM query function */
> > > > > > > > +ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > > +{
> > > > > > > > +uint8_t byte_list[1] = { 0x03 };
> > > > > > > > +aml_append(ifctx2, aml_return(aml_buffer(1,
> > > > > > > > byte_list)));
> > > > > > > > +}
> > > > > > > > +aml_append(ifctx, ifctx2);
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * TCG Platform Reset Attack Mitigation
> > > > > > > > Specification 1.0 Ch.6
> > > > > > > > + *
> > > > > > > > + * Arg 2 (Integer): Function Index = 1
> > > > > > > > + * Arg 3 (Package): Arguments = Package: Type:
> > > > > > > > Integer
> > > > > > > > + * Operation Value of the Request
> > > > > > > > + * Returns: Type: Integer
> > > > > > > > + * 0: Success
> > > > > > > > + * 1: General Failure
> > > > > > > > + */
> > > > > > > > +ifctx2 =
Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions
On 2018-09-06 16:50, Peter Maydell wrote: > On 6 September 2018 at 13:02, Thomas Huth wrote: >> I somehow fail to see that something outside of lsi53c895a.c should >> really need to access the internals of LSIState. If there is something >> that needs to be configured from the outside, it should be done via QOM >> properties instead, shouldn't it? So I think I'd rather prefer if you >> could do it the other way round and change the lsi*_create() functions >> to return a pointer to PCIDevice instead, if possible. > > Nothing typically does, but the "modern" style of having QOM objects which > use other QOM objects do so by embedding the child object's struct into > the struct of the parent requires that the struct definition is available. But in this case we don't have anything that "inherits" from LSIState, so shouldn't we rather follow the "information hiding" principle and keep the state local to the lsi53c895a.c file? If you want to use a "LSIState *" from another .c file, you can still put an "anonymous" struct LSIState; typedef struct LSIState LSIState; in a header somewhere without revealing the implementation. I'm fine with putting the whole LSIState into a header file if we really need it, but in this case, I don't see the point. Thomas
Re: [Qemu-devel] [PATCH 10/10] vl: list user creatable propeties if '?' as argument
Hi
On Thu, Sep 6, 2018 at 7:40 PM Eric Blake wrote:
>
> On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
>
> Subject has typo and awkward grammar; I'd suggest:
>
> vl: list user creatable properties when 'help' is argument
>
> > Iterate over the writable class properties, sort and print them out
> > with the description if available.
> >
> > Ex: qemu -object memory-backend-memfd,?
>
> As elsewhere in the series, s/\?/help/
>
> > memory-backend-memfd.dump=bool (Set to 'off' to exclude from core dump)
> > memory-backend-memfd.host-nodes=int (Binds memory to the list of NUMA host
> > nodes)
> > memory-backend-memfd.hugetlb=bool (Use huge pages)
> > memory-backend-memfd.hugetlbsize=int (Huge pages size (ex: 2M, 1G))
> > memory-backend-memfd.merge=bool (Mark memory as mergeable)
> > memory-backend-memfd.policy=HostMemPolicy (Set the NUMA policy)
> > memory-backend-memfd.prealloc=bool (Preallocate memory)
> > memory-backend-memfd.seal=bool (Seal growing & shrinking)
> > memory-backend-memfd.share=bool (Mark the memory as private to QEMU or
> > shared)
> > memory-backend-memfd.size=int (Size of the memory region (ex: 500M))
>
> Why "name=type (text)" here, but "name=type - text" in 2/10?
> Consistency would be nice.
We use name=type (text) for devices properties, ex:
qemu-system-x86_64 -device tpm-tis,?
tpm-tis.tpmdev=str (ID of a tpm to use as a backend)
tpm-tis.irq=uint32
tpm-tis.tpm-tis-mmio[0]=child
But
qemu-img create -f qcow2 -o ?
size Virtual disk size
compat Compatibility level (0.10 or 1.1)
backing_file File name of a base image
I think I like more "name=type - text" form I introduced in "improve
qemu_opts_print_help() output". I guess I should change device
properties help for consistency then.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > qom/object_interfaces.c | 6 +++---
> > vl.c| 45 ++---
> > 2 files changed, 45 insertions(+), 6 deletions(-)
> >
>
> > +++ b/vl.c
> > @@ -2721,6 +2721,11 @@ static int machine_set_property(void *opaque,
> > return 0;
> > }
> >
> > +static gint
> > +pstrcmp(const char **a, const char **b)
> > +{
> > +return g_strcmp0(*a, *b);
> > +}
>
> This is the second time your series has added this static helper. Should
> it be a common helper instead?
as qemu_pstrcmp in cutils? inline in the header?
>
>
> > +g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
> > +for (i = 0; i < array->len; i++) {
> > +error_printf("%s\n", (char *)array->pdata[i]);
> > +}
> > +g_ptr_array_set_free_func(array, g_free);
> > +g_ptr_array_free(array, true);
> > +exit(0);
>
> Again, printing to stderr then exiting with status 0 is awkward. Print
> to stdout when successfully offering help text.
We use error_printf() for qdev list (qdev_device_help), which
redirects to monitor or stderr.
Should I also change ir for consistency? hopefully nobody relies on
the output going to stderr...
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
Marc-André Lureau
Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross compilation in generating boot header file
- Original Message -
> From: "Andrew Jones"
> To: "Wei Huang"
> Cc: lviv...@redhat.com, "peter maydell" ,
> quint...@redhat.com, qemu-devel@nongnu.org,
> dgilb...@redhat.com, "alex bennee"
> Sent: Thursday, September 6, 2018 9:00:33 AM
> Subject: Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross
> compilation in generating boot header file
>
> On Thu, Sep 06, 2018 at 09:37:04AM -0400, Wei Huang wrote:
> >
> >
> > - Original Message -
> > > From: "Andrew Jones"
> > > To: "Wei Huang"
> > > Cc: qemu-devel@nongnu.org, lviv...@redhat.com, "peter maydell"
> > > , quint...@redhat.com,
> > > dgilb...@redhat.com, "alex bennee"
> > > Sent: Thursday, September 6, 2018 7:03:32 AM
> > > Subject: Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross
> > > compilation in generating boot header file
> > >
> > > On Wed, Sep 05, 2018 at 03:15:32PM -0400, Wei Huang wrote:
> > > > Recently a new configure option, CROSS_CC_GUEST, was added to
> > > > $(TARGET)-softmmu/config-target.mak to support TCG-related tests. This
> > > > patch tries to leverage this option to support cross compilation when
> > > > the
> > > > migration boot block file is being re-generated:
> > > >
> > > > * The x86 related files are moved to a new sub-dir (named ./i386).
> > > > * A new top-layer Makefile is created in tests/migration/ directory.
> > > >This Makefile searches and parses CROSS_CC_GUEST to generate
> > > >CROSS_PREFIX.
> > > >The CROSS_PREFIX, if available, is then passed to
> > > >migration/$ARCH/Makefile.
> > > >
> > > > Reviewed-by: Juan Quintela
> > > > Signed-off-by: Wei Huang
> > > > ---
> > > > tests/migration-test.c | 2 +-
> > > > tests/migration/Makefile | 44
> > > > --
> > > > tests/migration/i386/Makefile | 22 +++
> > > > .../{x86-a-b-bootblock.S => i386/a-b-bootblock.S} | 4 --
> > > > .../{x86-a-b-bootblock.h => i386/a-b-bootblock.h} | 8 ++--
> > > > 5 files changed, 51 insertions(+), 29 deletions(-)
> > > > create mode 100644 tests/migration/i386/Makefile
> > > > rename tests/migration/{x86-a-b-bootblock.S => i386/a-b-bootblock.S}
> > > > (93%)
> > > > rename tests/migration/{x86-a-b-bootblock.h => i386/a-b-bootblock.h}
> > > > (92%)
> > > >
> > > > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > > > index 0e687b7..fe6b41a 100644
> > > > --- a/tests/migration-test.c
> > > > +++ b/tests/migration-test.c
> > > > @@ -83,7 +83,7 @@ static const char *tmpfs;
> > > > /* A simple PC boot sector that modifies memory (1-100MB) quickly
> > > > * outputting a 'B' every so often if it's still running.
> > > > */
> > > > -#include "tests/migration/x86-a-b-bootblock.h"
> > > > +#include "tests/migration/i386/a-b-bootblock.h"
> > > >
> > > > static void init_bootfile_x86(const char *bootpath)
> > > > {
> > > > diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> > > > index c0824b4..a9ed875 100644
> > > > --- a/tests/migration/Makefile
> > > > +++ b/tests/migration/Makefile
> > > > @@ -1,31 +1,35 @@
> > > > -# To specify cross compiler prefix, use CROSS_PREFIX=
> > > > -# $ make CROSS_PREFIX=x86_64-linux-gnu-
> > > > +#
> > > > +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> > > > +#
> > > > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > > > later.
> > > > +# See the COPYING file in the top-level directory.
> > > > +#
> > > > +
> > > > +TARGET_LIST = i386
> > > > +
> > > > +SRC_PATH = ../..
> > > >
> > > > override define __note
> > > > -/* This file is automatically generated from
> > > > - * tests/migration/x86-a-b-bootblock.S, edit that and then run
> > > > - * tests/migration/rebuild-x86-bootblock.sh to update,
> > > > - * and then remember to send both in your patch submission.
> > > > +/* This file is automatically generated from the assembly file in
> > > > + * tests/migration/$@. Edit that file and then run "make all"
> > > > + * inside tests/migration to update, and then remember to send both
> > > > + * the header and the assembler differences in your patch submission.
> > > > */
> > > > endef
> > > > export __note
> > > >
> > > > -.PHONY: all clean
> > > > -all: x86-a-b-bootblock.h
> > > > -
> > > > -x86-a-b-bootblock.h: x86.bootsect
> > > > - echo "$$__note" > header.tmp
> > > > - xxd -i $< | sed -e 's/.*int.*//' >> header.tmp
> > > > - mv header.tmp $@
> > > > +find-arch-cross-cc = $(lastword $(shell grep -h "CROSS_CC_GUEST="
> > > > $(wildcard $(SRC_PATH)/$(patsubst
> > > > i386,*86*,$(1))-softmmu/config-target.mak)))
> > >
> > > The above function hangs unless configuring with
> > > '--target-list=x86_64-softmmu,aarch64-softmmu'. I tried just x86_64
> > > alone,
> > > just aarch64 alone, and also configuring both x86_64 and i386, but none
> > > of those worked. For some reason grep isn't happy with the generated path
> > >
Re: [Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort
On Thu, Sep 06, 2018 at 09:02:14AM -0400, John Snow wrote:
> In cases where we abort the block/mirror job, there's no point in
> installing the new backing chain before we finish aborting.
>
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> block/mirror.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..bd3e908710 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
> * required before it could become a backing file of target_bs. */
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> _abort);
> -if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> BlockDriverState *backing = s->is_none_mode ? src : s->base;
> if (backing_bs(target_bs) != backing) {
> bdrv_set_backing_hd(target_bs, backing, _err);
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH 1/3] scsi: move lsi53c895a structures and defines into separate lsi53c895a.h file
On 06/09/18 12:52, Thomas Huth wrote: > On 2018-09-06 07:57, Mark Cave-Ayland wrote: >> There is also one small change to the new header file which is the addition >> of the previously missing LSI53C810 define. >> >> Signed-off-by: Mark Cave-Ayland >> --- >> hw/scsi/lsi53c895a.c | 116 +--- >> include/hw/scsi/lsi53c895a.h | 137 >> +++ >> 2 files changed, 138 insertions(+), 115 deletions(-) >> create mode 100644 include/hw/scsi/lsi53c895a.h > [...] >> diff --git a/include/hw/scsi/lsi53c895a.h b/include/hw/scsi/lsi53c895a.h >> new file mode 100644 >> index 00..d80cb78c69 >> --- /dev/null >> +++ b/include/hw/scsi/lsi53c895a.h >> @@ -0,0 +1,137 @@ >> +/* >> + * QEMU LSI53C895A SCSI Host Bus Adapter emulation >> + * >> + * Copyright (c) 2006 CodeSourcery. >> + * Written by Paul Brook >> + * >> + * This code is licensed under the LGPL. >> + */ >> + >> +#ifndef LSI_H >> +#define LSI_H >> + >> +#include "qemu/osdep.h" > > Please don't include osdep.h from a header, that should only be done > from .c files. Ah yes, my mistake (obviously it was a cut/paste from the .c file). I wonder if this is something to teach checkpatch about? ATB, Mark.
Re: [Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks
On Thu, Sep 06, 2018 at 09:02:13AM -0400, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
>
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
>
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
>
> During refactor, a potential problem with bdrv_drop_intermediate
> was identified, The patched behavior is no worse than the pre-patch
> behavior, so leave a FIXME for now to be fixed in a future patch.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/commit.c | 92
> --
> 1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index b6e8969877..a2da5740b0 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
> BlockDriverState *commit_top_bs;
> BlockBackend *top;
> BlockBackend *base;
> +BlockDriverState *base_bs;
> BlockdevOnError on_error;
> int base_flags;
> char *backing_file_str;
> @@ -68,61 +69,67 @@ static int coroutine_fn commit_populate(BlockBackend *bs,
> BlockBackend *base,
> return 0;
> }
>
> -static void commit_exit(Job *job)
> +static int commit_prepare(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -BlockJob *bjob = >common;
> -BlockDriverState *top = blk_bs(s->top);
> -BlockDriverState *base = blk_bs(s->base);
> -BlockDriverState *commit_top_bs = s->commit_top_bs;
> -bool remove_commit_top_bs = false;
> -
> -/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> -bdrv_ref(top);
> -bdrv_ref(commit_top_bs);
>
> /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> * the normal backing chain can be restored. */
> blk_unref(s->base);
> +s->base = NULL;
>
> -if (!job_is_cancelled(job) && job->ret == 0) {
> -/* success */
> -job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> - s->backing_file_str);
> -} else {
> -/* XXX Can (or should) we somehow keep 'consistent read' blocked even
> - * after the failed/cancelled commit job is gone? If we already wrote
> - * something to base, the intermediate images aren't valid any more.
> */
> -remove_commit_top_bs = true;
> +/* FIXME: bdrv_drop_intermediate treats total failures and partial
> failures
> + * identically. Further work is needed to disambiguate these cases. */
> +return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> + s->backing_file_str);
> +}
> +
> +static void commit_abort(Job *job)
> +{
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +BlockDriverState *top_bs = blk_bs(s->top);
> +
> +/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> +bdrv_ref(top_bs);
> +bdrv_ref(s->commit_top_bs);
> +
> +if (s->base) {
> +blk_unref(s->base);
> }
>
> +/* free the blockers on the intermediate nodes so that bdrv_replace_nodes
> + * can succeed */
> +block_job_remove_all_bdrv(>common);
> +
> +/* If bdrv_drop_intermediate() failed (or was not invoked), remove the
> + * commit filter driver from the backing chain now. Do this as the final
> + * step so that the 'consistent read' permission can be granted.
> + *
> + * XXX Can (or should) we somehow keep 'consistent read' blocked even
> + * after the failed/cancelled commit job is gone? If we already wrote
> + * something to base, the intermediate images aren't valid any more. */
> +bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> +_abort);
> +bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> + _abort);
> +
> +bdrv_unref(s->commit_top_bs);
> +bdrv_unref(top_bs);
> +}
> +
> +static void commit_clean(Job *job)
> +{
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +
> /* restore base open flags here if appropriate (e.g., change the base
> back
> * to r/o). These reopens do not need to be atomic, since we won't abort
> * even on failure here */
> -if (s->base_flags != bdrv_get_flags(base)) {
> -bdrv_reopen(base, s->base_flags, NULL);
> +if (s->base_flags != bdrv_get_flags(s->base_bs)) {
> +bdrv_reopen(s->base_bs, s->base_flags, NULL);
> }
> +
> g_free(s->backing_file_str);
> blk_unref(s->top);
> -
> -/* If there is more than one reference to the job (e.g. if called from
> - * job_finish_sync()), job_completed() won't free it and therefore the
> - * blockers on the intermediate nodes remain. This would cause
> - * bdrv_set_backing_hd() to fail. */
>
[Qemu-devel] [PATCH] hostmem-memfd: add checks before adding hostmem-memfd & properties
Run some memfd-related checks before registering hostmem-memfd &
various properties. This will help libvirt to figure out what the host
is supposed to be capable of.
qemu_memfd_check() is changed to a less optimized version, since it is
used with various flags, it no longer caches the result.
Signed-off-by: Marc-André Lureau
---
include/qemu/memfd.h | 18 +-
backends/hostmem-memfd.c | 32 +++-
tests/vhost-user-test.c | 6 +++---
util/memfd.c | 35 ++-
4 files changed, 45 insertions(+), 46 deletions(-)
diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
index 49e79634da..d551c28b68 100644
--- a/include/qemu/memfd.h
+++ b/include/qemu/memfd.h
@@ -16,12 +16,28 @@
#define F_SEAL_WRITE0x0008 /* prevent writes */
#endif
+#ifndef MFD_CLOEXEC
+#define MFD_CLOEXEC 0x0001U
+#endif
+
+#ifndef MFD_ALLOW_SEALING
+#define MFD_ALLOW_SEALING 0x0002U
+#endif
+
+#ifndef MFD_HUGETLB
+#define MFD_HUGETLB 0x0004U
+#endif
+
+#ifndef MFD_HUGE_SHIFT
+#define MFD_HUGE_SHIFT 26
+#endif
+
int qemu_memfd_create(const char *name, size_t size, bool hugetlb,
uint64_t hugetlbsize, unsigned int seals, Error **errp);
bool qemu_memfd_alloc_check(void);
void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
int *fd, Error **errp);
void qemu_memfd_free(void *ptr, size_t size, int fd);
-bool qemu_memfd_check(void);
+bool qemu_memfd_check(unsigned int flags);
#endif /* QEMU_MEMFD_H */
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 7184918112..ccf70a0694 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -138,18 +138,22 @@ memfd_backend_class_init(ObjectClass *oc, void *data)
bc->alloc = memfd_backend_memory_alloc;
-object_class_property_add_bool(oc, "hugetlb",
- memfd_backend_get_hugetlb,
- memfd_backend_set_hugetlb,
- _abort);
-object_class_property_add(oc, "hugetlbsize", "int",
- memfd_backend_get_hugetlbsize,
- memfd_backend_set_hugetlbsize,
- NULL, NULL, _abort);
-object_class_property_add_bool(oc, "seal",
- memfd_backend_get_seal,
- memfd_backend_set_seal,
- _abort);
+if (qemu_memfd_check(MFD_HUGETLB)) {
+object_class_property_add_bool(oc, "hugetlb",
+ memfd_backend_get_hugetlb,
+ memfd_backend_set_hugetlb,
+ _abort);
+object_class_property_add(oc, "hugetlbsize", "int",
+ memfd_backend_get_hugetlbsize,
+ memfd_backend_set_hugetlbsize,
+ NULL, NULL, _abort);
+}
+if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
+object_class_property_add_bool(oc, "seal",
+ memfd_backend_get_seal,
+ memfd_backend_set_seal,
+ _abort);
+}
}
static const TypeInfo memfd_backend_info = {
@@ -162,7 +166,9 @@ static const TypeInfo memfd_backend_info = {
static void register_types(void)
{
-type_register_static(_backend_info);
+if (qemu_memfd_check(0)) {
+type_register_static(_backend_info);
+}
}
type_init(register_types);
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 716aff7153..45d58d8ea2 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s,
int mem, enum test_memfd memfd, const char *mem_path,
const char *chr_opts, const char *extra)
{
-if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check()) {
+if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) {
memfd = TEST_MEMFD_YES;
}
@@ -903,7 +903,7 @@ static void test_multiqueue(void)
s->queues = 2;
test_server_listen(s);
-if (qemu_memfd_check()) {
+if (qemu_memfd_check(0)) {
cmd = g_strdup_printf(
QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d "
"-device virtio-net-pci,netdev=net0,mq=on,vectors=%d",
@@ -963,7 +963,7 @@ int main(int argc, char **argv)
/* run the main loop thread so the chardev may operate */
thread = g_thread_new(NULL, thread_function, loop);
-if (qemu_memfd_check()) {
+if (qemu_memfd_check(0)) {
qtest_add_data_func("/vhost-user/read-guest-mem/memfd",
GINT_TO_POINTER(TEST_MEMFD_YES),
test_read_guest_mem);
diff --git a/util/memfd.c b/util/memfd.c
index
Re: [Qemu-devel] [PATCH] RISC-V - Dynamic parameterization of RISC-V memory map
On 6 September 2018 at 16:07, Michael Eager wrote: > Any comments? I'd quite like to hear from somebody more familiar with the readconfig/writeconfig stuff than me about whether this very riscv-centric approach makes sense and fits with how the config file is used by other parts of QEMU. I'm not sure who would be best to do that review, though. Paolo, any suggestions for who knows that bit of the code? thanks -- PMM > On 08/30/2018 09:22 AM, Michael Eager wrote: >> >> Corrected patch attached. >> >> On 08/29/2018 05:48 PM, Michael Eager wrote: >>> >>> Whoops. I just noticed that this patch is against the riscv-qemu >>> repo on github, not the qemu.org repo. I will rework it for the qemu.org >>> repo. Meanwhile, I welcome any comments. >>> >>> On 08/29/2018 05:21 PM, Michael Eager wrote: Memory parameters for RISC-V boards can be read from a configuration file using the -readconfig command line option. The configuration file should have a section for the board and memory. The configuration for the VirtIO board has the following configuration variables: [riscv-virt-mem] debug-base = "0x0" debug-size = "0x100" mrom-base= "0x1000" mrom-size= "0x11000" test-base= "0x10" test-size= "0x1000" clint-base = "0x200" clint-size = "0x1" plic-base= "0xc00" plic-size= "0x400" uart0-base = "0x1000" uart0-size = "0x100" virtio-base = "0x10001000" virtio-size = "0x1000" dram-base= "0x8000" dram-size= "0x0" Values must be enclosed within quotes. Signed-off-by: Michael Eager >>> >> > > -- > Michael Eagerea...@eagerm.com > 1960 Park Blvd., Palo Alto, CA 94306 >
Re: [Qemu-devel] [PATCH 10/10] vl: list user creatable propeties if '?' as argument
On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
Subject has typo and awkward grammar; I'd suggest:
vl: list user creatable properties when 'help' is argument
Iterate over the writable class properties, sort and print them out
with the description if available.
Ex: qemu -object memory-backend-memfd,?
As elsewhere in the series, s/\?/help/
memory-backend-memfd.dump=bool (Set to 'off' to exclude from core dump)
memory-backend-memfd.host-nodes=int (Binds memory to the list of NUMA host
nodes)
memory-backend-memfd.hugetlb=bool (Use huge pages)
memory-backend-memfd.hugetlbsize=int (Huge pages size (ex: 2M, 1G))
memory-backend-memfd.merge=bool (Mark memory as mergeable)
memory-backend-memfd.policy=HostMemPolicy (Set the NUMA policy)
memory-backend-memfd.prealloc=bool (Preallocate memory)
memory-backend-memfd.seal=bool (Seal growing & shrinking)
memory-backend-memfd.share=bool (Mark the memory as private to QEMU or shared)
memory-backend-memfd.size=int (Size of the memory region (ex: 500M))
Why "name=type (text)" here, but "name=type - text" in 2/10?
Consistency would be nice.
Signed-off-by: Marc-André Lureau
---
qom/object_interfaces.c | 6 +++---
vl.c| 45 ++---
2 files changed, 45 insertions(+), 6 deletions(-)
+++ b/vl.c
@@ -2721,6 +2721,11 @@ static int machine_set_property(void *opaque,
return 0;
}
+static gint
+pstrcmp(const char **a, const char **b)
+{
+return g_strcmp0(*a, *b);
+}
This is the second time your series has added this static helper. Should
it be a common helper instead?
+g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
+for (i = 0; i < array->len; i++) {
+error_printf("%s\n", (char *)array->pdata[i]);
+}
+g_ptr_array_set_free_func(array, g_free);
+g_ptr_array_free(array, true);
+exit(0);
Again, printing to stderr then exiting with status 0 is awkward. Print
to stdout when successfully offering help text.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 08/10] vl: handle -object ?
On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
In the subject, s/\?/help/
Signed-off-by: Marc-André Lureau
---
vl.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/vl.c b/vl.c
index 5ba06adf78..8a5fd0c81f 100644
--- a/vl.c
+++ b/vl.c
@@ -2731,6 +2731,19 @@ static int machine_set_property(void *opaque,
*/
static bool object_create_initial(const char *type)
{
+if (is_help_option(type)) {
+GSList *l, *list;
+
+error_printf("List of user creatable objects:\n");
+list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
+for (l = list; l != NULL; l = l->next) {
+ObjectClass *oc = OBJECT_CLASS(l->data);
+error_printf("%s\n", object_class_get_name(oc));
This prints to stderr,
+}
+g_slist_free(list);
+exit(0);
then exits with status 0. I'd much rather successful help text be
printed to stdout.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 02/10] qemu-option: improve qemu_opts_print_help() output
On 09/06/2018 10:12 AM, Marc-André Lureau wrote: Modify qemu_opts_print_help(): - to print expected argument type - skip description if not available - sort lines - prefix with the list name (like qdev, to avoid confusion) - drop 16-chars alignment, use a '-' as seperator for option name and description For ex, "-spice ?" output is changed from: s/?/help/ port No description available tls-port No description available addr No description available [...] gl No description available rendernode No description available to: spice.addr=str spice.agent-mouse=bool (on/off) spice.disable-agent-file-xfer=bool (on/off) [...] spice.x509-key-password=str spice.zlib-glz-wan-compression=str "qemu-img create -f qcow2 -o ?", changed from: and again size Virtual disk size compat Compatibility level (0.10 or 1.1) backing_file File name of a base image [...] lazy_refcounts Postpone refcount updates refcount_bitsWidth of a reference count entry in bits to: backing_file=str - File name of a base image backing_fmt=str - Image format of the base image cluster_size=size - qcow2 cluster size [...] refcount_bits=num - Width of a reference count entry in bits size=size - Virtual disk size Signed-off-by: Marc-André Lureau --- util/qemu-option.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 01/10] qemu-option: add help fallback to print the list of options
On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
QDev options accept '?' or 'help' in the list of parameters, which is
really handy to list the available options.
Unfortunately, this isn't built in QemuOpts. qemu_opts_parse_noisily()
seems to be the common path for command line options, so place a
fallback to check for '?' and print help listing available options.
This is very handy, for example with qemu "-spice ?".
Is that literal spelling intended (a single argument with an embedded
space)? Because that would result in:
qemu: -spice help: invalid option
Or did you mean "qemu -spice '?'" (with the outer quotes delimiting what
you type, and the inner quote properly escaping the ? to avoid
unintended globbing if you have a one-byte file name in the current
directory)?
Also, I'd rather see you favor the 'help' spelling in your text; the '?'
spelling should continue to work (by virtue of has_help_option), but as
that form requires shell quoting while 'help' does not, we should
minimize its use in our documentation to avoid people forgetting to
quote it and then hitting unintended shell globbing effects. That would
make your example "qemu -spice help".
Signed-off-by: Marc-André Lureau
---
util/qemu-option.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
@@ -886,10 +891,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list,
const char *params,
{
Error *err = NULL;
QemuOpts *opts;
+bool invalidp = false;
-opts = opts_parse(list, params, permit_abbrev, false, );
+opts = opts_parse(list, params, permit_abbrev, false, , );
if (err) {
-error_report_err(err);
+if (invalidp && has_help_option(params)) {
+qemu_opts_print_help(list);
+error_free(err);
+} else {
+error_report_err(err);
+}
This makes sense. With a better commit message,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-devel] [PATCH 09/10] hostmem: add some properties description
Signed-off-by: Marc-André Lureau
---
backends/hostmem-memfd.c | 9 +
backends/hostmem.c | 14 ++
2 files changed, 23 insertions(+)
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 1e20fe0ba8..789c8c3f87 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -144,14 +144,23 @@ memfd_backend_class_init(ObjectClass *oc, void *data)
memfd_backend_get_hugetlb,
memfd_backend_set_hugetlb,
_abort);
+object_class_property_set_description(oc, "hugetlb",
+ "Use huge pages",
+ _abort);
object_class_property_add(oc, "hugetlbsize", "int",
memfd_backend_get_hugetlbsize,
memfd_backend_set_hugetlbsize,
NULL, NULL, _abort);
+object_class_property_set_description(oc, "hugetlbsize",
+ "Huge pages size (ex: 2M, 1G)",
+ _abort);
object_class_property_add_bool(oc, "seal",
memfd_backend_get_seal,
memfd_backend_set_seal,
_abort);
+object_class_property_set_description(oc, "seal",
+ "Seal growing & shrinking",
+ _abort);
}
static const TypeInfo memfd_backend_info = {
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4908946cd3..1a89342039 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -397,27 +397,41 @@ host_memory_backend_class_init(ObjectClass *oc, void
*data)
object_class_property_add_bool(oc, "merge",
host_memory_backend_get_merge,
host_memory_backend_set_merge, _abort);
+object_class_property_set_description(oc, "merge",
+"Mark memory as mergeable", _abort);
object_class_property_add_bool(oc, "dump",
host_memory_backend_get_dump,
host_memory_backend_set_dump, _abort);
+object_class_property_set_description(oc, "dump",
+"Set to 'off' to exclude from core dump", _abort);
object_class_property_add_bool(oc, "prealloc",
host_memory_backend_get_prealloc,
host_memory_backend_set_prealloc, _abort);
+object_class_property_set_description(oc, "prealloc",
+"Preallocate memory", _abort);
object_class_property_add(oc, "size", "int",
host_memory_backend_get_size,
host_memory_backend_set_size,
NULL, NULL, _abort);
+object_class_property_set_description(oc, "size",
+"Size of the memory region (ex: 500M)", _abort);
object_class_property_add(oc, "host-nodes", "int",
host_memory_backend_get_host_nodes,
host_memory_backend_set_host_nodes,
NULL, NULL, _abort);
+object_class_property_set_description(oc, "host-nodes",
+"Binds memory to the list of NUMA host nodes", _abort);
object_class_property_add_enum(oc, "policy", "HostMemPolicy",
_lookup,
host_memory_backend_get_policy,
host_memory_backend_set_policy, _abort);
+object_class_property_set_description(oc, "policy",
+"Set the NUMA policy", _abort);
object_class_property_add_bool(oc, "share",
host_memory_backend_get_share, host_memory_backend_set_share,
_abort);
+object_class_property_set_description(oc, "share",
+"Mark the memory as private to QEMU or shared", _abort);
}
static const TypeInfo host_memory_backend_info = {
--
2.19.0.rc1
[Qemu-devel] [PATCH 04/10] qom/object: register 'type' property as class property
Signed-off-by: Marc-André Lureau
---
qom/object.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index d8666de3f2..185d1dd9f8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2423,9 +2423,10 @@ void object_class_property_set_description(ObjectClass
*klass,
op->description = g_strdup(description);
}
-static void object_instance_init(Object *obj)
+static void object_class_init(ObjectClass *klass, void *data)
{
-object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
+object_class_property_add_str(klass, "type", qdev_get_type,
+ NULL, _abort);
}
static void register_types(void)
@@ -2439,7 +2440,7 @@ static void register_types(void)
static TypeInfo object_info = {
.name = TYPE_OBJECT,
.instance_size = sizeof(Object),
-.instance_init = object_instance_init,
+.class_init = object_class_init,
.abstract = true,
};
--
2.19.0.rc1
[Qemu-devel] [PATCH 02/10] qemu-option: improve qemu_opts_print_help() output
Modify qemu_opts_print_help():
- to print expected argument type
- skip description if not available
- sort lines
- prefix with the list name (like qdev, to avoid confusion)
- drop 16-chars alignment, use a '-' as seperator for option name and
description
For ex, "-spice ?" output is changed from:
port No description available
tls-port No description available
addr No description available
[...]
gl No description available
rendernode No description available
to:
spice.addr=str
spice.agent-mouse=bool (on/off)
spice.disable-agent-file-xfer=bool (on/off)
[...]
spice.x509-key-password=str
spice.zlib-glz-wan-compression=str
"qemu-img create -f qcow2 -o ?", changed from:
size Virtual disk size
compat Compatibility level (0.10 or 1.1)
backing_file File name of a base image
[...]
lazy_refcounts Postpone refcount updates
refcount_bitsWidth of a reference count entry in bits
to:
backing_file=str - File name of a base image
backing_fmt=str - Image format of the base image
cluster_size=size - qcow2 cluster size
[...]
refcount_bits=num - Width of a reference count entry in bits
size=size - Virtual disk size
Signed-off-by: Marc-André Lureau
---
util/qemu-option.c | 44 ++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 557b6c6626..a54cf74b49 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -208,17 +208,57 @@ out:
return result;
}
+static const char *opt_type_to_string(enum QemuOptType type)
+{
+switch (type) {
+case QEMU_OPT_STRING:
+return "str";
+case QEMU_OPT_BOOL:
+return "bool (on/off)";
+case QEMU_OPT_NUMBER:
+return "num";
+case QEMU_OPT_SIZE:
+return "size";
+}
+
+g_assert_not_reached();
+}
+
+static gint
+pstrcmp(const char **a, const char **b)
+{
+return g_strcmp0(*a, *b);
+}
+
void qemu_opts_print_help(QemuOptsList *list)
{
QemuOptDesc *desc;
+int i;
+GPtrArray *array = g_ptr_array_new();
assert(list);
desc = list->desc;
while (desc && desc->name) {
-printf("%-16s %s\n", desc->name,
- desc->help ? desc->help : "No description available");
+GString *str = g_string_new(NULL);
+if (list->name) {
+g_string_append_printf(str, "%s.", list->name);
+}
+g_string_append_printf(str, "%s=%s", desc->name,
+ opt_type_to_string(desc->type));
+if (desc->help) {
+g_string_append_printf(str, " - %s", desc->help);
+}
+g_ptr_array_add(array, g_string_free(str, false));
desc++;
}
+
+g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
+for (i = 0; i < array->len; i++) {
+printf("%s\n", (char *)array->pdata[i]);
+}
+g_ptr_array_set_free_func(array, g_free);
+g_ptr_array_free(array, true);
+
}
/* -- */
--
2.19.0.rc1
[Qemu-devel] [PATCH 10/10] vl: list user creatable propeties if '?' as argument
Iterate over the writable class properties, sort and print them out
with the description if available.
Ex: qemu -object memory-backend-memfd,?
memory-backend-memfd.dump=bool (Set to 'off' to exclude from core dump)
memory-backend-memfd.host-nodes=int (Binds memory to the list of NUMA host
nodes)
memory-backend-memfd.hugetlb=bool (Use huge pages)
memory-backend-memfd.hugetlbsize=int (Huge pages size (ex: 2M, 1G))
memory-backend-memfd.merge=bool (Mark memory as mergeable)
memory-backend-memfd.policy=HostMemPolicy (Set the NUMA policy)
memory-backend-memfd.prealloc=bool (Preallocate memory)
memory-backend-memfd.seal=bool (Seal growing & shrinking)
memory-backend-memfd.share=bool (Mark the memory as private to QEMU or shared)
memory-backend-memfd.size=int (Size of the memory region (ex: 500M))
Signed-off-by: Marc-André Lureau
---
qom/object_interfaces.c | 6 +++---
vl.c| 45 ++---
2 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 72b97a8bed..941fd63afd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -141,14 +141,14 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error
**errp)
int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
{
-bool (*type_predicate)(const char *) = opaque;
+bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque;
Object *obj = NULL;
Error *err = NULL;
const char *type;
type = qemu_opt_get(opts, "qom-type");
-if (type && type_predicate &&
-!type_predicate(type)) {
+if (type && type_opt_predicate &&
+!type_opt_predicate(type, opts)) {
return 0;
}
diff --git a/vl.c b/vl.c
index 8a5fd0c81f..75c2bd5130 100644
--- a/vl.c
+++ b/vl.c
@@ -2721,6 +2721,11 @@ static int machine_set_property(void *opaque,
return 0;
}
+static gint
+pstrcmp(const char **a, const char **b)
+{
+return g_strcmp0(*a, *b);
+}
/*
* Initial object creation happens before all other
@@ -2729,8 +2734,10 @@ static int machine_set_property(void *opaque,
* cannot be created here, as it depends on the chardev
* already existing.
*/
-static bool object_create_initial(const char *type)
+static bool object_create_initial(const char *type, QemuOpts *opts)
{
+ObjectClass *klass;
+
if (is_help_option(type)) {
GSList *l, *list;
@@ -2744,6 +2751,38 @@ static bool object_create_initial(const char *type)
exit(0);
}
+klass = object_class_by_name(type);
+if (klass && qemu_opt_has_help_opt(opts)) {
+ObjectPropertyIterator iter;
+ObjectProperty *prop;
+GPtrArray *array = g_ptr_array_new();
+int i;
+
+object_class_property_iter_init(, klass);
+while ((prop = object_property_iter_next())) {
+GString *str;
+
+if (!prop->set) {
+continue;
+}
+
+str = g_string_new(NULL);
+g_string_append_printf(str, "%s.%s=%s", type,
+ prop->name, prop->type);
+if (prop->description) {
+g_string_append_printf(str, " (%s)", prop->description);
+}
+g_ptr_array_add(array, g_string_free(str, false));
+}
+g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
+for (i = 0; i < array->len; i++) {
+error_printf("%s\n", (char *)array->pdata[i]);
+}
+g_ptr_array_set_free_func(array, g_free);
+g_ptr_array_free(array, true);
+exit(0);
+}
+
if (g_str_equal(type, "rng-egd") ||
g_str_has_prefix(type, "pr-manager-")) {
return false;
@@ -2790,9 +2829,9 @@ static bool object_create_initial(const char *type)
* The remainder of object creation happens after the
* creation of chardev, fsdev, net clients and device data types.
*/
-static bool object_create_delayed(const char *type)
+static bool object_create_delayed(const char *type, QemuOpts *opts)
{
-return !object_create_initial(type);
+return !object_create_initial(type, opts);
}
--
2.19.0.rc1
[Qemu-devel] [PATCH 05/10] tests/qom-proplist: check duplicate "bv" property registration failed
"bv" is already a class property.
Signed-off-by: Marc-André Lureau
---
tests/check-qom-proplist.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 92898e1520..0f6d9c1ce3 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -125,10 +125,13 @@ static char *dummy_get_sv(Object *obj,
static void dummy_init(Object *obj)
{
+Error *err = NULL;
+
object_property_add_bool(obj, "bv",
dummy_get_bv,
dummy_set_bv,
- NULL);
+ );
+error_free_or_abort();
}
--
2.19.0.rc1
[Qemu-devel] [PATCH 08/10] vl: handle -object ?
Signed-off-by: Marc-André Lureau
---
vl.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/vl.c b/vl.c
index 5ba06adf78..8a5fd0c81f 100644
--- a/vl.c
+++ b/vl.c
@@ -2731,6 +2731,19 @@ static int machine_set_property(void *opaque,
*/
static bool object_create_initial(const char *type)
{
+if (is_help_option(type)) {
+GSList *l, *list;
+
+error_printf("List of user creatable objects:\n");
+list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
+for (l = list; l != NULL; l = l->next) {
+ObjectClass *oc = OBJECT_CLASS(l->data);
+error_printf("%s\n", object_class_get_name(oc));
+}
+g_slist_free(list);
+exit(0);
+}
+
if (g_str_equal(type, "rng-egd") ||
g_str_has_prefix(type, "pr-manager-")) {
return false;
--
2.19.0.rc1
[Qemu-devel] [PATCH 01/10] qemu-option: add help fallback to print the list of options
QDev options accept '?' or 'help' in the list of parameters, which is
really handy to list the available options.
Unfortunately, this isn't built in QemuOpts. qemu_opts_parse_noisily()
seems to be the common path for command line options, so place a
fallback to check for '?' and print help listing available options.
This is very handy, for example with qemu "-spice ?".
Signed-off-by: Marc-André Lureau
---
util/qemu-option.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 01886efe90..557b6c6626 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -486,7 +486,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
}
static void opt_set(QemuOpts *opts, const char *name, char *value,
-bool prepend, Error **errp)
+bool prepend, bool *invalidp, Error **errp)
{
QemuOpt *opt;
const QemuOptDesc *desc;
@@ -496,6 +496,9 @@ static void opt_set(QemuOpts *opts, const char *name, char
*value,
if (!desc && !opts_accepts_any(opts)) {
g_free(value);
error_setg(errp, QERR_INVALID_PARAMETER, name);
+if (invalidp) {
+*invalidp = true;
+}
return;
}
@@ -519,7 +522,7 @@ static void opt_set(QemuOpts *opts, const char *name, char
*value,
void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
Error **errp)
{
-opt_set(opts, name, g_strdup(value), false, errp);
+opt_set(opts, name, g_strdup(value), false, NULL, errp);
}
void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
@@ -750,7 +753,8 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
}
static void opts_do_parse(QemuOpts *opts, const char *params,
- const char *firstname, bool prepend, Error **errp)
+ const char *firstname, bool prepend,
+ bool *invalidp, Error **errp)
{
char *option = NULL;
char *value = NULL;
@@ -785,7 +789,7 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
}
if (strcmp(option, "id") != 0) {
/* store and parse */
-opt_set(opts, option, value, prepend, _err);
+opt_set(opts, option, value, prepend, invalidp, _err);
value = NULL;
if (local_err) {
error_propagate(errp, local_err);
@@ -814,11 +818,12 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
void qemu_opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, Error **errp)
{
-opts_do_parse(opts, params, firstname, false, errp);
+opts_do_parse(opts, params, firstname, false, NULL, errp);
}
static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-bool permit_abbrev, bool defaults, Error **errp)
+bool permit_abbrev, bool defaults,
+bool *invalidp, Error **errp)
{
const char *firstname;
char *id = NULL;
@@ -850,7 +855,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char
*params,
return NULL;
}
-opts_do_parse(opts, params, firstname, defaults, _err);
+opts_do_parse(opts, params, firstname, defaults, invalidp, _err);
if (local_err) {
error_propagate(errp, local_err);
qemu_opts_del(opts);
@@ -870,7 +875,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char
*params,
QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
bool permit_abbrev, Error **errp)
{
-return opts_parse(list, params, permit_abbrev, false, errp);
+return opts_parse(list, params, permit_abbrev, false, NULL, errp);
}
/**
@@ -886,10 +891,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list,
const char *params,
{
Error *err = NULL;
QemuOpts *opts;
+bool invalidp = false;
-opts = opts_parse(list, params, permit_abbrev, false, );
+opts = opts_parse(list, params, permit_abbrev, false, , );
if (err) {
-error_report_err(err);
+if (invalidp && has_help_option(params)) {
+qemu_opts_print_help(list);
+error_free(err);
+} else {
+error_report_err(err);
+}
}
return opts;
}
@@ -899,7 +910,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char
*params,
{
QemuOpts *opts;
-opts = opts_parse(list, params, permit_abbrev, true, NULL);
+opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
assert(opts);
}
--
2.19.0.rc1
[Qemu-devel] [PATCH 06/10] tests/qom-proplist: check properties are not listed multiple times
And factor out a common function used by the follow class properties
iterator test.
Signed-off-by: Marc-André Lureau
---
tests/check-qom-proplist.c | 44 +-
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 0f6d9c1ce3..8e1b9c27f3 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -520,32 +520,19 @@ static void test_dummy_getenum(void)
}
-static void test_dummy_iterator(void)
+static void test_dummy_prop_iterator(ObjectPropertyIterator *iter)
{
-Object *parent = object_get_objects_root();
-DummyObject *dobj = DUMMY_OBJECT(
-object_new_with_props(TYPE_DUMMY,
- parent,
- "dummy0",
- _abort,
- "bv", "yes",
- "sv", "Hiss hiss hiss",
- "av", "platypus",
- NULL));
-
-ObjectProperty *prop;
-ObjectPropertyIterator iter;
bool seenbv = false, seensv = false, seenav = false, seentype;
+ObjectProperty *prop;
-object_property_iter_init(, OBJECT(dobj));
-while ((prop = object_property_iter_next())) {
-if (g_str_equal(prop->name, "bv")) {
+while ((prop = object_property_iter_next(iter))) {
+if (!seenbv && g_str_equal(prop->name, "bv")) {
seenbv = true;
-} else if (g_str_equal(prop->name, "sv")) {
+} else if (!seensv && g_str_equal(prop->name, "sv")) {
seensv = true;
-} else if (g_str_equal(prop->name, "av")) {
+} else if (!seenav && g_str_equal(prop->name, "av")) {
seenav = true;
-} else if (g_str_equal(prop->name, "type")) {
+} else if (!seentype && g_str_equal(prop->name, "type")) {
/* This prop comes from the base Object class */
seentype = true;
} else {
@@ -557,7 +544,24 @@ static void test_dummy_iterator(void)
g_assert(seenav);
g_assert(seensv);
g_assert(seentype);
+}
+
+static void test_dummy_iterator(void)
+{
+Object *parent = object_get_objects_root();
+DummyObject *dobj = DUMMY_OBJECT(
+object_new_with_props(TYPE_DUMMY,
+ parent,
+ "dummy0",
+ _abort,
+ "bv", "yes",
+ "sv", "Hiss hiss hiss",
+ "av", "platypus",
+ NULL));
+ObjectPropertyIterator iter;
+object_property_iter_init(, OBJECT(dobj));
+test_dummy_prop_iterator();
object_unparent(OBJECT(dobj));
}
--
2.19.0.rc1
[Qemu-devel] [PATCH 07/10] tests/qom-proplist: check class properties iterator
This test failed before "fix iterating properties over a class".
Signed-off-by: Marc-André Lureau
---
tests/check-qom-proplist.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 8e1b9c27f3..7ed16b704b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -565,6 +565,14 @@ static void test_dummy_iterator(void)
object_unparent(OBJECT(dobj));
}
+static void test_dummy_class_iterator(void)
+{
+ObjectPropertyIterator iter;
+ObjectClass *klass = object_class_by_name(TYPE_DUMMY);
+
+object_class_property_iter_init(, klass);
+test_dummy_prop_iterator();
+}
static void test_dummy_delchild(void)
{
@@ -636,6 +644,7 @@ int main(int argc, char **argv)
g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
+g_test_add_func("/qom/proplist/class_iterator", test_dummy_class_iterator);
g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
g_test_add_func("/qom/resolve/partial", test_qom_partial_path);
--
2.19.0.rc1
[Qemu-devel] [PATCH 00/10] Various qemu command line options help improvements
Hi, This is a compilation of patches I have to improve command line help support. The first 2 patches have already been sent earlier, I modified the first to fix an issue reported by Markus. The other patches add support for -object help. A few preliminary patches for QOM, to fix/improve some minor issues. Marc-André Lureau (10): qemu-option: add help fallback to print the list of options qemu-option: improve qemu_opts_print_help() output qom/object: fix iterating properties over a class qom/object: register 'type' property as class property tests/qom-proplist: check duplicate "bv" property registration failed tests/qom-proplist: check properties are not listed multiple times tests/qom-proplist: check class properties iterator vl: handle -object ? hostmem: add some properties description vl: list user creatable propeties if '?' as argument backends/hostmem-memfd.c | 9 + backends/hostmem.c | 14 +++ qom/object.c | 9 +++-- qom/object_interfaces.c| 6 +-- tests/check-qom-proplist.c | 58 +--- util/qemu-option.c | 77 +++--- vl.c | 58 ++-- 7 files changed, 187 insertions(+), 44 deletions(-) -- 2.19.0.rc1
[Qemu-devel] [PATCH 03/10] qom/object: fix iterating properties over a class
object_class_property_iter_init() starts from the given class, so the
next class should continue with the parent class.
Signed-off-by: Marc-André Lureau
---
qom/object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qom/object.c b/qom/object.c
index 75d1d48944..d8666de3f2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1108,7 +1108,7 @@ void
object_class_property_iter_init(ObjectPropertyIterator *iter,
ObjectClass *klass)
{
g_hash_table_iter_init(>iter, klass->properties);
-iter->nextclass = klass;
+iter->nextclass = object_class_get_parent(klass);
}
ObjectProperty *object_class_property_find(ObjectClass *klass, const char
*name,
--
2.19.0.rc1
Re: [Qemu-devel] backend for blk or fs with guaranteed blocking/synchronous I/O
On Thu, Sep 06, 2018 at 04:24:12PM +0600, Artem Pisarenko wrote: > Hi all, > > I'm developing paravirtualized target linux system which runs multiple linux > containers (LXC) inside itself. (For those, who unfamiliar with LXC, simply > put, it's an isolated group of userspace processes with their own rootfs.) > Each > container should be provided access to its rootfs located at host and > execution > of container should be deterministic. Particularly, it means that container > I/O > operations must be synchronized within some predefined quantum of guest > _virtual_ time, i.e. its I/O activity shouldn't be delayed by host performance > or activities on host and other containers. In other words, guest should see > it's like either infinite throughput and zero latency, or some predefined > throughput/latency characteristics guaranteed per each rootfs. > > While other sources of non-determinism are seem to be eliminated (using TCG, > -icount, etc.), asynchronous I/O still introduces it. ... Just that you should realize that the issues are not limited to QEMU: to get real time behaviour out of a Linux host you need a real-time kernel and real-time capable hardware/firmware. I'm not an expert on this at all, but see e.g. these old presentations: https://lwn.net/Articles/656807/ -- MST
Re: [Qemu-devel] [PATCH] RISC-V - Dynamic parameterization of RISC-V memory map
Any comments? On 08/30/2018 09:22 AM, Michael Eager wrote: Corrected patch attached. On 08/29/2018 05:48 PM, Michael Eager wrote: Whoops. I just noticed that this patch is against the riscv-qemu repo on github, not the qemu.org repo. I will rework it for the qemu.org repo. Meanwhile, I welcome any comments. On 08/29/2018 05:21 PM, Michael Eager wrote: Memory parameters for RISC-V boards can be read from a configuration file using the -readconfig command line option. The configuration file should have a section for the board and memory. The configuration for the VirtIO board has the following configuration variables: [riscv-virt-mem] debug-base = "0x0" debug-size = "0x100" mrom-base = "0x1000" mrom-size = "0x11000" test-base = "0x10" test-size = "0x1000" clint-base = "0x200" clint-size = "0x1" plic-base = "0xc00" plic-size = "0x400" uart0-base = "0x1000" uart0-size = "0x100" virtio-base = "0x10001000" virtio-size = "0x1000" dram-base = "0x8000" dram-size = "0x0" Values must be enclosed within quotes. Signed-off-by: Michael Eager -- Michael Eagerea...@eagerm.com 1960 Park Blvd., Palo Alto, CA 94306
Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions
On 6 September 2018 at 13:02, Thomas Huth wrote: > I somehow fail to see that something outside of lsi53c895a.c should > really need to access the internals of LSIState. If there is something > that needs to be configured from the outside, it should be done via QOM > properties instead, shouldn't it? So I think I'd rather prefer if you > could do it the other way round and change the lsi*_create() functions > to return a pointer to PCIDevice instead, if possible. Nothing typically does, but the "modern" style of having QOM objects which use other QOM objects do so by embedding the child object's struct into the struct of the parent requires that the struct definition is available. The only thing of it that is used is (effectively) its size and alignment requirements. (I had an RFC patch some years back that would allow you to mark up the fields of the struct as private, so the struct could be put in the header but the compiler would complain if you tried to actually access any of its fields: http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01846.html ) thanks -- PMM
[Qemu-devel] [PATCH] block: Fix use after free error in bdrv_open_inherit()
When a block device is opened with BDRV_O_SNAPSHOT and the bdrv_append_temp_snapshot() call fails then the error code path tries to unref the already destroyed 'options' QDict. This can be reproduced easily by setting TMPDIR to a location where the QEMU process can't write: $ TMPDIR=/nonexistent $QEMU -drive driver=null-co,snapshot=on Signed-off-by: Alberto Garcia --- block.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block.c b/block.c index 0dbb1fcc7b..a381c8ece8 100644 --- a/block.c +++ b/block.c @@ -2792,6 +2792,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, bdrv_parent_cb_change_media(bs, true); qobject_unref(options); +options = NULL; /* For snapshot=on, create a temporary qcow2 overlay. bs points to the * temporary snapshot afterwards. */ -- 2.11.0
Re: [Qemu-devel] Some confusion about live migration of usb device
> -原始邮件- > 发件人: "gerd hoffmann" > 发送时间: 2018-09-06 21:52:23 (星期四) > 收件人: linzhecheng > 抄送: "wangxin (U)" , CheneyLin , > "qemu-devel@nongnu.org" > 主题: Re: [Qemu-devel] Some confusion about live migration of usb device > > On Thu, Sep 06, 2018 at 12:10:08PM +, linzhecheng wrote: > > You had said that copying vmstate of usb-host is pointless, so just unpulg > > and plug it after migration is all right, > > but will other usb devices like usb-storage devices lose pending > > USBPackets then? > > Ah, emulated usb devices. The usb host adapters will re-submit any > unfinished usb transfers on the target machine. You mean target vm usb controllers will re-submit them? And what about usb-redir, btw? > > cheers, > Gerd >
Re: [Qemu-devel] [PATCH v2] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
On 09/06/18 14:51, Igor Mammedov wrote:
> If VM has VCPUs plugged sparselly (for example a VM started with
> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
> only cpu0 and cpu2 are present), QGA will rise a error
> error: internal error: unable to execute QEMU agent command
> 'guest-get-vcpus':
> open("/sys/devices/system/cpu/cpu1/"): No such file or directory
> when
> virsh vcpucount FOO --guest
> is executed.
> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
>
> Signed-off-by: Igor Mammedov
> ---
> v2:
> do not create CPU entry if cpu isn't present
> (Laszlo Ersek )
> ---
> qga/commands-posix.c | 115
> ++-
> 1 file changed, 59 insertions(+), 56 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 37e8a2d..42d30f0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2035,61 +2035,56 @@ static long sysconf_exact(int name, const char
> *name_str, Error **errp)
> * Written members remain unmodified on error.
> */
> static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
> - Error **errp)
> + char *dirpath, Error **errp)
> {
> -char *dirpath;
> +int fd;
> +int res;
> int dirfd;
> +static const char fn[] = "online";
>
> -dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
> - vcpu->logical_id);
> dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> if (dirfd == -1) {
> error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> -} else {
> -static const char fn[] = "online";
> -int fd;
> -int res;
> -
> -fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
> -if (fd == -1) {
> -if (errno != ENOENT) {
> -error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath,
> fn);
> -} else if (sys2vcpu) {
> -vcpu->online = true;
> -vcpu->can_offline = false;
> -} else if (!vcpu->online) {
> -error_setg(errp, "logical processor #%" PRId64 " can't be "
> - "offlined", vcpu->logical_id);
> -} /* otherwise pretend successful re-onlining */
> -} else {
> -unsigned char status;
> -
> -res = pread(fd, , 1, 0);
> -if (res == -1) {
> -error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath,
> fn);
> -} else if (res == 0) {
> -error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
> - fn);
> -} else if (sys2vcpu) {
> -vcpu->online = (status != '0');
> -vcpu->can_offline = true;
> -} else if (vcpu->online != (status != '0')) {
> -status = '0' + vcpu->online;
> -if (pwrite(fd, , 1, 0) == -1) {
> -error_setg_errno(errp, errno, "pwrite(\"%s/%s\")",
> dirpath,
> - fn);
> -}
> -} /* otherwise pretend successful re-(on|off)-lining */
> +return;
> +}
>
> -res = close(fd);
> -g_assert(res == 0);
> -}
> +fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
> +if (fd == -1) {
> +if (errno != ENOENT) {
> +error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
> +} else if (sys2vcpu) {
> +vcpu->online = true;
> +vcpu->can_offline = false;
> +} else if (!vcpu->online) {
> +error_setg(errp, "logical processor #%" PRId64 " can't be "
> + "offlined", vcpu->logical_id);
> +} /* otherwise pretend successful re-onlining */
> +} else {
> +unsigned char status;
> +
> +res = pread(fd, , 1, 0);
> +if (res == -1) {
> +error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
> +} else if (res == 0) {
> +error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
> + fn);
> +} else if (sys2vcpu) {
> +vcpu->online = (status != '0');
> +vcpu->can_offline = true;
> +} else if (vcpu->online != (status != '0')) {
> +status = '0' + vcpu->online;
> +if (pwrite(fd, , 1, 0) == -1) {
> +error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
> + fn);
> +}
> +} /* otherwise pretend successful re-(on|off)-lining */
>
> -res = close(dirfd);
> +res = close(fd);
> g_assert(res == 0);
> }
>
> -g_free(dirpath);
> +res = close(dirfd);
> +g_assert(res == 0);
> }
>
> GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> @@ -2107,17 +2102,21 @@ GuestLogicalProcessorList
Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross compilation in generating boot header file
On Thu, Sep 06, 2018 at 09:37:04AM -0400, Wei Huang wrote:
>
>
> - Original Message -
> > From: "Andrew Jones"
> > To: "Wei Huang"
> > Cc: qemu-devel@nongnu.org, lviv...@redhat.com, "peter maydell"
> > , quint...@redhat.com,
> > dgilb...@redhat.com, "alex bennee"
> > Sent: Thursday, September 6, 2018 7:03:32 AM
> > Subject: Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross
> > compilation in generating boot header file
> >
> > On Wed, Sep 05, 2018 at 03:15:32PM -0400, Wei Huang wrote:
> > > Recently a new configure option, CROSS_CC_GUEST, was added to
> > > $(TARGET)-softmmu/config-target.mak to support TCG-related tests. This
> > > patch tries to leverage this option to support cross compilation when the
> > > migration boot block file is being re-generated:
> > >
> > > * The x86 related files are moved to a new sub-dir (named ./i386).
> > > * A new top-layer Makefile is created in tests/migration/ directory.
> > >This Makefile searches and parses CROSS_CC_GUEST to generate
> > >CROSS_PREFIX.
> > >The CROSS_PREFIX, if available, is then passed to
> > >migration/$ARCH/Makefile.
> > >
> > > Reviewed-by: Juan Quintela
> > > Signed-off-by: Wei Huang
> > > ---
> > > tests/migration-test.c | 2 +-
> > > tests/migration/Makefile | 44
> > > --
> > > tests/migration/i386/Makefile | 22 +++
> > > .../{x86-a-b-bootblock.S => i386/a-b-bootblock.S} | 4 --
> > > .../{x86-a-b-bootblock.h => i386/a-b-bootblock.h} | 8 ++--
> > > 5 files changed, 51 insertions(+), 29 deletions(-)
> > > create mode 100644 tests/migration/i386/Makefile
> > > rename tests/migration/{x86-a-b-bootblock.S => i386/a-b-bootblock.S}
> > > (93%)
> > > rename tests/migration/{x86-a-b-bootblock.h => i386/a-b-bootblock.h}
> > > (92%)
> > >
> > > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > > index 0e687b7..fe6b41a 100644
> > > --- a/tests/migration-test.c
> > > +++ b/tests/migration-test.c
> > > @@ -83,7 +83,7 @@ static const char *tmpfs;
> > > /* A simple PC boot sector that modifies memory (1-100MB) quickly
> > > * outputting a 'B' every so often if it's still running.
> > > */
> > > -#include "tests/migration/x86-a-b-bootblock.h"
> > > +#include "tests/migration/i386/a-b-bootblock.h"
> > >
> > > static void init_bootfile_x86(const char *bootpath)
> > > {
> > > diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> > > index c0824b4..a9ed875 100644
> > > --- a/tests/migration/Makefile
> > > +++ b/tests/migration/Makefile
> > > @@ -1,31 +1,35 @@
> > > -# To specify cross compiler prefix, use CROSS_PREFIX=
> > > -# $ make CROSS_PREFIX=x86_64-linux-gnu-
> > > +#
> > > +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> > > +#
> > > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > > later.
> > > +# See the COPYING file in the top-level directory.
> > > +#
> > > +
> > > +TARGET_LIST = i386
> > > +
> > > +SRC_PATH = ../..
> > >
> > > override define __note
> > > -/* This file is automatically generated from
> > > - * tests/migration/x86-a-b-bootblock.S, edit that and then run
> > > - * tests/migration/rebuild-x86-bootblock.sh to update,
> > > - * and then remember to send both in your patch submission.
> > > +/* This file is automatically generated from the assembly file in
> > > + * tests/migration/$@. Edit that file and then run "make all"
> > > + * inside tests/migration to update, and then remember to send both
> > > + * the header and the assembler differences in your patch submission.
> > > */
> > > endef
> > > export __note
> > >
> > > -.PHONY: all clean
> > > -all: x86-a-b-bootblock.h
> > > -
> > > -x86-a-b-bootblock.h: x86.bootsect
> > > - echo "$$__note" > header.tmp
> > > - xxd -i $< | sed -e 's/.*int.*//' >> header.tmp
> > > - mv header.tmp $@
> > > +find-arch-cross-cc = $(lastword $(shell grep -h "CROSS_CC_GUEST="
> > > $(wildcard $(SRC_PATH)/$(patsubst
> > > i386,*86*,$(1))-softmmu/config-target.mak)))
> >
> > The above function hangs unless configuring with
> > '--target-list=x86_64-softmmu,aarch64-softmmu'. I tried just x86_64 alone,
> > just aarch64 alone, and also configuring both x86_64 and i386, but none
> > of those worked. For some reason grep isn't happy with the generated path
> > list. I tested like this
> >
> > ./configure --target-list=x86_64-softmmu,i386-softmmu
> > make -C tests/migration
> >
> > And, while not an issue of this series, I had to manually add
> > CROSS_CC_GUEST="aarch64-linux-gnu-gcc" to aarch64-softmmu/config-target.mak
> > for it to work, because configure's compiler test fails with the Fedora
> > aarch64-linux-gnu-gcc installation (the linker can't find code it needs
> > to a build a program with main()). I'm not sure if building programs with
> > main() is something the CROSS_CC_GUEST compiler needs to do. Maybe that
> > test can be relaxed. Alex?
>
>
Re: [Qemu-devel] Some confusion about live migration of usb device
On Thu, Sep 06, 2018 at 12:10:08PM +, linzhecheng wrote: > You had said that copying vmstate of usb-host is pointless, so just unpulg > and plug it after migration is all right, > but will other usb devices like usb-storage devices lose pending USBPackets > then? Ah, emulated usb devices. The usb host adapters will re-submit any unfinished usb transfers on the target machine. cheers, Gerd
Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross compilation in generating boot header file
- Original Message -
> From: "Andrew Jones"
> To: "Wei Huang"
> Cc: qemu-devel@nongnu.org, lviv...@redhat.com, "peter maydell"
> , quint...@redhat.com,
> dgilb...@redhat.com, "alex bennee"
> Sent: Thursday, September 6, 2018 7:03:32 AM
> Subject: Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross
> compilation in generating boot header file
>
> On Wed, Sep 05, 2018 at 03:15:32PM -0400, Wei Huang wrote:
> > Recently a new configure option, CROSS_CC_GUEST, was added to
> > $(TARGET)-softmmu/config-target.mak to support TCG-related tests. This
> > patch tries to leverage this option to support cross compilation when the
> > migration boot block file is being re-generated:
> >
> > * The x86 related files are moved to a new sub-dir (named ./i386).
> > * A new top-layer Makefile is created in tests/migration/ directory.
> >This Makefile searches and parses CROSS_CC_GUEST to generate
> >CROSS_PREFIX.
> >The CROSS_PREFIX, if available, is then passed to
> >migration/$ARCH/Makefile.
> >
> > Reviewed-by: Juan Quintela
> > Signed-off-by: Wei Huang
> > ---
> > tests/migration-test.c | 2 +-
> > tests/migration/Makefile | 44
> > --
> > tests/migration/i386/Makefile | 22 +++
> > .../{x86-a-b-bootblock.S => i386/a-b-bootblock.S} | 4 --
> > .../{x86-a-b-bootblock.h => i386/a-b-bootblock.h} | 8 ++--
> > 5 files changed, 51 insertions(+), 29 deletions(-)
> > create mode 100644 tests/migration/i386/Makefile
> > rename tests/migration/{x86-a-b-bootblock.S => i386/a-b-bootblock.S} (93%)
> > rename tests/migration/{x86-a-b-bootblock.h => i386/a-b-bootblock.h} (92%)
> >
> > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > index 0e687b7..fe6b41a 100644
> > --- a/tests/migration-test.c
> > +++ b/tests/migration-test.c
> > @@ -83,7 +83,7 @@ static const char *tmpfs;
> > /* A simple PC boot sector that modifies memory (1-100MB) quickly
> > * outputting a 'B' every so often if it's still running.
> > */
> > -#include "tests/migration/x86-a-b-bootblock.h"
> > +#include "tests/migration/i386/a-b-bootblock.h"
> >
> > static void init_bootfile_x86(const char *bootpath)
> > {
> > diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> > index c0824b4..a9ed875 100644
> > --- a/tests/migration/Makefile
> > +++ b/tests/migration/Makefile
> > @@ -1,31 +1,35 @@
> > -# To specify cross compiler prefix, use CROSS_PREFIX=
> > -# $ make CROSS_PREFIX=x86_64-linux-gnu-
> > +#
> > +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > +# See the COPYING file in the top-level directory.
> > +#
> > +
> > +TARGET_LIST = i386
> > +
> > +SRC_PATH = ../..
> >
> > override define __note
> > -/* This file is automatically generated from
> > - * tests/migration/x86-a-b-bootblock.S, edit that and then run
> > - * tests/migration/rebuild-x86-bootblock.sh to update,
> > - * and then remember to send both in your patch submission.
> > +/* This file is automatically generated from the assembly file in
> > + * tests/migration/$@. Edit that file and then run "make all"
> > + * inside tests/migration to update, and then remember to send both
> > + * the header and the assembler differences in your patch submission.
> > */
> > endef
> > export __note
> >
> > -.PHONY: all clean
> > -all: x86-a-b-bootblock.h
> > -
> > -x86-a-b-bootblock.h: x86.bootsect
> > - echo "$$__note" > header.tmp
> > - xxd -i $< | sed -e 's/.*int.*//' >> header.tmp
> > - mv header.tmp $@
> > +find-arch-cross-cc = $(lastword $(shell grep -h "CROSS_CC_GUEST="
> > $(wildcard $(SRC_PATH)/$(patsubst
> > i386,*86*,$(1))-softmmu/config-target.mak)))
>
> The above function hangs unless configuring with
> '--target-list=x86_64-softmmu,aarch64-softmmu'. I tried just x86_64 alone,
> just aarch64 alone, and also configuring both x86_64 and i386, but none
> of those worked. For some reason grep isn't happy with the generated path
> list. I tested like this
>
> ./configure --target-list=x86_64-softmmu,i386-softmmu
> make -C tests/migration
>
> And, while not an issue of this series, I had to manually add
> CROSS_CC_GUEST="aarch64-linux-gnu-gcc" to aarch64-softmmu/config-target.mak
> for it to work, because configure's compiler test fails with the Fedora
> aarch64-linux-gnu-gcc installation (the linker can't find code it needs
> to a build a program with main()). I'm not sure if building programs with
> main() is something the CROSS_CC_GUEST compiler needs to do. Maybe that
> test can be relaxed. Alex?
I saw the same problem and it needs to be addressed in ./configure file. Most
distros don't ship with
cross-compiled glibc. So compiling main() will fail because of that.
>
> Thanks,
> drew
>
> > +parse-cross-prefix = $(subst gcc,,$(patsubst cc,gcc,$(patsubst
> >
[Qemu-devel] [PATCH v5 12/16] qapi/block-commit: expose new job properties
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
blockdev.c | 8
qapi/block-core.json | 16 +++-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index ec90eb1cf9..98b91e75a7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3204,6 +3204,8 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_filter_node_name, const char *filter_node_name,
+ bool has_auto_finalize, bool auto_finalize,
+ bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
{
BlockDriverState *bs;
@@ -3223,6 +3225,12 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
if (!has_filter_node_name) {
filter_node_name = NULL;
}
+if (has_auto_finalize && !auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (has_auto_dismiss && !auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
/* Important Note:
* libvirt relies on the DeviceNotFound error class in order to probe for
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37afdc..d5b62e50d7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1498,6 +1498,19 @@
#above @top. If this option is not given, a node name is
#autogenerated. (Since: 2.9)
#
+# @auto-finalize: When false, this job will wait in a PENDING state after it
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the
query
+#list without user intervention.
+#Defaults to true. (Since 3.1)
+#
# Returns: Nothing on success
# If @device does not exist, DeviceNotFound
# Any other error returns a GenericError.
@@ -1515,7 +1528,8 @@
{ 'command': 'block-commit',
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
'*backing-file': 'str', '*speed': 'int',
-'*filter-node-name': 'str' } }
+'*filter-node-name': 'str',
+'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
# @drive-backup:
--
2.14.4
[Qemu-devel] [PATCH v5 13/16] qapi/block-mirror: expose new job properties
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
blockdev.c | 14 ++
qapi/block-core.json | 30 --
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 98b91e75a7..429cdf9901 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3597,6 +3597,8 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
bool has_filter_node_name,
const char *filter_node_name,
bool has_copy_mode, MirrorCopyMode
copy_mode,
+ bool has_auto_finalize, bool auto_finalize,
+ bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
{
int job_flags = JOB_DEFAULT;
@@ -3625,6 +3627,12 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
if (!has_copy_mode) {
copy_mode = MIRROR_COPY_MODE_BACKGROUND;
}
+if (has_auto_finalize && !auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (has_auto_dismiss && !auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64))
{
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -3802,6 +3810,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
arg->has_unmap, arg->unmap,
false, NULL,
arg->has_copy_mode, arg->copy_mode,
+ arg->has_auto_finalize, arg->auto_finalize,
+ arg->has_auto_dismiss, arg->auto_dismiss,
_err);
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -3823,6 +3833,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
*job_id,
bool has_filter_node_name,
const char *filter_node_name,
bool has_copy_mode, MirrorCopyMode copy_mode,
+ bool has_auto_finalize, bool auto_finalize,
+ bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
{
BlockDriverState *bs;
@@ -3856,6 +3868,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
*job_id,
true, true,
has_filter_node_name, filter_node_name,
has_copy_mode, copy_mode,
+ has_auto_finalize, auto_finalize,
+ has_auto_dismiss, auto_dismiss,
_err);
error_propagate(errp, local_err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d5b62e50d7..e785c2e9fe 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1729,6 +1729,18 @@
# @copy-mode: when to copy data to the destination; defaults to 'background'
# (Since: 3.0)
#
+# @auto-finalize: When false, this job will wait in a PENDING state after it
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the
query
+#list without user intervention.
+#Defaults to true. (Since 3.1)
# Since: 1.3
##
{ 'struct': 'DriveMirror',
@@ -1738,7 +1750,8 @@
'*speed': 'int', '*granularity': 'uint32',
'*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError',
-'*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
+'*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
+'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
# @BlockDirtyBitmap:
@@ -2004,6 +2017,18 @@
# @copy-mode: when to copy data to the destination; defaults to 'background'
# (Since: 3.0)
#
+# @auto-finalize: When false, this job will wait in a PENDING state after it
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically
[Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings
Presently only the backup job really guarantees what one would consider
transactional semantics. To guard against someone helpfully adding them
in the future, document that there are shortcomings in the model that
would need to be audited at that time.
Signed-off-by: John Snow
---
blockdev.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 0cf8febe6c..d4b42403df 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2182,7 +2182,13 @@ static const BlkActionOps actions[] = {
.instance_size = sizeof(BlockDirtyBitmapState),
.prepare = block_dirty_bitmap_disable_prepare,
.abort = block_dirty_bitmap_disable_abort,
- }
+},
+/* Where are transactions for MIRROR, COMMIT and STREAM?
+ * Although these blockjobs use transaction callbacks like the backup job,
+ * these jobs do not necessarily adhere to transaction semantics.
+ * These jobs may not fully undo all of their actions on abort, nor do they
+ * necessarily work in transactions with more than one job in them.
+ */
};
/**
--
2.14.4
[Qemu-devel] [PATCH v5 07/16] block/stream: refactor stream to use job callbacks
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
block/stream.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 700eb239e4..81a7ec8ece 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
return blk_co_preadv(blk, offset, qiov.size, , BDRV_REQ_COPY_ON_READ);
}
-static void stream_exit(Job *job)
+static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
BlockDriverState *base = s->base;
Error *local_err = NULL;
-int ret = job->ret;
+int ret = 0;
-if (!job_is_cancelled(job) && bs->backing && ret == 0) {
+if (bs->backing) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
base_id = s->backing_file_str;
@@ -75,12 +75,19 @@ static void stream_exit(Job *job)
bdrv_set_backing_hd(bs, base, _err);
if (local_err) {
error_report_err(local_err);
-ret = -EPERM;
-goto out;
+return -EPERM;
}
}
-out:
+return ret;
+}
+
+static void stream_clean(Job *job)
+{
+StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+BlockJob *bjob = >common;
+BlockDriverState *bs = blk_bs(bjob->blk);
+
/* Reopen the image back in read-only mode if necessary */
if (s->bs_flags != bdrv_get_flags(bs)) {
/* Give up write permissions before making it read-only */
@@ -89,7 +96,6 @@ out:
}
g_free(s->backing_file_str);
-job->ret = ret;
}
static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
.job_type = JOB_TYPE_STREAM,
.free = block_job_free,
.run = stream_run,
-.exit = stream_exit,
+.prepare = stream_prepare,
+.clean = stream_clean,
.user_resume = block_job_user_resume,
.drain = block_job_drain,
},
--
2.14.4
[Qemu-devel] [PATCH v5 14/16] qapi/block-stream: expose new job properties
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
blockdev.c | 9 +
hmp.c| 5 +++--
qapi/block-core.json | 16 +++-
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 429cdf9901..0cf8febe6c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3116,6 +3116,8 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
+ bool has_auto_finalize, bool auto_finalize,
+ bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
{
BlockDriverState *bs, *iter;
@@ -3185,6 +3187,13 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
/* backing_file string overrides base bs filename */
base_name = has_backing_file ? backing_file : base_name;
+if (has_auto_finalize && !auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (has_auto_dismiss && !auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
+
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
job_flags, has_speed ? speed : 0, on_error, _err);
if (local_err) {
diff --git a/hmp.c b/hmp.c
index 4975fa56b0..868c1a049d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1905,8 +1905,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
- false, NULL, qdict_haskey(qdict, "speed"), speed,
- true, BLOCKDEV_ON_ERROR_REPORT, );
+ false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+ BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
+ );
hmp_handle_error(mon, );
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e785c2e9fe..f877e9e414 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2317,6 +2317,19 @@
#'stop' and 'enospc' can only be used if the block device
#supports io-status (see BlockInfo). Since 1.3.
#
+# @auto-finalize: When false, this job will wait in a PENDING state after it
has
+# finished its work, waiting for @block-job-finalize before
+# making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true. (Since 3.1)
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the
query
+#list without user intervention.
+#Defaults to true. (Since 3.1)
+#
# Returns: Nothing on success. If @device does not exist, DeviceNotFound.
#
# Since: 1.1
@@ -2332,7 +2345,8 @@
{ 'command': 'block-stream',
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
-'*on-error': 'BlockdevOnError' } }
+'*on-error': 'BlockdevOnError',
+'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
# @block-job-set-speed:
--
2.14.4
[Qemu-devel] [PATCH v5 15/16] block/backup: qapi documentation fixup
Fix documentation to match the other jobs amended for 3.1. Signed-off-by: John Snow Reviewed-by: Max Reitz --- qapi/block-core.json | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index f877e9e414..c0b3d33dbb 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1272,13 +1272,14 @@ # a different block device than @device). # # @auto-finalize: When false, this job will wait in a PENDING state after it has -# finished its work, waiting for @block-job-finalize. -# When true, this job will automatically perform its abort or -# commit actions. +# finished its work, waiting for @block-job-finalize before +# making any block graph changes. +# When true, this job will automatically +# perform its abort or commit actions. # Defaults to true. (Since 2.12) # # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it -#has completed ceased all work, and wait for @block-job-dismiss. +#has completely ceased all work, and awaits @block-job-dismiss. #When true, this job will automatically disappear from the query #list without user intervention. #Defaults to true. (Since 2.12) @@ -1327,13 +1328,14 @@ # a different block device than @device). # # @auto-finalize: When false, this job will wait in a PENDING state after it has -# finished its work, waiting for @block-job-finalize. -# When true, this job will automatically perform its abort or -# commit actions. +# finished its work, waiting for @block-job-finalize before +# making any block graph changes. +# When true, this job will automatically +# perform its abort or commit actions. # Defaults to true. (Since 2.12) # # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it -#has completed ceased all work, and wait for @block-job-dismiss. +#has completely ceased all work, and awaits @block-job-dismiss. #When true, this job will automatically disappear from the query #list without user intervention. #Defaults to true. (Since 2.12) -- 2.14.4
[Qemu-devel] [PATCH v5 08/16] tests/blockjob: replace Blockjob with Job
These tests don't actually test blockjobs anymore, they test
generic Job lifetimes. Change the types accordingly.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
tests/test-blockjob.c | 98 ++-
1 file changed, 50 insertions(+), 48 deletions(-)
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index ad4a65bc78..8e8b680416 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -206,18 +206,20 @@ static const BlockJobDriver test_cancel_driver = {
},
};
-static CancelJob *create_common(BlockJob **pjob)
+static CancelJob *create_common(Job **pjob)
{
BlockBackend *blk;
-BlockJob *job;
+Job *job;
+BlockJob *bjob;
CancelJob *s;
blk = create_blk(NULL);
-job = mk_job(blk, "Steve", _cancel_driver, true,
- JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
-job_ref(>job);
-assert(job->job.status == JOB_STATUS_CREATED);
-s = container_of(job, CancelJob, common);
+bjob = mk_job(blk, "Steve", _cancel_driver, true,
+ JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
+job = >job;
+job_ref(job);
+assert(job->status == JOB_STATUS_CREATED);
+s = container_of(bjob, CancelJob, common);
s->blk = blk;
*pjob = job;
@@ -242,7 +244,7 @@ static void cancel_common(CancelJob *s)
static void test_cancel_created(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
@@ -251,119 +253,119 @@ static void test_cancel_created(void)
static void test_cancel_running(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
cancel_common(s);
}
static void test_cancel_paused(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
-job_user_pause(>job, _abort);
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_PAUSED);
+job_user_pause(job, _abort);
+job_enter(job);
+assert(job->status == JOB_STATUS_PAUSED);
cancel_common(s);
}
static void test_cancel_ready(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
s->should_converge = true;
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_READY);
+job_enter(job);
+assert(job->status == JOB_STATUS_READY);
cancel_common(s);
}
static void test_cancel_standby(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
s->should_converge = true;
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_READY);
+job_enter(job);
+assert(job->status == JOB_STATUS_READY);
-job_user_pause(>job, _abort);
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_STANDBY);
+job_user_pause(job, _abort);
+job_enter(job);
+assert(job->status == JOB_STATUS_STANDBY);
cancel_common(s);
}
static void test_cancel_pending(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
s->should_converge = true;
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_READY);
+job_enter(job);
+assert(job->status == JOB_STATUS_READY);
-job_complete(>job, _abort);
-job_enter(>job);
+job_complete(job, _abort);
+job_enter(job);
while (!s->completed) {
aio_poll(qemu_get_aio_context(), true);
}
-assert(job->job.status == JOB_STATUS_PENDING);
+assert(job->status == JOB_STATUS_PENDING);
cancel_common(s);
}
static void test_cancel_concluded(void)
{
-BlockJob *job;
+Job *job;
CancelJob *s;
s = create_common();
-job_start(>job);
-assert(job->job.status == JOB_STATUS_RUNNING);
+job_start(job);
+assert(job->status == JOB_STATUS_RUNNING);
s->should_converge = true;
-job_enter(>job);
-assert(job->job.status == JOB_STATUS_READY);
+job_enter(job);
+assert(job->status == JOB_STATUS_READY);
-job_complete(>job, _abort);
-job_enter(>job);
+job_complete(job, _abort);
+job_enter(job);
while (!s->completed) {
aio_poll(qemu_get_aio_context(), true);
}
-assert(job->job.status == JOB_STATUS_PENDING);
+assert(job->status ==
[Qemu-devel] [PATCH v5 02/16] block/mirror: add block job creation flags
Add support for taking and passing forward job creation flags.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
---
block/mirror.c| 5 +++--
blockdev.c| 3 ++-
include/block/block_int.h | 5 -
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index b8941db6c1..cba555b4ef 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1639,7 +1639,8 @@ fail:
void mirror_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *target, const char *replaces,
- int64_t speed, uint32_t granularity, int64_t buf_size,
+ int creation_flags, int64_t speed,
+ uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
@@ -1655,7 +1656,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
}
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
+mirror_start_job(job_id, bs, creation_flags, target, replaces,
speed, granularity, buf_size, backing_mode,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
diff --git a/blockdev.c b/blockdev.c
index c15a1e624b..6574356708 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3590,6 +3590,7 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
bool has_copy_mode, MirrorCopyMode
copy_mode,
Error **errp)
{
+int job_flags = JOB_DEFAULT;
if (!has_speed) {
speed = 0;
@@ -3642,7 +3643,7 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
* and will allow to check whether the node still exist at mirror
completion
*/
mirror_start(job_id, bs, target,
- has_replaces ? replaces : NULL,
+ has_replaces ? replaces : NULL, job_flags,
speed, granularity, buf_size, sync, backing_mode,
on_source_error, on_target_error, unmap, filter_node_name,
copy_mode, errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ffab0b4d3e..b40f0bfc9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1029,6 +1029,8 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
* @target: Block device to write to.
* @replaces: Block graph node name to replace once the mirror is done. Can
*only be used when full mirroring is selected.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ * See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @granularity: The chosen granularity for the dirty bitmap.
* @buf_size: The amount of data that can be in flight at one time.
@@ -1050,7 +1052,8 @@ void commit_active_start(const char *job_id,
BlockDriverState *bs,
*/
void mirror_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *target, const char *replaces,
- int64_t speed, uint32_t granularity, int64_t buf_size,
+ int creation_flags, int64_t speed,
+ uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
--
2.14.4
[Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort
In cases where we abort the block/mirror job, there's no point in
installing the new backing chain before we finish aborting.
Signed-off-by: John Snow
---
block/mirror.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index cba555b4ef..bd3e908710 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
* required before it could become a backing file of target_bs. */
bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
_abort);
-if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
BlockDriverState *backing = s->is_none_mode ? src : s->base;
if (backing_bs(target_bs) != backing) {
bdrv_set_backing_hd(target_bs, backing, _err);
--
2.14.4
[Qemu-devel] [PATCH v5 11/16] jobs: remove .exit callback
Now that all of the jobs use the component finalization callbacks,
there's no use for the heavy-hammer .exit callback anymore.
job_exit becomes a glorified type shim so that we can call
job_completed from aio_bh_schedule_oneshot.
Move these three functions down into job.c to eliminate a
forward reference.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
include/qemu/job.h | 11
job.c | 77 --
2 files changed, 34 insertions(+), 54 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index e0cff702b7..5cb0681834 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -221,17 +221,6 @@ struct JobDriver {
*/
void (*drain)(Job *job);
-/**
- * If the callback is not NULL, exit will be invoked from the main thread
- * when the job's coroutine has finished, but before transactional
- * convergence; before @prepare or @abort.
- *
- * FIXME TODO: This callback is only temporary to transition remaining jobs
- * to prepare/commit/abort/clean callbacks and will be removed before 3.1.
- * is released.
- */
-void (*exit)(Job *job);
-
/**
* If the callback is not NULL, prepare will be invoked when all the jobs
* belonging to the same transaction complete; or upon this job's
completion
diff --git a/job.c b/job.c
index 01dd97fee3..72f7de1f36 100644
--- a/job.c
+++ b/job.c
@@ -535,49 +535,6 @@ void job_drain(Job *job)
}
}
-static void job_completed(Job *job);
-
-static void job_exit(void *opaque)
-{
-Job *job = (Job *)opaque;
-AioContext *aio_context = job->aio_context;
-
-if (job->driver->exit) {
-aio_context_acquire(aio_context);
-job->driver->exit(job);
-aio_context_release(aio_context);
-}
-job_completed(job);
-}
-
-/**
- * All jobs must allow a pause point before entering their job proper. This
- * ensures that jobs can be paused prior to being started, then resumed later.
- */
-static void coroutine_fn job_co_entry(void *opaque)
-{
-Job *job = opaque;
-
-assert(job && job->driver && job->driver->run);
-job_pause_point(job);
-job->ret = job->driver->run(job, >err);
-job->deferred_to_main_loop = true;
-aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
-}
-
-
-void job_start(Job *job)
-{
-assert(job && !job_started(job) && job->paused &&
- job->driver && job->driver->run);
-job->co = qemu_coroutine_create(job_co_entry, job);
-job->pause_count--;
-job->busy = true;
-job->paused = false;
-job_state_transition(job, JOB_STATUS_RUNNING);
-aio_co_enter(job->aio_context, job->co);
-}
-
/* Assumes the block_job_mutex is held */
static bool job_timer_not_pending(Job *job)
{
@@ -894,6 +851,40 @@ static void job_completed(Job *job)
}
}
+/** Useful only as a type shim for aio_bh_schedule_oneshot. */
+static void job_exit(void *opaque)
+{
+Job *job = (Job *)opaque;
+job_completed(job);
+}
+
+/**
+ * All jobs must allow a pause point before entering their job proper. This
+ * ensures that jobs can be paused prior to being started, then resumed later.
+ */
+static void coroutine_fn job_co_entry(void *opaque)
+{
+Job *job = opaque;
+
+assert(job && job->driver && job->driver->run);
+job_pause_point(job);
+job->ret = job->driver->run(job, >err);
+job->deferred_to_main_loop = true;
+aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
+}
+
+void job_start(Job *job)
+{
+assert(job && !job_started(job) && job->paused &&
+ job->driver && job->driver->run);
+job->co = qemu_coroutine_create(job_co_entry, job);
+job->pause_count--;
+job->busy = true;
+job->paused = false;
+job_state_transition(job, JOB_STATUS_RUNNING);
+aio_co_enter(job->aio_context, job->co);
+}
+
void job_cancel(Job *job, bool force)
{
if (job->status == JOB_STATUS_CONCLUDED) {
--
2.14.4
[Qemu-devel] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean
The exit callback in this test actually only performs cleanup.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
tests/test-blockjob-txn.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index ef29f35e44..86606f92b3 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -24,7 +24,7 @@ typedef struct {
int *result;
} TestBlockJob;
-static void test_block_job_exit(Job *job)
+static void test_block_job_clean(Job *job)
{
BlockJob *bjob = container_of(job, BlockJob, job);
BlockDriverState *bs = blk_bs(bjob->blk);
@@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
.user_resume = block_job_user_resume,
.drain = block_job_drain,
.run = test_block_job_run,
-.exit = test_block_job_exit,
+.clean = test_block_job_clean,
},
};
--
2.14.4
[Qemu-devel] [PATCH v5 01/16] block/commit: add block job creation flags
Add support for taking and passing forward job creation flags.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
---
block/commit.c| 5 +++--
blockdev.c| 7 ---
include/block/block_int.h | 5 -
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index da69165de3..b6e8969877 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -249,7 +249,8 @@ static BlockDriver bdrv_commit_top = {
};
void commit_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, BlockDriverState *top, int64_t speed,
+ BlockDriverState *base, BlockDriverState *top,
+ int creation_flags, int64_t speed,
BlockdevOnError on_error, const char *backing_file_str,
const char *filter_node_name, Error **errp)
{
@@ -267,7 +268,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
s = block_job_create(job_id, _job_driver, NULL, bs, 0, BLK_PERM_ALL,
- speed, JOB_DEFAULT, NULL, NULL, errp);
+ speed, creation_flags, NULL, NULL, errp);
if (!s) {
return;
}
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..c15a1e624b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3214,6 +3214,7 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
* BlockdevOnError change for blkmirror makes it in
*/
BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
+int job_flags = JOB_DEFAULT;
if (!has_speed) {
speed = 0;
@@ -3295,15 +3296,15 @@ void qmp_block_commit(bool has_job_id, const char
*job_id, const char *device,
goto out;
}
commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
-JOB_DEFAULT, speed, on_error,
+job_flags, speed, on_error,
filter_node_name, NULL, NULL, false, _err);
} else {
BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp))
{
goto out;
}
-commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
- on_error, has_backing_file ? backing_file : NULL,
+commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs,
job_flags,
+ speed, on_error, has_backing_file ? backing_file : NULL,
filter_node_name, _err);
}
if (local_err != NULL) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 903b9c1034..ffab0b4d3e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -980,6 +980,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* @bs: Active block device.
* @top: Top block device to be committed.
* @base: Block device that will be written into, and become the new top.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ * See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
* @backing_file_str: String to use as the backing file in @top's overlay
@@ -990,7 +992,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
*
*/
void commit_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, BlockDriverState *top, int64_t speed,
+ BlockDriverState *base, BlockDriverState *top,
+ int creation_flags, int64_t speed,
BlockdevOnError on_error, const char *backing_file_str,
const char *filter_node_name, Error **errp);
/**
--
2.14.4
[Qemu-devel] [PATCH v5 00/16] jobs: Job Exit Refactoring Pt 2
This is part two of a two part series that refactors the exit logic of jobs. This series forces all jobs to use the "finalize" semantics that were introduced previously, but only exposed via the backup jobs. Patches 1-3 add plumbing for the auto-dismiss and auto-finalize flags but do not expose these via QAPI/QMP. Patches 4-7 refactor the .exit() callbacks into the component pieces of .prepare(), .commit(), .abort() and .clean(). Except mirror, which I cheat with. Patches 8-10 remove the last usages of .exit in a test. Patche 11 removes the .exit callback and the machinery to invoke it. Patches 12-14 expose the new QMP options to all of the jobs. Patch 15 is a doc fixup. "V5" Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/16:[] [--] 'block/commit: add block job creation flags' 002/16:[] [--] 'block/mirror: add block job creation flags' 003/16:[] [--] 'block/stream: add block job creation flags' 004/16:[0002] [FC] 'block/commit: refactor commit to use job callbacks' 005/16:[0005] [FC] 'block/mirror: don't install backing chain on abort' 006/16:[0005] [FC] 'block/mirror: conservative mirror_exit refactor' 007/16:[] [--] 'block/stream: refactor stream to use job callbacks' 008/16:[] [--] 'tests/blockjob: replace Blockjob with Job' 009/16:[] [--] 'tests/test-blockjob: remove exit callback' 010/16:[] [--] 'tests/test-blockjob-txn: move .exit to .clean' 011/16:[] [--] 'jobs: remove .exit callback' 012/16:[] [--] 'qapi/block-commit: expose new job properties' 013/16:[] [--] 'qapi/block-mirror: expose new job properties' 014/16:[] [--] 'qapi/block-stream: expose new job properties' 015/16:[] [--] 'block/backup: qapi documentation fixup' 016/16:[down] 'blockdev: document transactional shortcomings' 004: Add FIXME comment (Jeff) 005: Update commit message, move ternary touchup to next patch (Max) 006: Adopt ternary fixup from previous patch (Max) 016: New, does what it says on the tin (Max) "V4" 002, 003: Fix typo (Eric) 005: Don't not install the backing chain if we cancel a successful job (Max) 006: Fallout from 005 007: Fixed commit title. (Max) "V3" 000: Fixed my cover letter subject. Phew! 004: Adjusted comment, added R-B 005: New; based on discussions from what is now 006 (was 005) 006: mirror_exit_common returns ret == 0 if we are aborting and it aborts successfully (Max) Added assertion that we aborted successfully (Max) 010: New, minor cleanup before we can actually remove .exit. 011: Removed what's now patch 10, added R-B per Max's comment. = "V2": = - Split off the first part of the series to Pt.1 - More aggressively refactored .commit() - Went all the way to deleting .exit() callback (Kevin) John Snow (16): block/commit: add block job creation flags block/mirror: add block job creation flags block/stream: add block job creation flags block/commit: refactor commit to use job callbacks block/mirror: don't install backing chain on abort block/mirror: conservative mirror_exit refactor block/stream: refactor stream to use job callbacks tests/blockjob: replace Blockjob with Job tests/test-blockjob: remove exit callback tests/test-blockjob-txn: move .exit to .clean jobs: remove .exit callback qapi/block-commit: expose new job properties qapi/block-mirror: expose new job properties qapi/block-stream: expose new job properties block/backup: qapi documentation fixup blockdev: document transactional shortcomings block/commit.c| 97 ++- block/mirror.c| 44 -- block/stream.c| 28 blockdev.c| 52 ++--- hmp.c | 5 +- include/block/block_int.h | 15 -- include/qemu/job.h| 11 - job.c | 77 ++- qapi/block-core.json | 80 +++- tests/test-blockjob-txn.c | 4 +- tests/test-blockjob.c | 114 +++--- 11 files changed, 324 insertions(+), 203 deletions(-) -- 2.14.4
[Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks
Use the component callbacks; prepare, abort, and clean.
NB: prepare is only called when the job has not yet failed;
and abort can be called after prepare.
complete -> prepare -> abort -> clean
complete -> abort -> clean
During refactor, a potential problem with bdrv_drop_intermediate
was identified, The patched behavior is no worse than the pre-patch
behavior, so leave a FIXME for now to be fixed in a future patch.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
block/commit.c | 92 --
1 file changed, 51 insertions(+), 41 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index b6e8969877..a2da5740b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
BlockDriverState *commit_top_bs;
BlockBackend *top;
BlockBackend *base;
+BlockDriverState *base_bs;
BlockdevOnError on_error;
int base_flags;
char *backing_file_str;
@@ -68,61 +69,67 @@ static int coroutine_fn commit_populate(BlockBackend *bs,
BlockBackend *base,
return 0;
}
-static void commit_exit(Job *job)
+static int commit_prepare(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
-BlockJob *bjob = >common;
-BlockDriverState *top = blk_bs(s->top);
-BlockDriverState *base = blk_bs(s->base);
-BlockDriverState *commit_top_bs = s->commit_top_bs;
-bool remove_commit_top_bs = false;
-
-/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
-bdrv_ref(top);
-bdrv_ref(commit_top_bs);
/* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
* the normal backing chain can be restored. */
blk_unref(s->base);
+s->base = NULL;
-if (!job_is_cancelled(job) && job->ret == 0) {
-/* success */
-job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
- s->backing_file_str);
-} else {
-/* XXX Can (or should) we somehow keep 'consistent read' blocked even
- * after the failed/cancelled commit job is gone? If we already wrote
- * something to base, the intermediate images aren't valid any more. */
-remove_commit_top_bs = true;
+/* FIXME: bdrv_drop_intermediate treats total failures and partial failures
+ * identically. Further work is needed to disambiguate these cases. */
+return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
+ s->backing_file_str);
+}
+
+static void commit_abort(Job *job)
+{
+CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+BlockDriverState *top_bs = blk_bs(s->top);
+
+/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
+bdrv_ref(top_bs);
+bdrv_ref(s->commit_top_bs);
+
+if (s->base) {
+blk_unref(s->base);
}
+/* free the blockers on the intermediate nodes so that bdrv_replace_nodes
+ * can succeed */
+block_job_remove_all_bdrv(>common);
+
+/* If bdrv_drop_intermediate() failed (or was not invoked), remove the
+ * commit filter driver from the backing chain now. Do this as the final
+ * step so that the 'consistent read' permission can be granted.
+ *
+ * XXX Can (or should) we somehow keep 'consistent read' blocked even
+ * after the failed/cancelled commit job is gone? If we already wrote
+ * something to base, the intermediate images aren't valid any more. */
+bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
+_abort);
+bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
+ _abort);
+
+bdrv_unref(s->commit_top_bs);
+bdrv_unref(top_bs);
+}
+
+static void commit_clean(Job *job)
+{
+CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+
/* restore base open flags here if appropriate (e.g., change the base back
* to r/o). These reopens do not need to be atomic, since we won't abort
* even on failure here */
-if (s->base_flags != bdrv_get_flags(base)) {
-bdrv_reopen(base, s->base_flags, NULL);
+if (s->base_flags != bdrv_get_flags(s->base_bs)) {
+bdrv_reopen(s->base_bs, s->base_flags, NULL);
}
+
g_free(s->backing_file_str);
blk_unref(s->top);
-
-/* If there is more than one reference to the job (e.g. if called from
- * job_finish_sync()), job_completed() won't free it and therefore the
- * blockers on the intermediate nodes remain. This would cause
- * bdrv_set_backing_hd() to fail. */
-block_job_remove_all_bdrv(bjob);
-
-/* If bdrv_drop_intermediate() didn't already do that, remove the commit
- * filter driver from the backing chain. Do this as the final step so that
- * the 'consistent read' permission can be granted. */
-if (remove_commit_top_bs) {
-bdrv_child_try_set_perm(commit_top_bs->backing,
[Qemu-devel] [PATCH v5 09/16] tests/test-blockjob: remove exit callback
We remove the exit callback and the completed boolean along with it.
We can simulate it just fine by waiting for the job to defer to the
main loop, and then giving it one final kick to get the main loop
portion to run.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
---
tests/test-blockjob.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 8e8b680416..de4c1c20aa 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -160,15 +160,8 @@ typedef struct CancelJob {
BlockBackend *blk;
bool should_converge;
bool should_complete;
-bool completed;
} CancelJob;
-static void cancel_job_exit(Job *job)
-{
-CancelJob *s = container_of(job, CancelJob, common.job);
-s->completed = true;
-}
-
static void cancel_job_complete(Job *job, Error **errp)
{
CancelJob *s = container_of(job, CancelJob, common.job);
@@ -201,7 +194,6 @@ static const BlockJobDriver test_cancel_driver = {
.user_resume = block_job_user_resume,
.drain = block_job_drain,
.run = cancel_job_run,
-.exit = cancel_job_exit,
.complete = cancel_job_complete,
},
};
@@ -335,9 +327,11 @@ static void test_cancel_pending(void)
job_complete(job, _abort);
job_enter(job);
-while (!s->completed) {
+while (!job->deferred_to_main_loop) {
aio_poll(qemu_get_aio_context(), true);
}
+assert(job->status == JOB_STATUS_READY);
+aio_poll(qemu_get_aio_context(), true);
assert(job->status == JOB_STATUS_PENDING);
cancel_common(s);
@@ -359,9 +353,11 @@ static void test_cancel_concluded(void)
job_complete(job, _abort);
job_enter(job);
-while (!s->completed) {
+while (!job->deferred_to_main_loop) {
aio_poll(qemu_get_aio_context(), true);
}
+assert(job->status == JOB_STATUS_READY);
+aio_poll(qemu_get_aio_context(), true);
assert(job->status == JOB_STATUS_PENDING);
job_finalize(job, _abort);
--
2.14.4
[Qemu-devel] [PATCH v5 03/16] block/stream: add block job creation flags
Add support for taking and passing forward job creation flags.
Signed-off-by: John Snow
Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
---
block/stream.c| 5 +++--
blockdev.c| 3 ++-
include/block/block_int.h | 5 -
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 67e1e72e23..700eb239e4 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -214,7 +214,8 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int64_t speed, BlockdevOnError on_error, Error **errp)
+ int creation_flags, int64_t speed,
+ BlockdevOnError on_error, Error **errp)
{
StreamBlockJob *s;
BlockDriverState *iter;
@@ -236,7 +237,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
BLK_PERM_GRAPH_MOD,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
BLK_PERM_WRITE,
- speed, JOB_DEFAULT, NULL, NULL, errp);
+ speed, creation_flags, NULL, NULL, errp);
if (!s) {
goto fail;
}
diff --git a/blockdev.c b/blockdev.c
index 6574356708..ec90eb1cf9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3123,6 +3123,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
AioContext *aio_context;
Error *local_err = NULL;
const char *base_name = NULL;
+int job_flags = JOB_DEFAULT;
if (!has_on_error) {
on_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -3185,7 +3186,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
base_name = has_backing_file ? backing_file : base_name;
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- has_speed ? speed : 0, on_error, _err);
+ job_flags, has_speed ? speed : 0, on_error, _err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b40f0bfc9b..4000d2af45 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -958,6 +958,8 @@ int is_windows_drive(const char *filename);
* flatten the whole backing file chain onto @bs.
* @backing_file_str: The file name that will be written to @bs as the
* the new backing file if the job completes. Ignored if @base is %NULL.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ * See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
* @errp: Error object.
@@ -971,7 +973,8 @@ int is_windows_drive(const char *filename);
*/
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int64_t speed, BlockdevOnError on_error, Error **errp);
+ int creation_flags, int64_t speed,
+ BlockdevOnError on_error, Error **errp);
/**
* commit_start:
--
2.14.4
[Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor
For purposes of minimum code movement, refactor the mirror_exit
callback to use the post-finalization callbacks in a trivial way.
Signed-off-by: John Snow
---
block/mirror.c | 39 ---
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index bd3e908710..a92b4702c5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
int max_iov;
bool initial_zeroing_ongoing;
int in_active_write_counter;
+bool prepared;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
}
}
-static void mirror_exit(Job *job)
+static int mirror_exit_common(Job *job)
{
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
BlockJob *bjob = >common;
@@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
BlockDriverState *target_bs = blk_bs(s->target);
BlockDriverState *mirror_top_bs = s->mirror_top_bs;
Error *local_err = NULL;
-int ret = job->ret;
+bool abort = job->ret < 0;
+int ret = 0;
+
+if (s->prepared) {
+return 0;
+}
+s->prepared = true;
bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
@@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
* required before it could become a backing file of target_bs. */
bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
_abort);
-if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
BlockDriverState *backing = s->is_none_mode ? src : s->base;
if (backing_bs(target_bs) != backing) {
bdrv_set_backing_hd(target_bs, backing, _err);
@@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
aio_context_acquire(replace_aio_context);
}
-if (s->should_complete && ret == 0) {
-BlockDriverState *to_replace = src;
-if (s->to_replace) {
-to_replace = s->to_replace;
-}
+if (s->should_complete && !abort) {
+BlockDriverState *to_replace = s->to_replace ?: src;
if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
@@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
bdrv_unref(mirror_top_bs);
bdrv_unref(src);
-job->ret = ret;
+return ret;
+}
+
+static int mirror_prepare(Job *job)
+{
+return mirror_exit_common(job);
+}
+
+static void mirror_abort(Job *job)
+{
+int ret = mirror_exit_common(job);
+assert(ret == 0);
}
static void mirror_throttle(MirrorBlockJob *s)
@@ -1132,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = {
.user_resume= block_job_user_resume,
.drain = block_job_drain,
.run= mirror_run,
-.exit = mirror_exit,
+.prepare= mirror_prepare,
+.abort = mirror_abort,
.pause = mirror_pause,
.complete = mirror_complete,
},
@@ -1149,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = {
.user_resume= block_job_user_resume,
.drain = block_job_drain,
.run= mirror_run,
-.exit = mirror_exit,
+.prepare= mirror_prepare,
+.abort = mirror_abort,
.pause = mirror_pause,
.complete = mirror_complete,
},
--
2.14.4
[Qemu-devel] [PATCH v2] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
If VM has VCPUs plugged sparselly (for example a VM started with
3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
only cpu0 and cpu2 are present), QGA will rise a error
error: internal error: unable to execute QEMU agent command 'guest-get-vcpus':
open("/sys/devices/system/cpu/cpu1/"): No such file or directory
when
virsh vcpucount FOO --guest
is executed.
Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
Signed-off-by: Igor Mammedov
---
v2:
do not create CPU entry if cpu isn't present
(Laszlo Ersek )
---
qga/commands-posix.c | 115 ++-
1 file changed, 59 insertions(+), 56 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 37e8a2d..42d30f0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2035,61 +2035,56 @@ static long sysconf_exact(int name, const char
*name_str, Error **errp)
* Written members remain unmodified on error.
*/
static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
- Error **errp)
+ char *dirpath, Error **errp)
{
-char *dirpath;
+int fd;
+int res;
int dirfd;
+static const char fn[] = "online";
-dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- vcpu->logical_id);
dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
if (dirfd == -1) {
error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
-} else {
-static const char fn[] = "online";
-int fd;
-int res;
-
-fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
-if (fd == -1) {
-if (errno != ENOENT) {
-error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
-} else if (sys2vcpu) {
-vcpu->online = true;
-vcpu->can_offline = false;
-} else if (!vcpu->online) {
-error_setg(errp, "logical processor #%" PRId64 " can't be "
- "offlined", vcpu->logical_id);
-} /* otherwise pretend successful re-onlining */
-} else {
-unsigned char status;
-
-res = pread(fd, , 1, 0);
-if (res == -1) {
-error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
-} else if (res == 0) {
-error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
- fn);
-} else if (sys2vcpu) {
-vcpu->online = (status != '0');
-vcpu->can_offline = true;
-} else if (vcpu->online != (status != '0')) {
-status = '0' + vcpu->online;
-if (pwrite(fd, , 1, 0) == -1) {
-error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
- fn);
-}
-} /* otherwise pretend successful re-(on|off)-lining */
+return;
+}
-res = close(fd);
-g_assert(res == 0);
-}
+fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
+if (fd == -1) {
+if (errno != ENOENT) {
+error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
+} else if (sys2vcpu) {
+vcpu->online = true;
+vcpu->can_offline = false;
+} else if (!vcpu->online) {
+error_setg(errp, "logical processor #%" PRId64 " can't be "
+ "offlined", vcpu->logical_id);
+} /* otherwise pretend successful re-onlining */
+} else {
+unsigned char status;
+
+res = pread(fd, , 1, 0);
+if (res == -1) {
+error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
+} else if (res == 0) {
+error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
+ fn);
+} else if (sys2vcpu) {
+vcpu->online = (status != '0');
+vcpu->can_offline = true;
+} else if (vcpu->online != (status != '0')) {
+status = '0' + vcpu->online;
+if (pwrite(fd, , 1, 0) == -1) {
+error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
+ fn);
+}
+} /* otherwise pretend successful re-(on|off)-lining */
-res = close(dirfd);
+res = close(fd);
g_assert(res == 0);
}
-g_free(dirpath);
+res = close(dirfd);
+g_assert(res == 0);
}
GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
@@ -2107,17 +2102,21 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error
**errp)
while (local_err == NULL && current < sc_max) {
GuestLogicalProcessor *vcpu;
GuestLogicalProcessorList *entry;
-
-vcpu = g_malloc0(sizeof *vcpu);
-vcpu->logical_id = current++;
-vcpu->has_can_offline = true; /*
Re: [Qemu-devel] [PATCH v8 1/3] qmp: query-current-machine with wakeup-suspend-support
On 09/05/2018 09:21 PM, Michael Roth wrote:
Quoting Daniel Henrique Barboza (2018-07-05 15:08:11)
When issuing the qmp/hmp 'system_wakeup' command, what happens in a
nutshell is:
- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason
and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all
subscribers of wakeup_notifiers receives a notification, vcpus are then
resumed and the wake up QAPI event is fired
Note that this procedure alone doesn't ensure that the guest will awake
from SUSPENDED state - the subscribers of the wake up event must take
action to resume the guest, otherwise the guest will simply reboot.
At this moment, there are only two subscribers of the wake up event: one
in hw/acpi/core.c and another in hw/i386/xen/xen-hvm.c. This means
that system_wakeup does not work as intended with other architectures.
However, only the presence of 'system_wakeup' is required for QGA to
support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment.
This means that the user/management will expect to suspend the guest using
one of those suspend commands and then resume execution using system_wakeup,
regardless of the support offered in system_wakeup in the first place.
This patch creates a new API called query-current-machine [1], that holds
a new flag called 'wakeup-suspend-support', which indicates if the guest
supports wake up from suspend via system_wakeup. The guest is considered
to implement wake-up support if:
- there is at least one subscriber for the wake up event;
and, for the PC machine type:
- ACPI is enabled.
This is the expected output of query-current-machine when running a x86
guest:
{"execute" : "query-current-machine"}
{"return": {"wakeup-suspend-support": true}}
Running the same x86 guest, but with the --no-acpi option:
{"execute" : "query-current-machine"}
{"return": {"wakeup-suspend-support": false}}
This is the output when running a pseries guest:
{"execute" : "query-current-machine"}
{"return": {"wakeup-suspend-support": false}}
With this extra tool, management can avoid situations where a guest
that does not have proper suspend/wake capabilities ends up in
inconsistent state (e.g.
https://github.com/open-power-host-os/qemu/issues/31).
[1] the decision of creating the query-current-machine API is based
on discussions in the QEMU mailing list where it was decided that
query-target wasn't a proper place to store the wake-up flag, neither
was query-machines because this isn't a static property of the
machine object. This new API can then be used to store other
dynamic machine properties that are scattered around the code
ATM. More info at:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04235.html
Reported-by: Balamuruhan S
Signed-off-by: Daniel Henrique Barboza
---
qapi/misc.json | 24
vl.c | 30 ++
2 files changed, 54 insertions(+)
diff --git a/qapi/misc.json b/qapi/misc.json
index 29da7856e3..266f88cb09 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2003,6 +2003,30 @@
##
{ 'command': 'query-machines', 'returns': ['MachineInfo'] }
+##
+# @CurrentMachineParams:
+#
+# Information describing the running machine parameters.
+#
+# @wakeup-suspend-support: true if the target supports wake up from
+# suspend
+#
+# Since: 3.0.0
3.1.0 now (hopefully :))
+##
+{ 'struct': 'CurrentMachineParams',
+ 'data': { 'wakeup-suspend-support': 'bool'} }
+
+##
+# @query-current-machine:
+#
+# Return a list of parameters of the running machine.
+#
+# Returns: CurrentMachineParams
+#
+# Since: 3.0.0
+##
+{ 'command': 'query-current-machine', 'returns': 'CurrentMachineParams' }
+
##
# @CpuDefinitionInfo:
#
diff --git a/vl.c b/vl.c
index ef6cfcec40..96ad448d57 100644
--- a/vl.c
+++ b/vl.c
@@ -1747,11 +1747,41 @@ void qemu_system_wakeup_enable(WakeupReason reason,
bool enabled)
}
}
+/*
+ * The existence of a wake-up notifier is being checked in the function
+ * qemu_wakeup_suspend_support and it's used in the logic of the
+ * wakeup-suspend-support flag of QMP 'query-current-machine' command.
+ * The idea of this flag is to indicate whether the guest supports wake-up
+ * from suspend (via system_wakeup QMP/HMP call for example), warning the
+ * user that the guest can't handle both wake-up from suspend and the
+ * suspend itself via QGA guest-suspend-ram and guest-suspend-hybrid (if
+ * it can't wake up, it can't be suspended safely).
+ *
+ * An assumption is made by the wakeup-suspend-support flag that only the
+ * guests that can go to RUN_STATE_SUSPENDED and wake up properly would
+ * be interested in this wakeup_notifier. Adding a wakeup_notifier for
+ * any other reason will break the logic of the wakeup-suspend-support
+ * flag and can lead to user/management confusion about the suspend/wake-up
+ * support of the guest.
+ */
void qemu_register_wakeup_notifier(Notifier *notifier)
{
[Qemu-devel] [PATCH risu] ppc64.risu: Fix pattern for darn
This fixes the pattern for the Deliver A Random Number (darn)
instruction to ensure that the value of the L field, which is
used to determine the type and length of the generated random
number, is never 3 which is currently reserved for future use.
Signed-off-by: Sandipan Das
---
ppc64.risu | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ppc64.risu b/ppc64.risu
index 2018103..a27e4fd 100644
--- a/ppc64.risu
+++ b/ppc64.risu
@@ -277,7 +277,7 @@ DADDQd PPC64LE 11 frtp:5 frap:5 frbp:5 101
# format:X book:I page:79 v3.0 darn Deliver A Random Number
DARN PPC64LE 01 rt:5 000 l:2 01000110 \
-!constraints { $rt != 1 && $rt != 13; }
+!constraints { $rt != 1 && $rt != 13 && $l != 3; }
# format:X book:I page:217 v2.06 dcffix DFP Convert From Fixed
DCFFIX PPC64LE 111011 frt:5 0 frb:5 11001000100
--
2.14.4
[Qemu-devel] [PATCH] target-ppc: Extend HWCAP2 bits for ISA 3.0
This adds the HWCAP2 bit to detect if a linux user process is
running on an ISA 3.0 compliant cpu like POWER9. This can be
verified using a simple test program that prints the value in
the auxiliary vector for AT_HWCAP2 as shown below.
Before:
$ qemu-ppc64le -cpu power8 test
0x8c00
$ qemu-ppc64le -cpu power9 test
0x8c00
After:
$ qemu-ppc64le -cpu power8 test
0x8c00
$ qemu-ppc64le -cpu power9 test
0x8c80
Signed-off-by: Sandipan Das
---
linux-user/elfload.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8638612aec..e97c4cde49 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -710,6 +710,7 @@ enum {
QEMU_PPC_FEATURE2_HAS_EBB = 0x1000, /* Event Base Branching */
QEMU_PPC_FEATURE2_HAS_ISEL = 0x0800, /* Integer Select */
QEMU_PPC_FEATURE2_HAS_TAR = 0x0400, /* Target Address Register */
+QEMU_PPC_FEATURE2_ARCH_3_00 = 0x0080, /* ISA 3.00 */
};
#define ELF_HWCAP get_elf_hwcap()
@@ -764,6 +765,7 @@ static uint32_t get_elf_hwcap2(void)
GET_FEATURE2(PPC2_BCTAR_ISA207, QEMU_PPC_FEATURE2_HAS_TAR);
GET_FEATURE2((PPC2_BCTAR_ISA207 | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 |
PPC2_ISA207S), QEMU_PPC_FEATURE2_ARCH_2_07);
+GET_FEATURE2(PPC2_ISA300, QEMU_PPC_FEATURE2_ARCH_3_00);
#undef GET_FEATURE
#undef GET_FEATURE2
--
2.14.4
[Qemu-devel] [Bug 1771948] Re: aarch64 msr CNTFRQ_EL0
** Changed in: qemu Status: New => Invalid -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1771948 Title: aarch64 msr CNTFRQ_EL0 Status in QEMU: Invalid Bug description: Hello, I'm running qemu 2.12 on a raspberry pi 3 with the command: qemu-system-aarch64 -M raspi3 -serial stdio -kernel executable.bin On my start file (right in the beginning with the highest EL), the following instructions: ldr x0 , =1920 msr CNTFRQ_EL0, x0 and qemu halts on the "msr CNTFRQ_EL0, x0" instruction. I believe this is not a normal behavior. Thank you To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1771948/+subscriptions
Re: [Qemu-devel] [PATCH V9 4/4] tests: Add migration test for aarch64
On Wed, Sep 05, 2018 at 03:15:34PM -0400, Wei Huang wrote: > This patch adds migration test support for aarch64. The test code, which > implements the same functionality as x86, is booted as a kernel in qemu. > Here are the design choices we make for aarch64: > > * We choose this -kernel approach because aarch64 QEMU doesn't provide a >built-in fw like x86 does. So instead of relying on a boot loader, we >use -kernel approach for aarch64. > * The serial output is sent to PL011 directly. > * The physical memory base for mach-virt machine is 0x4000. We change >the start_address and end_address for aarch64. > > In addition to providing the binary, this patch also includes the source > code and the build script in tests/migration/aarch64. So users can change > the source and/or re-compile the binary as they wish. > > Reviewed-by: Juan Quintela > Signed-off-by: Wei Huang > --- > tests/Makefile.include | 1 + > tests/migration-test.c | 27 +++-- > tests/migration/Makefile | 2 +- > tests/migration/aarch64/Makefile | 20 ++ > tests/migration/aarch64/a-b-kernel.S | 75 > > tests/migration/aarch64/a-b-kernel.h | 19 + > tests/migration/migration-test.h | 9 + > 7 files changed, 148 insertions(+), 5 deletions(-) > create mode 100644 tests/migration/aarch64/Makefile > create mode 100644 tests/migration/aarch64/a-b-kernel.S > create mode 100644 tests/migration/aarch64/a-b-kernel.h > I see this is already pulled, but just in case it still matters Reviewed-by: Andrew Jones
Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions
On 2018-09-06 07:57, Mark Cave-Ayland wrote: > As part of an upcoming 40p patchset I have a requirement to change the PCI > configuration of the LSI SCSI. However since commits a64aa5785d "hw: > Deprecate -drive > if=scsi with non-onboard HBAs" and b891538e81 "hw/ppc/prep: Fix implicit > creation of > "-drive if=scsi", the lsi53c8*_create() wrapper functions don't return the > device > state itself. > > Rather than altering these functions to return PCIDevice I simply went ahead > and split > the LSIState structure and QOM types into a new lsi53c895a.h file, as is > fairly > standard QEMU practice. > > Not only does this enable me to change the PCI configuration of the LSI SCSI > device > in an upcoming patchset, but it also allows full access to LSIState if > required > (which is fairly similar to how the code was arranged before a64aa5785d). I somehow fail to see that something outside of lsi53c895a.c should really need to access the internals of LSIState. If there is something that needs to be configured from the outside, it should be done via QOM properties instead, shouldn't it? So I think I'd rather prefer if you could do it the other way round and change the lsi*_create() functions to return a pointer to PCIDevice instead, if possible. Thomas
Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
On Thu, 6 Sep 2018 13:52:49 +0200
Laszlo Ersek wrote:
> On 09/06/18 12:50, Igor Mammedov wrote:
> > On Thu, 6 Sep 2018 12:26:12 +0200
> > Laszlo Ersek wrote:
> >
> >> On 09/06/18 11:49, Igor Mammedov wrote:
> >>> On Thu, 30 Aug 2018 17:51:13 +0200
> >>> Laszlo Ersek wrote:
> >>>
> +Drew
>
> On 08/30/18 14:08, Igor Mammedov wrote:
> > If VM has VCPUs plugged sparselly (for example a VM started with
> > 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
> > only cpu0 and cpu2 are present), QGA will rise a error
> > error: internal error: unable to execute QEMU agent command
> > 'guest-get-vcpus':
> > open("/sys/devices/system/cpu/cpu1/"): No such file or directory
> > when
> > virsh vcpucount FOO --guest
> > is executed.
> > Fix it by ignoring non present CPUs when fetching CPUs status from
> > sysfs.
> >
> > Signed-off-by: Igor Mammedov
> > ---
> > qga/commands-posix.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 37e8a2d..2929872 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor
> > *vcpu, bool sys2vcpu,
> >vcpu->logical_id);
> > dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> > if (dirfd == -1) {
> > -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> > +if (!(sys2vcpu && errno == ENOENT)) {
> > +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> > +}
> > } else {
> > static const char fn[] = "online";
> > int fd;
> >
> >>> [...]
> >>>
> I wonder if, instead of this patch, we should rework
> qmp_guest_get_vcpus(), to silently skip processors for which this
> dirpath ENOENT condition arises (i.e., return a shorter list of
> GuestLogicalProcessor objects).
> >>> would something like this on top of this patch do?
> >>>
> >>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >>> index 2929872..990bb80 100644
> >>> --- a/qga/commands-posix.c
> >>> +++ b/qga/commands-posix.c
> >>> @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList
> >>> *qmp_guest_get_vcpus(Error **errp)
> >>> vcpu->logical_id = current++;
> >>> vcpu->has_can_offline = true; /* lolspeak ftw */
> >>> transfer_vcpu(vcpu, true, _err);
> >>> -
> >>> -entry = g_malloc0(sizeof *entry);
> >>> -entry->value = vcpu;
> >>> -
> >>> -*link = entry;
> >>> -link = >next;
> >>> +if (errno == ENOENT) {
> >>> +g_free(vcpu);
> >>> +} else {
> >>> +entry = g_malloc0(sizeof *entry);
> >>> +entry->value = vcpu;
> >>> +*link = entry;
> >>> +link = >next;
> >>> +}
> >>> }
> >>>
> >>> if (local_err == NULL) {
> >>>
> >>> [...]
> >>>
> >>
> >> To me that looks like the right approach, but the details should be
> >> polished a bit:
> >>
> >> - After we drop the vcpu object, "local_err" is still set, and would
> >> terminate the loop in the next iteration.
> > local_error is not set due 'if (!(sys2vcpu && errno == ENOENT))'
> > condition in the in the transfer_vcpu().
>
> ah, sorry, you did say this was on top of your original patch, but I had
> forgotten the details of that.
>
> > the thing is that in case of vcpu2sys direction ENOENT is hard error.
> >
> >> - It seems like ENOENT can indeed only come from openat(), in
> >> transfer_vcpu(), however, it would be nice if we could grab the error
> >> code from the error object somehow, and not from the "errno" variable. I
> >> vaguely recall this is what error classes were originally invented for,
> >> but now we just use ERROR_CLASS_GENERIC_ERROR...
> > I've checked it and errno is preserved during error_setg_errno() call but
> > not saved in Error, so I've dropped that idea.
> >
> >> How about this: we could add a boolean output param to transfer_vcpu(),
> >> called "fatal". Ignored when the function succeeds. When the function
> >> fails (seen from "local_err"), the loop consults "fatal". If the error
> >> is fatal, we act as before; otherwise, we drop the vcpu object, release
> >> -- and zero out -- "local_err" as well, and continue. I think this is
> >> more generic / safer than trying to infer the failure location from the
> >> outside.
> > It looked more uglier to me, so I've turned to libc style of reporting
> > (assuming that g_free() doesn't touch errno ever)
> >
> > But if you prefer using extra parameter, I'll respin patch with it.
instead of inventing not really error parameter
we could move dirpath outside of transfer_vcpu().
It's bigger patch due to code movement but more straightforward logic.
I'll send
Re: [Qemu-devel] Some confusion about live migration of usb device
You had said that copying vmstate of usb-host is pointless, so just unpulg and plug it after migration is all right, but will other usb devices like usb-storage devices lose pending USBPackets then? > -Original Message- > From: gerd hoffmann [mailto:kra...@redhat.com] > Sent: Thursday, September 06, 2018 8:04 PM > To: linzhecheng > Cc: CheneyLin ; wangxin (U) > ; qemu-devel@nongnu.org > Subject: Re: Some confusion about live migration of usb device > > On Thu, Sep 06, 2018 at 10:25:20AM +, linzhecheng wrote: > > Hi, Gerd > > > > I'm going through relevant codes about live migration of usb devices, > > it seems that we will not save/load USBpacket in any vmstate, so > > pending usb packets will be lost after live migration, is it a > > problem? > > With usb-host or usb-redir? > > cheers, > Gerd
Re: [Qemu-devel] Some confusion about live migration of usb device
On Thu, Sep 06, 2018 at 10:25:20AM +, linzhecheng wrote: > Hi, Gerd > > I'm going through relevant codes about live migration of usb devices, > it seems that we will not save/load USBpacket in any vmstate, so > pending usb packets will be lost after live migration, is it a > problem? With usb-host or usb-redir? cheers, Gerd
Re: [Qemu-devel] [PATCH V9 2/4] tests/migration: Support cross compilation in generating boot header file
On Wed, Sep 05, 2018 at 03:15:32PM -0400, Wei Huang wrote:
> Recently a new configure option, CROSS_CC_GUEST, was added to
> $(TARGET)-softmmu/config-target.mak to support TCG-related tests. This
> patch tries to leverage this option to support cross compilation when the
> migration boot block file is being re-generated:
>
> * The x86 related files are moved to a new sub-dir (named ./i386).
> * A new top-layer Makefile is created in tests/migration/ directory.
>This Makefile searches and parses CROSS_CC_GUEST to generate CROSS_PREFIX.
>The CROSS_PREFIX, if available, is then passed to migration/$ARCH/Makefile.
>
> Reviewed-by: Juan Quintela
> Signed-off-by: Wei Huang
> ---
> tests/migration-test.c | 2 +-
> tests/migration/Makefile | 44
> --
> tests/migration/i386/Makefile | 22 +++
> .../{x86-a-b-bootblock.S => i386/a-b-bootblock.S} | 4 --
> .../{x86-a-b-bootblock.h => i386/a-b-bootblock.h} | 8 ++--
> 5 files changed, 51 insertions(+), 29 deletions(-)
> create mode 100644 tests/migration/i386/Makefile
> rename tests/migration/{x86-a-b-bootblock.S => i386/a-b-bootblock.S} (93%)
> rename tests/migration/{x86-a-b-bootblock.h => i386/a-b-bootblock.h} (92%)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 0e687b7..fe6b41a 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -83,7 +83,7 @@ static const char *tmpfs;
> /* A simple PC boot sector that modifies memory (1-100MB) quickly
> * outputting a 'B' every so often if it's still running.
> */
> -#include "tests/migration/x86-a-b-bootblock.h"
> +#include "tests/migration/i386/a-b-bootblock.h"
>
> static void init_bootfile_x86(const char *bootpath)
> {
> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> index c0824b4..a9ed875 100644
> --- a/tests/migration/Makefile
> +++ b/tests/migration/Makefile
> @@ -1,31 +1,35 @@
> -# To specify cross compiler prefix, use CROSS_PREFIX=
> -# $ make CROSS_PREFIX=x86_64-linux-gnu-
> +#
> +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +
> +TARGET_LIST = i386
> +
> +SRC_PATH = ../..
>
> override define __note
> -/* This file is automatically generated from
> - * tests/migration/x86-a-b-bootblock.S, edit that and then run
> - * tests/migration/rebuild-x86-bootblock.sh to update,
> - * and then remember to send both in your patch submission.
> +/* This file is automatically generated from the assembly file in
> + * tests/migration/$@. Edit that file and then run "make all"
> + * inside tests/migration to update, and then remember to send both
> + * the header and the assembler differences in your patch submission.
> */
> endef
> export __note
>
> -.PHONY: all clean
> -all: x86-a-b-bootblock.h
> -
> -x86-a-b-bootblock.h: x86.bootsect
> - echo "$$__note" > header.tmp
> - xxd -i $< | sed -e 's/.*int.*//' >> header.tmp
> - mv header.tmp $@
> +find-arch-cross-cc = $(lastword $(shell grep -h "CROSS_CC_GUEST=" $(wildcard
> $(SRC_PATH)/$(patsubst i386,*86*,$(1))-softmmu/config-target.mak)))
The above function hangs unless configuring with
'--target-list=x86_64-softmmu,aarch64-softmmu'. I tried just x86_64 alone,
just aarch64 alone, and also configuring both x86_64 and i386, but none
of those worked. For some reason grep isn't happy with the generated path
list. I tested like this
./configure --target-list=x86_64-softmmu,i386-softmmu
make -C tests/migration
And, while not an issue of this series, I had to manually add
CROSS_CC_GUEST="aarch64-linux-gnu-gcc" to aarch64-softmmu/config-target.mak
for it to work, because configure's compiler test fails with the Fedora
aarch64-linux-gnu-gcc installation (the linker can't find code it needs
to a build a program with main()). I'm not sure if building programs with
main() is something the CROSS_CC_GUEST compiler needs to do. Maybe that
test can be relaxed. Alex?
Thanks,
drew
> +parse-cross-prefix = $(subst gcc,,$(patsubst cc,gcc,$(patsubst
> CROSS_CC_GUEST="%",%,$(call find-arch-cross-cc,$(1)
> +gen-cross-prefix = $(patsubst %-,CROSS_PREFIX=%-,$(call
> parse-cross-prefix,$(1)))
>
> -x86.bootsect: x86.boot
> - dd if=$< of=$@ bs=256 count=2 skip=124
> +.PHONY: all $(TARGET_LIST)
>
> -x86.boot: x86.o
> - $(CROSS_PREFIX)objcopy -O binary $< $@
> +all: $(TARGET_LIST)
>
> -x86.o: x86-a-b-bootblock.S
> - $(CROSS_PREFIX)gcc -m32 -march=i486 -c $< -o $@
> +$(TARGET_LIST):
> + $(MAKE) -C $@ $(call gen-cross-prefix,$@)
>
> clean:
> - @rm -rf *.boot *.o *.bootsect
> + for target in $(TARGET_LIST); do \
> + $(MAKE) -C $$target clean; \
> + done
> diff --git a/tests/migration/i386/Makefile b/tests/migration/i386/Makefile
> new file mode 100644
> index 000..5c03241
> ---
Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()
On 09/06/18 12:50, Igor Mammedov wrote:
> On Thu, 6 Sep 2018 12:26:12 +0200
> Laszlo Ersek wrote:
>
>> On 09/06/18 11:49, Igor Mammedov wrote:
>>> On Thu, 30 Aug 2018 17:51:13 +0200
>>> Laszlo Ersek wrote:
>>>
+Drew
On 08/30/18 14:08, Igor Mammedov wrote:
> If VM has VCPUs plugged sparselly (for example a VM started with
> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
> only cpu0 and cpu2 are present), QGA will rise a error
> error: internal error: unable to execute QEMU agent command
> 'guest-get-vcpus':
> open("/sys/devices/system/cpu/cpu1/"): No such file or directory
> when
> virsh vcpucount FOO --guest
> is executed.
> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
>
> Signed-off-by: Igor Mammedov
> ---
> qga/commands-posix.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 37e8a2d..2929872 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor
> *vcpu, bool sys2vcpu,
>vcpu->logical_id);
> dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> if (dirfd == -1) {
> -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +if (!(sys2vcpu && errno == ENOENT)) {
> +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +}
> } else {
> static const char fn[] = "online";
> int fd;
>
>>> [...]
>>>
I wonder if, instead of this patch, we should rework
qmp_guest_get_vcpus(), to silently skip processors for which this
dirpath ENOENT condition arises (i.e., return a shorter list of
GuestLogicalProcessor objects).
>>> would something like this on top of this patch do?
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 2929872..990bb80 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList
>>> *qmp_guest_get_vcpus(Error **errp)
>>> vcpu->logical_id = current++;
>>> vcpu->has_can_offline = true; /* lolspeak ftw */
>>> transfer_vcpu(vcpu, true, _err);
>>> -
>>> -entry = g_malloc0(sizeof *entry);
>>> -entry->value = vcpu;
>>> -
>>> -*link = entry;
>>> -link = >next;
>>> +if (errno == ENOENT) {
>>> +g_free(vcpu);
>>> +} else {
>>> +entry = g_malloc0(sizeof *entry);
>>> +entry->value = vcpu;
>>> +*link = entry;
>>> +link = >next;
>>> +}
>>> }
>>>
>>> if (local_err == NULL) {
>>>
>>> [...]
>>>
>>
>> To me that looks like the right approach, but the details should be
>> polished a bit:
>>
>> - After we drop the vcpu object, "local_err" is still set, and would
>> terminate the loop in the next iteration.
> local_error is not set due 'if (!(sys2vcpu && errno == ENOENT))'
> condition in the in the transfer_vcpu().
ah, sorry, you did say this was on top of your original patch, but I had
forgotten the details of that.
> the thing is that in case of vcpu2sys direction ENOENT is hard error.
>
>> - It seems like ENOENT can indeed only come from openat(), in
>> transfer_vcpu(), however, it would be nice if we could grab the error
>> code from the error object somehow, and not from the "errno" variable. I
>> vaguely recall this is what error classes were originally invented for,
>> but now we just use ERROR_CLASS_GENERIC_ERROR...
> I've checked it and errno is preserved during error_setg_errno() call but
> not saved in Error, so I've dropped that idea.
>
>> How about this: we could add a boolean output param to transfer_vcpu(),
>> called "fatal". Ignored when the function succeeds. When the function
>> fails (seen from "local_err"), the loop consults "fatal". If the error
>> is fatal, we act as before; otherwise, we drop the vcpu object, release
>> -- and zero out -- "local_err" as well, and continue. I think this is
>> more generic / safer than trying to infer the failure location from the
>> outside.
> It looked more uglier to me, so I've turned to libc style of reporting
> (assuming that g_free() doesn't touch errno ever)
>
> But if you prefer using extra parameter, I'll respin patch with it.
Michael, what is your preference? I guess I'll be fine both ways.
Thanks,
Laszlo
>
>> I'm not quite up to date on structured error propagation in QEMU, so
>> take the above with a grain of salt...
>>
>> Thanks,
>> Laszlo
>
Re: [Qemu-devel] [PATCH 1/3] scsi: move lsi53c895a structures and defines into separate lsi53c895a.h file
On 2018-09-06 07:57, Mark Cave-Ayland wrote: > There is also one small change to the new header file which is the addition > of the previously missing LSI53C810 define. > > Signed-off-by: Mark Cave-Ayland > --- > hw/scsi/lsi53c895a.c | 116 +--- > include/hw/scsi/lsi53c895a.h | 137 > +++ > 2 files changed, 138 insertions(+), 115 deletions(-) > create mode 100644 include/hw/scsi/lsi53c895a.h [...] > diff --git a/include/hw/scsi/lsi53c895a.h b/include/hw/scsi/lsi53c895a.h > new file mode 100644 > index 00..d80cb78c69 > --- /dev/null > +++ b/include/hw/scsi/lsi53c895a.h > @@ -0,0 +1,137 @@ > +/* > + * QEMU LSI53C895A SCSI Host Bus Adapter emulation > + * > + * Copyright (c) 2006 CodeSourcery. > + * Written by Paul Brook > + * > + * This code is licensed under the LGPL. > + */ > + > +#ifndef LSI_H > +#define LSI_H > + > +#include "qemu/osdep.h" Please don't include osdep.h from a header, that should only be done from .c files. Thanks, Thomas
[Qemu-devel] [PATCH v2 2/3] block/snapshot: remove bdrv_snapshot_delete_by_id_or_name
After the previous patch, the only instance of this function left
is inside qemu-img.c.
qemu-img is using it inside the 'img_snapshot' function to delete
snapshots in the SNAPSHOT_DELETE case, based on a "snapshot_name"
string that refers to the tag, not ID, of the QEMUSnapshotInfo struct.
This can be verified by checking the SNAPSHOT_CREATE case that
comes shortly before SNAPSHOT_DELETE. In that case, the same
"snapshot_name" variable is being strcpy to the 'name' field
of the QEMUSnapshotInfo struct sn:
pstrcpy(sn.name, sizeof(sn.name), snapshot_name);
Based on that, it is unlikely that "snapshot_name" might contain
an "id" in SNAPSHOT_DELETE.
This patch changes SNAPSHOT_DELETE to use snapshot_find() and
snapshot_delete() instead of bdrv_snapshot_delete_by_id_or_name.
After that, there is no instances left of bdrv_snapshot_delete_by_id_or_name
in the code, so it is safe to remove it entirely.
Suggested-by: Murilo Opsfelder Araujo
Signed-off-by: Daniel Henrique Barboza
---
block/snapshot.c | 20
include/block/snapshot.h | 3 ---
qemu-img.c | 15 +++
3 files changed, 11 insertions(+), 27 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index e371d2243d..f2f48f926a 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -301,26 +301,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
return ret;
}
-int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
- const char *id_or_name,
- Error **errp)
-{
-int ret;
-Error *local_err = NULL;
-
-ret = bdrv_snapshot_delete(bs, id_or_name, NULL, _err);
-if (ret == -ENOENT || ret == -EINVAL) {
-error_free(local_err);
-local_err = NULL;
-ret = bdrv_snapshot_delete(bs, NULL, id_or_name, _err);
-}
-
-if (ret < 0) {
-error_propagate(errp, local_err);
-}
-return ret;
-}
-
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info)
{
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index f73d1094af..b5d5084a12 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -61,9 +61,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
const char *name,
Error **errp);
-int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
- const char *id_or_name,
- Error **errp);
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..62bb35f1f0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3121,11 +3121,18 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
-bdrv_snapshot_delete_by_id_or_name(bs, snapshot_name, );
-if (err) {
-error_reportf_err(err, "Could not delete snapshot '%s': ",
- snapshot_name);
+ret = bdrv_snapshot_find(bs, , snapshot_name);
+if (ret < 0) {
+error_report("Could not delete snapshot '%s': snapshot not "
+ "found", snapshot_name);
ret = 1;
+} else {
+ret = bdrv_snapshot_delete(bs, sn.id_str, sn.name, );
+if (ret < 0) {
+error_reportf_err(err, "Could not delete snapshot '%s': ",
+ snapshot_name);
+ret = 1;
+}
}
break;
}
--
2.17.1
[Qemu-devel] [PATCH v2 3/3] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call
In qcow2_snapshot_create there is the following code block:
/* Generate an ID */
find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
/* Check that the ID is unique */
if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
return -EEXIST;
}
find_new_snapshot_id cycles through all snapshots, getting the id_str
as an unsigned long int, calculating the max id_max value of all the
existing id_strs and writing in the id_str pointer id_max + 1:
for(i = 0; i < s->nb_snapshots; i++) {
sn = s->snapshots + i;
id = strtoul(sn->id_str, NULL, 10);
if (id > id_max)
id_max = id;
}
snprintf(id_str, id_str_size, "%lu", id_max + 1);
Here, sn_info->id_str will have the unique value id_max + 1. Right
after that, find_snapshot_by_id_and_name is called with
id = sn_info->id_str and name = NULL. This will cause the function
to execute the following:
} else if (id) {
for (i = 0; i < s->nb_snapshots; i++) {
if (!strcmp(s->snapshots[i].id_str, id)) {
return i;
}
}
}
In short, we're searching the existing snapshots to see if sn_info->id_str
matches any existing id, right after we set in the previous line a
sn_info->id_str value that is already unique.
The first code block goes way back to commit 585f8587ad, a 2006 commit from
Fabrice Bellard that simply says "new qcow2 disk image format". No more
info is provided about this logic in any subsequent commits that moved
this code block around.
I can't say about the original design, but the current logic is redundant.
bdrv_snapshot_create is called in aio_context lock, forbidding any
concurrent call to accidentally create a new snapshot between
the find_new_snapshot_id and find_snapshot_by_id_and_name calls. What
we're ending up doing is to cycle through the snapshots two times
for no viable reason.
This patch eliminates the redundancy by removing the 'id is unique'
check that calls find_snapshot_by_id_and_name.
Signed-off-by: Daniel Henrique Barboza
---
block/qcow2-snapshot.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bb6a5b7516..20e8472191 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -358,11 +358,6 @@ int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
/* Generate an ID */
find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
-/* Check that the ID is unique */
-if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
-return -EEXIST;
-}
-
/* Populate sn with passed data */
sn->id_str = g_strdup(sn_info->id_str);
sn->name = g_strdup(sn_info->name);
--
2.17.1
[Qemu-devel] [PATCH v2 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations
At this moment, QEMU attempts to create/load/delete snapshots
by using either an ID (id_str) or a name. The problem is that the code
isn't consistent of whether the entered argument is an ID or a name,
causing unexpected behaviors.
For example, when creating snapshots via savevm , what happens is that
"arg" is treated as both name and id_str. In a guest without snapshots, create
a single snapshot via savevm:
(qemu) savevm 0
(qemu) info snapshots
List of snapshots present on all disks:
IDTAG VM SIZEDATE VM CLOCK
--0 741M 2018-07-31 13:39:56 00:41:25.313
A snapshot with name "0" is created. ID is hidden from the user, but the
ID is a non-zero integer that starts at "1". Thus, this snapshot has
id_str=1, TAG="0". Creating a second snapshot with arg = 1, the first one
is deleted:
(qemu) savevm 1
(qemu) info snapshots
List of snapshots present on all disks:
IDTAG VM SIZEDATE VM CLOCK
--1 741M 2018-07-31 13:42:14 00:41:55.252
What happened?
- when creating the second snapshot, a verification is done inside
bdrv_all_delete_snapshot to delete any existing snapshots that matches an
string argument. Here, the code calls bdrv_all_delete_snapshot("1", ...);
- bdrv_all_delete_snapshot calls bdrv_snapshot_find(..., "1") for each
BlockDriverState of the guest. And this is where things goes tilting:
bdrv_snapshot_find does a search by both id_str and name. It finds
out that there is a snapshot that has id_str = 1, stores a reference
to the snapshot in the sn_info pointer and then returns match found;
- since a match was found, a call to bdrv_snapshot_delete_by_id_or_name() is
made. This function ignores the pointer written by bdrv_snapshot_find. Instead,
it deletes the snapshot using bdrv_snapshot_delete() calling it first with
id_str = 1. If it fails to delete, then it calls it again with name = 1.
- after all that, QEMU creates the new snapshot, that has id_str = 1 and
name = 1. The user is left wondering that happened with the first snapshot
created. Similar bugs can be triggered when using loadvm and delvm.
Before contemplating discarding the use of ID input in these operations,
I've searched the code of what would be the implications. My findings
are:
- the RBD and Sheepdog drivers don't care. Both uses the 'name' field as
key in their logic, making id_str = name when appropriate.
replay-snapshot.c does not make any special use of id_str;
- qcow2 uses id_str as an unique identifier but it is automatically
calculated, not being influenced by user input. Other than that, there are
no distinguish operations made only with id_str;
- in blockdev.c, the delete operation uses a match of both id_str AND
name. Given that id_str is either a copy of 'name' or auto-generated,
we're fine here.
This gives motivation to not consider ID as a valid user input in HMP
commands - sticking with 'name' input only is more consistent. To
accomplish that, the following changes were made in this patch:
- bdrv_snapshot_find() does not match for id_str anymore, only 'name'. The
function is called in save_snapshot(), load_snapshot(),
bdrv_all_delete_snapshot()
and bdrv_all_find_snapshot(). This change makes the search function more
predictable and does not change the behavior of any underlying code that uses
these affected functions, which are related to HMP (which is fine) and the
main loop inside vl.c (which doesn't care about it anyways);
- bdrv_all_delete_snapshot() does not call bdrv_snapshot_delete_by_id_or_name
anymore. Instead, it uses the pointer returned by bdrv_snapshot_find to
erase the snapshot with the exact match of id_str an name. This function
is called in save_snapshot and hmp_delvm, thus this change produces the
intended effect;
- documentation changes to reflect the new behavior. I consider this to
be an API fix instead of an API change - the user was already creating
snapshots using 'name', but now he/she will also enjoy a consistent
behavior. Libvirt does not care about this change either, as far as
I've tested.
Ideally we would get rid of the id_str field entirely, but this would have
repercussions on existing snapshots. Another day perhaps.
Signed-off-by: Daniel Henrique Barboza
---
block/snapshot.c | 5 +++--
hmp-commands.hx | 24
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 3218a542df..e371d2243d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -63,7 +63,7 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo
*sn_info,
}
for (i = 0; i < nb_sns; i++) {
sn = _tab[i];
-if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+if (!strcmp(sn->name, name)) {
*sn_info = *sn;
ret = 0;
break;
@@ -448,7 +448,8 @@ int bdrv_all_delete_snapshot(const char *name,
[Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore
changes in v2: - removed the "RFC" marker; - added a new patch (patch 2) that removes bdrv_snapshot_delete_by_id_or_name from the code; - made changes in patch 1 as suggested by Murilo; - previous patch set link: https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.html It is not uncommon to see bugs being opened by testers that attempt to create VM snapshots using HMP. It turns out that "0" and "1" are quite common snapshot names and they trigger a lot of bugs. I gave an example in the commit message of patch 1, but to sum up here: QEMU treats the input of savevm/loadvm/delvm sometimes as 'ID', sometimes as 'name'. It is documented as such, but this can lead to strange situations. Given that it is strange for an API to consider a parameter to be 2 fields at the same time, and inadvently treating them as one or the other, and that removing the ID field is too drastic, my idea here is to keep the ID field for internal control, but do not let the user set it. I guess there's room for discussion about considering this change an API change or not. It doesn't affect users of HMP and it doesn't affect Libvirt, but simplifying the meaning of the parameters of savevm/loadvm/delvm. Daniel Henrique Barboza (3): block/snapshot.c: eliminate use of ID input in snapshot operations block/snapshot: remove bdrv_snapshot_delete_by_id_or_name qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call block/qcow2-snapshot.c | 5 - block/snapshot.c | 25 +++-- hmp-commands.hx | 24 include/block/snapshot.h | 3 --- qemu-img.c | 15 +++ 5 files changed, 26 insertions(+), 46 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v6 0/3] migration: compression optimization
guangrong.x...@gmail.com wrote: > From: Xiao Guangrong > > Changelog in v6: > > Thanks to Juan's review, in this version we > 1) move flush compressed data to find_dirty_block() where it hits the end >of memblock > 2) use save_page_use_compression instead of migrate_use_compression in >flush_compressed_data > > Xiao Guangrong (3): > migration: do not flush_compressed_data at the end of iteration > migration: show the statistics of compression > migration: use save_page_use_compression in flush_compressed_data > > hmp.c | 13 +++ > migration/migration.c | 12 ++ > migration/ram.c | 63 > +++ > migration/ram.h | 1 + > qapi/migration.json | 26 - > 5 files changed, 105 insertions(+), 10 deletions(-) queued
