Re: [PATCH] COLO-compare: Fix incorrect `if` logic
OK, thank you all :)
Jason Wang writes:
> On 2019/9/24 下午11:35, Philippe Mathieu-Daudé wrote:
>> Hi Fan,
>>
>> you forgot to Cc the maintainers (doing that for you):
>>
>> ./scripts/get_maintainer.pl -f net/colo-compare.c
>> Zhang Chen (supporter:COLO Proxy)
>> Li Zhijian (supporter:COLO Proxy)
>> Jason Wang (maintainer:Network device ba...)
>> qemu-devel@nongnu.org (open list:All patches CC here)
>>
>> On 9/24/19 4:08 PM, Fan Yang wrote:
>>> 'colo_mark_tcp_pkt' should return 'true' when packets are the same, and
>>> 'false' otherwise. However, it returns 'true' when
>>> 'colo_compare_packet_payload' returns non-zero while
>>> 'colo_compare_packet_payload' is just a 'memcmp'. The result is that
>>> COLO-compare reports inconsistent TCP packets when they are actually
>>> the same.
>>>
>> Fixes: f449c9e549c
>> Reviewed-by: Philippe Mathieu-Daudé
>
>
> Applied.
>
> Thanks
>
>
>>
>>> Signed-off-by: Fan Yang
>>> ---
>>> net/colo-compare.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index 7489840bde..7ee17f2cf8 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -319,7 +319,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet
>>> *spkt,
>>> *mark = 0;
>>>
>>> if (ppkt->tcp_seq == spkt->tcp_seq && ppkt->seq_end == spkt->seq_end) {
>>> -if (colo_compare_packet_payload(ppkt, spkt,
>>> +if (!colo_compare_packet_payload(ppkt, spkt,
>>> ppkt->header_size,
>>> spkt->header_size,
>>> ppkt->payload_size)) {
>>> *mark = COLO_COMPARE_FREE_SECONDARY |
>>> COLO_COMPARE_FREE_PRIMARY;
>>> @@ -329,7 +329,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet
>>> *spkt,
>>>
>>> /* one part of secondary packet payload still need to be compared */
>>> if (!after(ppkt->seq_end, spkt->seq_end)) {
>>> -if (colo_compare_packet_payload(ppkt, spkt,
>>> +if (!colo_compare_packet_payload(ppkt, spkt,
>>> ppkt->header_size + ppkt->offset,
>>> spkt->header_size + spkt->offset,
>>> ppkt->payload_size -
>>> ppkt->offset)) {
>>> @@ -348,7 +348,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet
>>> *spkt,
>>> /* primary packet is longer than secondary packet, compare
>>> * the same part and mark the primary packet offset
>>> */
>>> -if (colo_compare_packet_payload(ppkt, spkt,
>>> +if (!colo_compare_packet_payload(ppkt, spkt,
>>> ppkt->header_size + ppkt->offset,
>>> spkt->header_size + spkt->offset,
>>> spkt->payload_size -
>>> spkt->offset)) {
>>>
Re: [PATCH v4 8/8] hw/i386: Introduce the microvm machine type
Paolo Bonzini writes: > On 24/09/19 14:44, Sergio Lopez wrote: >> microvm.option-roms=bool (Set off to disable loading option ROMs) > > Please make this x-option-roms OK. >> microvm.isa-serial=bool (Set off to disable the instantiation an ISA serial >> port) >> microvm.rtc=bool (Set off to disable the instantiation of an MC146818 RTC) >> microvm.kernel-cmdline=bool (Set off to disable adding virtio-mmio devices >> to the kernel cmdline) > > Perhaps auto-kernel-cmdline? Yeah, that sounds better. Thanks, Sergio. signature.asc Description: PGP signature
Re: [PATCH v4 0/8] Introduce the microvm machine type
Peter Maydell writes: > On Tue, 24 Sep 2019 at 14:25, Sergio Lopez wrote: >> >> Microvm is a machine type inspired by both NEMU and Firecracker, and >> constructed after the machine model implemented by the latter. >> >> It's main purpose is providing users a minimalist machine type free >> from the burden of legacy compatibility, serving as a stepping stone >> for future projects aiming at improving boot times, reducing the >> attack surface and slimming down QEMU's footprint. > > >> docs/microvm.txt | 78 +++ > > I'm not sure how close to acceptance this patchset is at the > moment, so not necessarily something you need to do now, > but could new documentation in docs/ be in rst format, not > plain text, please? (Ideally also they should be in the right > manual subdirectory, but documentation of system emulation > machines at the moment is still in texinfo format, so we > don't have a subdir for it yet.) Sure. What I didn't get is, should I put it in "docs/microvm.rst" or in some other subdirectory? Thanks, Sergio. signature.asc Description: PGP signature
Re: docker: how to use it when developing on QEMU?
Alex, thank you for these hints. Let me add one more: docs/devel/testing.rst section "Docker based tests". Does it contain your hints already? If not, it may need an update.
Re: [PATCH v4 7/8] docs/microvm.txt: document the new microvm machine type
Paolo Bonzini writes: > On 24/09/19 14:44, Sergio Lopez wrote: >> +Microvm is a machine type inspired by both NEMU and Firecracker, and >> +constructed after the machine model implemented by the latter. > > I would say it's inspired by Firecracker only. The NEMU virt machine > had virtio-pci and ACPI. Actually, the NEMU reference comes from the fact that, originally, microvm.c code was based on virt.c, but on v4 all that is already gone, so it makes sense to remove the reference. >> +It's main purpose is providing users a minimalist machine type free >> +from the burden of legacy compatibility, > > I think this is too strong, especially if you keep the PIC and PIT. :) > Maybe just "It's a minimalist machine type without PCI support designed > for short-lived guests". OK. >> +serving as a stepping stone >> +for future projects aiming at improving boot times, reducing the >> +attack surface and slimming down QEMU's footprint. > > "Microvm also establishes a baseline for benchmarking QEMU and operating > systems, since it is optimized for both boot time and footprint". Well, I prefer my paragraph, but I'm good with either. >> +The microvm machine type supports the following devices: >> + >> + - ISA bus >> + - i8259 PIC >> + - LAPIC (implicit if using KVM) >> + - IOAPIC (defaults to kernel_irqchip_split = true) >> + - i8254 PIT > > Do we need the PIT? And perhaps the PIC even? We need the PIT for non-KVM accel (if present with KVM and kernel_irqchip_split = off, it basically becomes a placeholder), and the PIC for both the PIT and the ISA serial port. Thanks, Sergio. >> + - MC146818 RTC (optional) >> + - kvmclock (if using KVM) >> + - fw_cfg >> + - One ISA serial port (optional) >> + - Up to eight virtio-mmio devices (configured by the user) >> + signature.asc Description: PGP signature
Re: [PATCH v14 2/7] ppc: spapr: Introduce FWNMI capability
On Wednesday 25 September 2019 06:42 AM, David Gibson wrote:
> On Wed, Sep 18, 2019 at 01:42:17PM +0530, Aravinda Prasad wrote:
>> Introduce the KVM capability KVM_CAP_PPC_FWNMI so that
>> the KVM causes guest exit with NMI as exit reason
>> when it encounters a machine check exception on the
>> address belonging to a guest. Without this capability
>> enabled, KVM redirects machine check exceptions to
>> guest's 0x200 vector.
>>
>> This patch also introduces fwnmi-mce capability to
>> deal with the case when a guest with the
>> KVM_CAP_PPC_FWNMI capability enabled is attempted
>> to migrate to a host that does not support this
>> capability.
>>
>> Signed-off-by: Aravinda Prasad
>
> Mostly ok, but there's one ugly problem.
>
>> ---
>> hw/ppc/spapr.c |1 +
>> hw/ppc/spapr_caps.c| 29 +
>> include/hw/ppc/spapr.h |4 +++-
>> target/ppc/kvm.c | 26 ++
>> target/ppc/kvm_ppc.h | 12
>> 5 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index ea56499..8288e8b 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4487,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
>> void *data)
>> smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
>> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
>> +smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
>> spapr_caps_add_properties(smc, _abort);
>> smc->irq = _irq_dual;
>> smc->dr_phb_enabled = true;
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 481dfd2..c11ff87 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -496,6 +496,25 @@ static void cap_ccf_assist_apply(SpaprMachineState
>> *spapr, uint8_t val,
>> }
>> }
>>
>> +static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
>> +Error **errp)
>> +{
>> +if (!val) {
>> +return; /* Disabled by default */
>> +}
>> +
>> +if (tcg_enabled()) {
>> +/*
>> + * TCG support may not be correct in some conditions (e.g., in case
>> + * of software injected faults like duplicate SLBs).
>> + */
>> +warn_report("Firmware Assisted Non-Maskable Interrupts not
>> supported in TCG");
>> +} else if (kvm_enabled() && !kvmppc_has_cap_ppc_fwnmi()) {
>> +error_setg(errp,
>> +"Firmware Assisted Non-Maskable Interrupts not supported by KVM, try
>> cap-fwnmi-mce=off");
>> +}
>> +}
>> +
>> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>> [SPAPR_CAP_HTM] = {
>> .name = "htm",
>> @@ -595,6 +614,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>> .type = "bool",
>> .apply = cap_ccf_assist_apply,
>> },
>> +[SPAPR_CAP_FWNMI_MCE] = {
>> +.name = "fwnmi-mce",
>> +.description = "Handle fwnmi machine check exceptions",
>> +.index = SPAPR_CAP_FWNMI_MCE,
>> +.get = spapr_cap_get_bool,
>> +.set = spapr_cap_set_bool,
>> +.type = "bool",
>> +.apply = cap_fwnmi_mce_apply,
>> +},
>> };
>>
>> static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>> @@ -734,6 +762,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize,
>> SPAPR_CAP_HPT_MAXPAGESIZE);
>> SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>> SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>> SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>> +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE);
>>
>> void spapr_caps_init(SpaprMachineState *spapr)
>> {
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 03111fd..66049ac 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -79,8 +79,10 @@ typedef enum {
>> #define SPAPR_CAP_LARGE_DECREMENTER 0x08
>> /* Count Cache Flush Assist HW Instruction */
>> #define SPAPR_CAP_CCF_ASSIST0x09
>> +/* FWNMI machine check handling */
>> +#define SPAPR_CAP_FWNMI_MCE 0x0A
>> /* Num Caps */
>> -#define SPAPR_CAP_NUM (SPAPR_CAP_CCF_ASSIST + 1)
>> +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1)
>>
>> /*
>> * Capability Values
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 8c5b1f2..8b1ab78 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
>> static int cap_ppc_count_cache_flush_assist;
>> static int cap_ppc_nested_kvm_hv;
>> static int cap_large_decr;
>> +static int cap_ppc_fwnmi;
>>
>> static uint32_t debug_inst_opcode;
>>
>> @@ -2055,6 +2056,26 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int
>> mpic_proxy)
>> }
>> }
>>
>> +int kvmppc_set_fwnmi(void)
>> +{
>> +PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
Re: [PATCH] qemu-options.hx: remove stray quote
John Snow writes:
> Signed-off-by: John Snow
> ---
> qemu-options.hx | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2a04ca6ac5..629a7b1186 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1192,7 +1192,7 @@ Instead of @option{-fda}, @option{-fdb}, you can use:
> By default, @var{interface} is "ide" and @var{index} is automatically
> incremented:
> @example
> -@value{qemu_system_x86} -drive file=a -drive file=b"
> +@value{qemu_system_x86} -drive file=a -drive file=b
> @end example
> is interpreted like:
> @example
Messed up more than ten years ago, in commit e0e7ada1d55, faithfully
copied around ever since. Makes me wonder how much this part of
qemu-doc is actually read.
Reviewed-by: Markus Armbruster
Re: [PATCH V3] target/riscv: Bugfix reserved bits in PTE for RV64
On Wed, Sep 25, 2019 at 12:49 PM wrote:
>
> From: Guo Ren
>
nits: the title is probably better to be rephrased to: Ignore reserved
bits when calculating PPN for RV64
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
nits: cannot
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>
> Signed-off-by: Guo Ren
> Reviewed-by: Liu Zhiwei
> ---
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 4 +++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
> ---
> Changelog V3:
nits: normally we put changelog before the changed file summary above,
and there is no need to put another ---
> - Use UUL define for PTE_RESERVED.
> - Keep ppn >> PTE_PPN_SHIFT
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..cdc62a8 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED0xFFC0ULL
Can we define the macro for RV32 too, so that (see below)
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7e04ff5 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,12 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> +hwaddr ppn = pte;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> +hwaddr ppn = pte & ~PTE_RESERVED;
> #endif
> -hwaddr ppn = pte >> PTE_PPN_SHIFT;
> +ppn = ppn >> PTE_PPN_SHIFT;
we can just do this in this single line?
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
Regards,
Bin
Re: [PATCH V3] target/riscv: Bugfix reserved bits in PTE for RV64
On Tue, Sep 24, 2019 at 9:48 PM wrote:
>
> From: Guo Ren
>
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Hey,
As I mentioned on patch 2 I don't think this is right. It isn't up to
HW to clear these bits, software should keep them clear.
Alistair
>
> Signed-off-by: Guo Ren
> Reviewed-by: Liu Zhiwei
> ---
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 4 +++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
> ---
> Changelog V3:
> - Use UUL define for PTE_RESERVED.
> - Keep ppn >> PTE_PPN_SHIFT
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..cdc62a8 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED0xFFC0ULL
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7e04ff5 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,12 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> +hwaddr ppn = pte;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> +hwaddr ppn = pte & ~PTE_RESERVED;
> #endif
> -hwaddr ppn = pte >> PTE_PPN_SHIFT;
> +ppn = ppn >> PTE_PPN_SHIFT;
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
> 2.7.4
>
Re: [PATCH v4 7/8] docs/microvm.txt: document the new microvm machine type
Hi, > +microvm.kernel-cmdline=bool (Set off to disable adding virtio-mmio devices > to the kernel cmdline) Hmm, is that the long-term plan? IMO the virtio-mmio devices should be discoverable somehow. ACPI, or device-tree, or fw_cfg, or ... > +As no current FW is able to boot from a block device using virtio-mmio > +as its transport, To fix that the firmware must be able to find the virtio-mmio devices. cheers, Gerd
[PATCH V3] target/riscv: Bugfix reserved bits in PTE for RV64
From: Guo Ren
Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
need to ignore them. They can not be a part of ppn.
1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
4.4 Sv39: Page-Based 39-bit Virtual-Memory System
4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Signed-off-by: Guo Ren
Reviewed-by: Liu Zhiwei
---
target/riscv/cpu_bits.h | 3 +++
target/riscv/cpu_helper.c | 4 +++-
2 files changed, 6 insertions(+), 1 deletion(-)
---
Changelog V3:
- Use UUL define for PTE_RESERVED.
- Keep ppn >> PTE_PPN_SHIFT
Changelog V2:
- Bugfix pte destroyed cause boot fail
- Change to AND with a mask instead of shifting both directions
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index e998348..cdc62a8 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -470,6 +470,9 @@
#define PTE_D 0x080 /* Dirty */
#define PTE_SOFT0x300 /* Reserved for Software */
+/* Reserved highest 10 bits in PTE */
+#define PTE_RESERVED0xFFC0ULL
+
/* Page table PPN shift amount */
#define PTE_PPN_SHIFT 10
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 87dd6a6..7e04ff5 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -258,10 +258,12 @@ restart:
}
#if defined(TARGET_RISCV32)
target_ulong pte = ldl_phys(cs->as, pte_addr);
+hwaddr ppn = pte;
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
+hwaddr ppn = pte & ~PTE_RESERVED;
#endif
-hwaddr ppn = pte >> PTE_PPN_SHIFT;
+ppn = ppn >> PTE_PPN_SHIFT;
if (!(pte & PTE_V)) {
/* Invalid PTE */
--
2.7.4
Re: [PATCH] COLO-compare: Fix incorrect `if` logic
On 2019/9/24 下午11:35, Philippe Mathieu-Daudé wrote:
> Hi Fan,
>
> you forgot to Cc the maintainers (doing that for you):
>
> ./scripts/get_maintainer.pl -f net/colo-compare.c
> Zhang Chen (supporter:COLO Proxy)
> Li Zhijian (supporter:COLO Proxy)
> Jason Wang (maintainer:Network device ba...)
> qemu-devel@nongnu.org (open list:All patches CC here)
>
> On 9/24/19 4:08 PM, Fan Yang wrote:
>> 'colo_mark_tcp_pkt' should return 'true' when packets are the same, and
>> 'false' otherwise. However, it returns 'true' when
>> 'colo_compare_packet_payload' returns non-zero while
>> 'colo_compare_packet_payload' is just a 'memcmp'. The result is that
>> COLO-compare reports inconsistent TCP packets when they are actually
>> the same.
>>
> Fixes: f449c9e549c
> Reviewed-by: Philippe Mathieu-Daudé
Applied.
Thanks
>
>> Signed-off-by: Fan Yang
>> ---
>> net/colo-compare.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 7489840bde..7ee17f2cf8 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -319,7 +319,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet *spkt,
>> *mark = 0;
>>
>> if (ppkt->tcp_seq == spkt->tcp_seq && ppkt->seq_end == spkt->seq_end) {
>> -if (colo_compare_packet_payload(ppkt, spkt,
>> +if (!colo_compare_packet_payload(ppkt, spkt,
>> ppkt->header_size,
>> spkt->header_size,
>> ppkt->payload_size)) {
>> *mark = COLO_COMPARE_FREE_SECONDARY | COLO_COMPARE_FREE_PRIMARY;
>> @@ -329,7 +329,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet *spkt,
>>
>> /* one part of secondary packet payload still need to be compared */
>> if (!after(ppkt->seq_end, spkt->seq_end)) {
>> -if (colo_compare_packet_payload(ppkt, spkt,
>> +if (!colo_compare_packet_payload(ppkt, spkt,
>> ppkt->header_size + ppkt->offset,
>> spkt->header_size + spkt->offset,
>> ppkt->payload_size - ppkt->offset))
>> {
>> @@ -348,7 +348,7 @@ static bool colo_mark_tcp_pkt(Packet *ppkt, Packet *spkt,
>> /* primary packet is longer than secondary packet, compare
>> * the same part and mark the primary packet offset
>> */
>> -if (colo_compare_packet_payload(ppkt, spkt,
>> +if (!colo_compare_packet_payload(ppkt, spkt,
>> ppkt->header_size + ppkt->offset,
>> spkt->header_size + spkt->offset,
>> spkt->payload_size - spkt->offset))
>> {
>>
Re: [PATCH v3] vhost-user: save features if the char dev is closed
On 2019/9/25 上午12:20, Adrian Moreno wrote:
> That way the state can be correctly restored when the device is opened
> again. This might happen if the backend is restarted.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1738768
> Reported-by: Pei Zhang
> Fixes: 6ab79a20af3a (do not call vhost_net_cleanup() on running net from char
> user event)
> Cc: ddstr...@canonical.com
> Cc: Michael S. Tsirkin
>
> Signed-off-by: Adrian Moreno
> ---
> net/vhost-user.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 51921de443..014199d600 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -235,6 +235,10 @@ static void chr_closed_bh(void *opaque)
>
> s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>
> +if (s->vhost_net) {
> +s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> +}
> +
> qmp_set_link(name, false, );
>
> qemu_chr_fe_set_handlers(>chr, NULL, NULL, net_vhost_user_event,
Acked-by: Jason Wang
RE: [PATCH v2 0/2] RTC support for QEMU RISC-V virt machine
> -Original Message- > From: Philippe Mathieu-Daudé > Sent: Tuesday, September 24, 2019 7:35 PM > To: Anup Patel ; Peter Maydell > ; Palmer Dabbelt ; Alistair > Francis ; Sagar Karandikar > ; Bastian Koppelmann paderborn.de> > Cc: Atish Patra ; qemu-ri...@nongnu.org; qemu- > de...@nongnu.org; Anup Patel > Subject: Re: [PATCH v2 0/2] RTC support for QEMU RISC-V virt machine > > Hi Anup, > > On 9/24/19 3:11 PM, Anup Patel wrote: > > This series adds RTC device to QEMU RISC-V virt machine. We have > > selected Goldfish RTC device model for this. It's a pretty simple > > synthetic device with few MMIO registers and no dependency external > > clock. The driver for Goldfish RTC is already available in Linux so we > > just need to enable it in Kconfig for RISCV and also update Linux > > defconfigs. > > > > We have tested this series with Linux-5.3 plus defconfig changes > > available in 'goldfish_rtc_v1' branch of: > > https://github.com/avpatel/linux.git > > > > Changes since v1: > > - Removed redundant object properties from Goldfish RTC emulation > > - Added vmstate for Goldfish RTC > > > > Anup Patel (2): > > hw: timer: Add Goldfish RTC device > > riscv: virt: Use Goldfish RTC device > > > > hw/riscv/Kconfig| 1 + > > hw/riscv/virt.c | 15 ++ > > hw/timer/Kconfig| 3 + > > hw/timer/Makefile.objs | 1 + > > hw/timer/goldfish_rtc.c | 278 > > > include/hw/riscv/virt.h | 2 + > > include/hw/timer/goldfish_rtc.h | 46 ++ > > Minor comment, if my ongoing series "Split RTC devices from hw/timer/ to > hw/rtc/" is accepted, you'd have to rebase this in hw/rtc/goldfish_rtc: > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03334.html > (no logical change involved). No problem, I will rebase these patches once your series after it is accepted. Regards, Anup
Re: [Qemu-devel] vhost, iova, and dirty page tracking
On 2019/9/24 上午10:02, Tian, Kevin wrote: From: Jason Wang [mailto:jasow...@redhat.com] Sent: Friday, September 20, 2019 9:19 AM On 2019/9/20 上午6:54, Tian, Kevin wrote: From: Paolo Bonzini [mailto:pbonz...@redhat.com] Sent: Thursday, September 19, 2019 7:14 PM On 19/09/19 09:16, Tian, Kevin wrote: why GPA1 and GPA2 should be both dirty? even they have the same HVA due to overlaping virtual address space in two processes, they still correspond to two physical pages. don't get what's your meaning :) The point is not leave any corner case that is hard to debug or fix in the future. Let's just start by a single process, the API allows userspace to maps HVA to both GPA1 and GPA2. Since it knows GPA1 and GPA2 are equivalent, it's ok to sync just through GPA1. That means if you only log GPA2, it won't work. I noted KVM itself doesn't consider such situation (one HVA is mapped to multiple GPAs), when doing its dirty page tracking. If you look at kvm_vcpu_mark_page_dirty, it simply finds the unique memslot which contains the dirty gfn and then set the dirty bit within that slot. It doesn't attempt to walk all memslots to find out any other GPA which may be mapped to the same HVA. So there must be some disconnect here. let's hear from Paolo first and understand the rationale behind such situation. In general, userspace cannot assume that it's okay to sync just through GPA1. It must sync the host page if *either* GPA1 or GPA2 are marked dirty. Agree. In this case the kernel only needs to track whether GPA1 or GPA2 is dirtied by guest operations. Not necessarily guest operations. The reason why vhost has to set both GPA1 and GPA2 is due to its own design - it maintains IOVA->HVA and GPA->HVA mappings thus given a IOVA you have to reverse lookup GPA->HVA memTable which gives multiple possible GPAs. So if userspace need to track both GPA1 and GPA2, vhost can just stop when it found a one HVA->GPA mapping there. But in concept if vhost can maintain a IOVA->GPA mapping, then it is straightforward to set the right GPA every time when a IOVA is tracked. That means, the translation is done twice by software, IOVA->GPA and GPA->HVA for each packet. Thanks yes, it's not necessary if we care about only the content of the dirty GPA, as seen in live migration. In that case, just setting the first GPA in the loop is sufficient as you pointed out. However there is one corner case which I'm not sure. What about an usage (e.g. VM introspection) which cares only about the guest access pattern i.e. which GPA is dirtied instead of poking its content? Neither setting the first GPA nor setting all the aliasing GPAs can provide the accurate info, if no explicit IOVA->GPA mapping is maintained inside vhost. But I cannot tell whether maintaining such accuracy for aliasing GPAs is really necessary. +VM introspection guys if they have some opinions. Interesting, for vhost, vIOMMU can pass IOVA->GPA actually and vhost can keep it and just do the translation from GPA->HVA in the map command. So it can have both IOVA->GPA and IOVA->HVA mapping. Thanks Thanks Kevin
Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times
On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote: > s390 was trying to solve limited KVM memslot size issue by abusing > memory_region_allocate_system_memory(), which breaks API contract > where the function might be called only once. > > Beside an invalid use of API, the approach also introduced migration > issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in > migration stream as separate RAMBlocks. > > After discussion [1], it was agreed to break migration from older > QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12) > and considered to be not actually used downstream). > Migration should keep working for guests with less than 8TB and for > more than 8TB with QEMU 4.2 and newer binary. > In case user tries to migrate more than 8TB guest, between incompatible > QEMU versions, migration should fail gracefully due to non-exiting > RAMBlock ID or RAMBlock size mismatch. > > Taking in account above and that now KVM code is able to split too > big MemorySection into several memslots, partially revert commit > (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB) > and use kvm_set_max_memslot_size() to set KVMSlot size to > KVM_SLOT_MAX_BYTES. > > 1) [PATCH RFC v2 4/4] s390: do not call > memory_region_allocate_system_memory() multiple times > > Signed-off-by: Igor Mammedov Acked-by: Peter Xu IMHO it would be good to at least mention bb223055b9 in the commit message even if not with a "Fixed:" tag. May be amended during commit if anyone prefers. Also, this only applies the split limitation to s390. Would that be a good thing to some other archs as well? Thanks, -- Peter Xu
Re: [PATCH v7 3/4] kvm: split too big memory section on several memslots
On Tue, Sep 24, 2019 at 10:47:50AM -0400, Igor Mammedov wrote:
[...]
> @@ -2877,6 +2912,7 @@ static bool kvm_accel_has_memory(MachineState *ms,
> AddressSpace *as,
>
> for (i = 0; i < kvm->nr_as; ++i) {
> if (kvm->as[i].as == as && kvm->as[i].ml) {
> +size = MIN(kvm_max_slot_size, size);
> return NULL != kvm_lookup_matching_slot(kvm->as[i].ml,
> start_addr, size);
> }
Ideally we could also check that the whole (start_addr, size) region
is covered by KVM memslots here, but with current code I can't think
of a case where the result doesn't match with only checking the 1st
memslot. So I assume it's fine.
Reviewed-by: Peter Xu
--
Peter Xu
Re: [PATCH 4/4] xics: Merge TYPE_ICS_BASE and TYPE_ICS_SIMPLE classes
On Tue, Sep 24, 2019 at 04:06:02PM +0200, Cédric Le Goater wrote: > On 24/09/2019 13:41, David Gibson wrote: > > On Tue, Sep 24, 2019 at 07:31:44AM +0200, Cédric Le Goater wrote: > >> On 24/09/2019 06:59, David Gibson wrote: > >>> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever > >>> instantiated, and the only one we're ever likely to want. The > >>> existence of different classes is just a hang over from when we > >>> (misguidedly) had separate subtypes for the KVM and non-KVM version of > >>> the device. > >>> > >>> So, collapse the two classes together into just TYPE_ICS. > >> > >> > >> Well, I have been maintaining another subclass for the PHB3 MSI > >> but it has never been merged and it will require some rework. > > > > Well, if you did do this again, is there an actual need for it to be a > > subclass of ICS_BASE, and not ICS_SIMPLE? AFAICT the merged ICS class > > should be fine for pnv as well. > > the reject resend handlers might be an issue. Anyhow, let's merge this > cleanup. PHB3 has been out of tree for too long. Hrm, are you sure. I remember thinking the other day "whatever happened to that PHB3 patchset?". Is it actually broken, or has it just been a long time since it was posted, and therefore been forgotten by me. -- 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: [PATCH v14 4/7] target/ppc: Build rtas error log upon an MCE
On Wed, Sep 18, 2019 at 01:42:34PM +0530, Aravinda Prasad wrote:
> Upon a machine check exception (MCE) in a guest address space,
> KVM causes a guest exit to enable QEMU to build and pass the
> error to the guest in the PAPR defined rtas error log format.
>
> This patch builds the rtas error log, copies it to the rtas_addr
> and then invokes the guest registered machine check handler. The
> handler in the guest takes suitable action(s) depending on the type
> and criticality of the error. For example, if an error is
> unrecoverable memory corruption in an application inside the
> guest, then the guest kernel sends a SIGBUS to the application.
> For recoverable errors, the guest performs recovery actions and
> logs the error.
>
> Signed-off-by: Aravinda Prasad
> ---
> hw/ppc/spapr.c | 13 +++
> hw/ppc/spapr_events.c | 222
>
> hw/ppc/spapr_rtas.c| 26 ++
> include/hw/ppc/spapr.h |6 +
> target/ppc/kvm.c |4 +
> 5 files changed, 268 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 76ed988..9f2e5d2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2930,6 +2930,19 @@ static void spapr_machine_init(MachineState *machine)
> error_report("Could not get size of LPAR rtas '%s'", filename);
> exit(1);
> }
> +
> +if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) {
> +/*
> + * Ensure that the rtas image size is less than RTAS_ERROR_LOG_OFFSET
> + * or else the rtas image will be overwritten with the rtas error log
> + * when a machine check exception is encountered.
> + */
> +g_assert(spapr->rtas_size < RTAS_ERROR_LOG_OFFSET);
> +
> +/* Resize rtas blob to accommodate error log */
> +spapr->rtas_size = RTAS_ERROR_LOG_MAX;
> +}
> +
I've recently merged into ppc-for-4.2 the change to have SLOF supply
the RTAS blob, rather than qemu. So this code will need to be updated
accordingly.
> spapr->rtas_blob = g_malloc(spapr->rtas_size);
> if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
> error_report("Could not load LPAR rtas '%s'", filename);
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 0ce96b8..ecc3d68 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -214,6 +214,106 @@ struct hp_extended_log {
> struct rtas_event_log_v6_hp hp;
> } QEMU_PACKED;
>
> +struct rtas_event_log_v6_mc {
> +#define RTAS_LOG_V6_SECTION_ID_MC 0x4D43 /* MC */
> +struct rtas_event_log_v6_section_header hdr;
> +uint32_t fru_id;
> +uint32_t proc_id;
> +uint8_t error_type;
> +#define RTAS_LOG_V6_MC_TYPE_UE 0
> +#define RTAS_LOG_V6_MC_TYPE_SLB 1
> +#define RTAS_LOG_V6_MC_TYPE_ERAT 2
> +#define RTAS_LOG_V6_MC_TYPE_TLB 4
> +#define RTAS_LOG_V6_MC_TYPE_D_CACHE 5
> +#define RTAS_LOG_V6_MC_TYPE_I_CACHE 7
> +uint8_t sub_err_type;
> +#define RTAS_LOG_V6_MC_UE_INDETERMINATE 0
> +#define RTAS_LOG_V6_MC_UE_IFETCH 1
> +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_IFETCH 2
> +#define RTAS_LOG_V6_MC_UE_LOAD_STORE 3
> +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_LOAD_STORE 4
> +#define RTAS_LOG_V6_MC_SLB_PARITY0
> +#define RTAS_LOG_V6_MC_SLB_MULTIHIT 1
> +#define RTAS_LOG_V6_MC_SLB_INDETERMINATE 2
> +#define RTAS_LOG_V6_MC_ERAT_PARITY 1
> +#define RTAS_LOG_V6_MC_ERAT_MULTIHIT 2
> +#define RTAS_LOG_V6_MC_ERAT_INDETERMINATE3
> +#define RTAS_LOG_V6_MC_TLB_PARITY1
> +#define RTAS_LOG_V6_MC_TLB_MULTIHIT 2
> +#define RTAS_LOG_V6_MC_TLB_INDETERMINATE 3
> +uint8_t reserved_1[6];
> +uint64_t effective_address;
> +uint64_t logical_address;
> +} QEMU_PACKED;
> +
> +struct mc_extended_log {
> +struct rtas_event_log_v6 v6hdr;
> +struct rtas_event_log_v6_mc mc;
> +} QEMU_PACKED;
> +
> +struct MC_ierror_table {
> +unsigned long srr1_mask;
> +unsigned long srr1_value;
> +bool nip_valid; /* nip is a valid indicator of faulting address */
> +uint8_t error_type;
> +uint8_t error_subtype;
> +unsigned int initiator;
> +unsigned int severity;
> +};
> +
> +static const struct MC_ierror_table mc_ierror_table[] = {
> +{ 0x081c, 0x0004, true,
> + RTAS_LOG_V6_MC_TYPE_UE, RTAS_LOG_V6_MC_UE_IFETCH,
> + RTAS_LOG_INITIATOR_CPU, RTAS_LOG_SEVERITY_ERROR_SYNC, },
> +{ 0x081c, 0x0008, true,
> + RTAS_LOG_V6_MC_TYPE_SLB, RTAS_LOG_V6_MC_SLB_PARITY,
> + RTAS_LOG_INITIATOR_CPU, RTAS_LOG_SEVERITY_ERROR_SYNC, },
> +{ 0x081c,
Re: [PATCH v14 2/7] ppc: spapr: Introduce FWNMI capability
On Wed, Sep 18, 2019 at 01:42:17PM +0530, Aravinda Prasad wrote:
> Introduce the KVM capability KVM_CAP_PPC_FWNMI so that
> the KVM causes guest exit with NMI as exit reason
> when it encounters a machine check exception on the
> address belonging to a guest. Without this capability
> enabled, KVM redirects machine check exceptions to
> guest's 0x200 vector.
>
> This patch also introduces fwnmi-mce capability to
> deal with the case when a guest with the
> KVM_CAP_PPC_FWNMI capability enabled is attempted
> to migrate to a host that does not support this
> capability.
>
> Signed-off-by: Aravinda Prasad
Mostly ok, but there's one ugly problem.
> ---
> hw/ppc/spapr.c |1 +
> hw/ppc/spapr_caps.c| 29 +
> include/hw/ppc/spapr.h |4 +++-
> target/ppc/kvm.c | 26 ++
> target/ppc/kvm_ppc.h | 12
> 5 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ea56499..8288e8b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4487,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> void *data)
> smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> +smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> spapr_caps_add_properties(smc, _abort);
> smc->irq = _irq_dual;
> smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 481dfd2..c11ff87 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -496,6 +496,25 @@ static void cap_ccf_assist_apply(SpaprMachineState
> *spapr, uint8_t val,
> }
> }
>
> +static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
> +Error **errp)
> +{
> +if (!val) {
> +return; /* Disabled by default */
> +}
> +
> +if (tcg_enabled()) {
> +/*
> + * TCG support may not be correct in some conditions (e.g., in case
> + * of software injected faults like duplicate SLBs).
> + */
> +warn_report("Firmware Assisted Non-Maskable Interrupts not supported
> in TCG");
> +} else if (kvm_enabled() && !kvmppc_has_cap_ppc_fwnmi()) {
> +error_setg(errp,
> +"Firmware Assisted Non-Maskable Interrupts not supported by KVM, try
> cap-fwnmi-mce=off");
> +}
> +}
> +
> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> [SPAPR_CAP_HTM] = {
> .name = "htm",
> @@ -595,6 +614,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> .type = "bool",
> .apply = cap_ccf_assist_apply,
> },
> +[SPAPR_CAP_FWNMI_MCE] = {
> +.name = "fwnmi-mce",
> +.description = "Handle fwnmi machine check exceptions",
> +.index = SPAPR_CAP_FWNMI_MCE,
> +.get = spapr_cap_get_bool,
> +.set = spapr_cap_set_bool,
> +.type = "bool",
> +.apply = cap_fwnmi_mce_apply,
> +},
> };
>
> static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -734,6 +762,7 @@ SPAPR_CAP_MIG_STATE(hpt_maxpagesize,
> SPAPR_CAP_HPT_MAXPAGESIZE);
> SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE);
>
> void spapr_caps_init(SpaprMachineState *spapr)
> {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 03111fd..66049ac 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -79,8 +79,10 @@ typedef enum {
> #define SPAPR_CAP_LARGE_DECREMENTER 0x08
> /* Count Cache Flush Assist HW Instruction */
> #define SPAPR_CAP_CCF_ASSIST0x09
> +/* FWNMI machine check handling */
> +#define SPAPR_CAP_FWNMI_MCE 0x0A
> /* Num Caps */
> -#define SPAPR_CAP_NUM (SPAPR_CAP_CCF_ASSIST + 1)
> +#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI_MCE + 1)
>
> /*
> * Capability Values
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8c5b1f2..8b1ab78 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
> static int cap_ppc_count_cache_flush_assist;
> static int cap_ppc_nested_kvm_hv;
> static int cap_large_decr;
> +static int cap_ppc_fwnmi;
>
> static uint32_t debug_inst_opcode;
>
> @@ -2055,6 +2056,26 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int
> mpic_proxy)
> }
> }
>
> +int kvmppc_set_fwnmi(void)
> +{
> +PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +CPUState *cs = CPU(cpu);
> +int ret;
> +
> +ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
> +if (ret) {
> +error_report("This KVM version does not support FWNMI");
>
Re: [PATCH v14 5/7] ppc: spapr: Handle "ibm, nmi-register" and "ibm, nmi-interlock" RTAS calls
On Wed, Sep 18, 2019 at 01:42:43PM +0530, Aravinda Prasad wrote:
> This patch adds support in QEMU to handle "ibm,nmi-register"
> and "ibm,nmi-interlock" RTAS calls.
>
> The machine check notification address is saved when the
> OS issues "ibm,nmi-register" RTAS call.
>
> This patch also handles the case when multiple processors
> experience machine check at or about the same time by
> handling "ibm,nmi-interlock" call. In such cases, as per
> PAPR, subsequent processors serialize waiting for the first
> processor to issue the "ibm,nmi-interlock" call. The second
> processor that also received a machine check error waits
> till the first processor is done reading the error log.
> The first processor issues "ibm,nmi-interlock" call
> when the error log is consumed.
>
> Signed-off-by: Aravinda Prasad
> ---
> hw/ppc/spapr.c |9
> hw/ppc/spapr_rtas.c| 57
>
> include/hw/ppc/spapr.h |5 +++-
> 3 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9f2e5d2..6992b32 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2941,6 +2941,15 @@ static void spapr_machine_init(MachineState *machine)
>
> /* Resize rtas blob to accommodate error log */
> spapr->rtas_size = RTAS_ERROR_LOG_MAX;
> +
> +/* Set fwnmi capability in KVM */
> +if (kvmppc_set_fwnmi() < 0) {
> +error_report("Could not enable FWNMI capability");
> +exit(1);
> +}
> +
> +/* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> +spapr_fwnmi_register();
This setup only needs to happen if CAP_MCE_FWNMI is turned on, so it
makes more sense in the .apply hooks for that rather than general
machine_init, I think.
> }
>
> spapr->rtas_blob = g_malloc(spapr->rtas_size);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index d8fb8a8..b569538 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -400,6 +400,55 @@ static void rtas_get_power_level(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> rtas_st(rets, 1, 100);
> }
>
> +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> +hwaddr rtas_addr = spapr_get_rtas_addr();
> +
> +if (!rtas_addr) {
> +rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> +return;
> +}
> +
> +if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) {
> +rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> +return;
> +}
> +
> +spapr->guest_machine_check_addr = rtas_ld(args, 1);
> +rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
> +static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> +if (spapr->guest_machine_check_addr == -1) {
> +/* NMI register not called */
> +rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +return;
> +}
> +
> +if (spapr->mc_status != cpu->vcpu_id) {
> +/* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */
> +rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +return;
> +}
> +
> +/*
> + * vCPU issuing "ibm,nmi-interlock" is done with NMI handling,
> + * hence unset mc_status.
> + */
> +spapr->mc_status = -1;
> +qemu_cond_signal(>mc_delivery_cond);
> +rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +}
> +
> static struct rtas_call {
> const char *name;
> spapr_rtas_fn fn;
> @@ -544,6 +593,14 @@ hwaddr spapr_get_rtas_addr(void)
> return (hwaddr)fdt32_to_cpu(*rtas_data);
> }
>
> +void spapr_fwnmi_register(void)
> +{
> +spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
> +rtas_ibm_nmi_register);
> +spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
> +rtas_ibm_nmi_interlock);
> +}
> +
> static void core_rtas_register_types(void)
> {
> spapr_rtas_register(RTAS_DISPLAY_CHARACTER, "display-character",
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ffefde7..dada821 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -655,8 +655,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu,
> target_ulong opcode,
> #define RTAS_IBM_REMOVE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x28)
> #define RTAS_IBM_RESET_PE_DMA_WINDOW(RTAS_TOKEN_BASE + 0x29)
> #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
> +#define
Re: [PATCH v14 6/7] migration: Include migration support for machine check handling
On Wed, Sep 18, 2019 at 01:42:51PM +0530, Aravinda Prasad wrote:
> This patch includes migration support for machine check
> handling. Especially this patch blocks VM migration
> requests until the machine check error handling is
> complete as these errors are specific to the source
> hardware and is irrelevant on the target hardware.
>
> Signed-off-by: Aravinda Prasad
> ---
> hw/ppc/spapr.c | 63
>
> hw/ppc/spapr_events.c | 16 +++-
> hw/ppc/spapr_rtas.c|2 ++
> include/hw/ppc/spapr.h |2 ++
> 4 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6992b32..a72a4b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -46,6 +46,7 @@
> #include "migration/qemu-file-types.h"
> #include "migration/global_state.h"
> #include "migration/register.h"
> +#include "migration/blocker.h"
> #include "mmu-hash64.h"
> #include "mmu-book3s-v3.h"
> #include "cpu-models.h"
> @@ -1829,6 +1830,8 @@ static void spapr_machine_reset(MachineState *machine)
>
> /* Signal all vCPUs waiting on this condition */
> qemu_cond_broadcast(>mc_delivery_cond);
> +
> +migrate_del_blocker(spapr->fwnmi_migration_blocker);
> }
>
> static void spapr_create_nvram(SpaprMachineState *spapr)
> @@ -2119,6 +2122,60 @@ static const VMStateDescription vmstate_spapr_dtb = {
> },
> };
>
> +static bool spapr_fwnmi_needed(void *opaque)
> +{
> +SpaprMachineState *spapr = (SpaprMachineState *)opaque;
> +
> +return spapr->guest_machine_check_addr != -1;
> +}
> +
> +static int spapr_fwnmi_post_load(void *opaque, int version_id)
> +{
> +SpaprMachineState *spapr = (SpaprMachineState *)opaque;
> +
> +if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) {
> +
> +if (kvmppc_has_cap_ppc_fwnmi()) {
> +return 0;
> +}
> +
> +return kvmppc_set_fwnmi();
> +}
I don't see that you need this. The spapr caps need to be set the
same on source and destination (the caps infrastructure handles that),
so setup should already be handled by the caps .apply hooks.
> +
> +return 0;
> +}
> +
> +static int spapr_fwnmi_pre_save(void *opaque)
> +{
> +SpaprMachineState *spapr = (SpaprMachineState *)opaque;
> +
> +/*
> + * With -only-migratable QEMU option, we cannot block migration.
> + * Hence check if machine check handling is in progress and print
> + * a warning message.
> + */
> +if (spapr->mc_status != -1) {
> +warn_report("A machine check is being handled during migration. The"
> +"handler may run and log hardware error on the destination");
> +}
> +
> +return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_machine_check = {
> +.name = "spapr_machine_check",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_fwnmi_needed,
> +.post_load = spapr_fwnmi_post_load,
> +.pre_save = spapr_fwnmi_pre_save,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT64(guest_machine_check_addr, SpaprMachineState),
> +VMSTATE_INT32(mc_status, SpaprMachineState),
> +VMSTATE_END_OF_LIST()
> +},
> +};
So, I know I suggested earlier that you postpone the migration support
to a later patch in the series. But at least for the actual vmstate
stuff, I think that was due to considerations that have since gone. I
don't see any reason you can't put this in as soon as you add the
machine_check_addr and mc_status fields.
The migration blocker stuff might have to come in a later patch, but
that's ok.
> +
> static const VMStateDescription vmstate_spapr = {
> .name = "spapr",
> .version_id = 3,
> @@ -2152,6 +2209,7 @@ static const VMStateDescription vmstate_spapr = {
> _spapr_dtb,
> _spapr_cap_large_decr,
> _spapr_cap_ccf_assist,
> +_spapr_machine_check,
> NULL
> }
> };
> @@ -2948,6 +3006,11 @@ static void spapr_machine_init(MachineState *machine)
> exit(1);
> }
>
> +/* Create the error string for live migration blocker */
> +error_setg(>fwnmi_migration_blocker,
> +"A machine check is being handled during migration. The handler"
> +"may run and log hardware error on the destination");
> +
> /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> spapr_fwnmi_register();
> }
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index ecc3d68..71caa03 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -43,6 +43,7 @@
> #include "qemu/main-loop.h"
> #include "hw/ppc/spapr_ovec.h"
> #include
> +#include "migration/blocker.h"
>
> #define RTAS_LOG_VERSION_MASK 0xff00
> #define RTAS_LOG_VERSION_60x0600
> @@ -844,6 +845,8 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
> {
>
Re: [PATCH 1/1] dirty-bitmaps: remove deprecated autoload parameter
On 9/24/19 6:01 PM, John Snow wrote: > This parameter has been deprecated since 2.12.0 and is eligible for > removal. Remove this parameter as it is actually completely ignored; > let's not give false hope. > > Signed-off-by: John Snow > --- > qemu-deprecated.texi | 20 +++- > qapi/block-core.json | 6 +- > blockdev.c | 6 -- > 3 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi > index 01245e0b1c..d60246d5d6 100644 > --- a/qemu-deprecated.texi > +++ b/qemu-deprecated.texi > +@node Recently removed features > +@appendix Recently removed features > + > +What follows is a record of recently removed, formerly deprecated > +features that serves as a record for users who have encountered > +trouble after a recent upgrade. > + > +@section QEMU Machine Protocol (QMP) commands > + > +@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0) Do we want to state 'since 4.2' here, as the point where it was removed? Otherwise, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PULL 00/37] QAPI patches for 2019-09-24
Patchew URL: https://patchew.org/QEMU/20190924123334.30645-1-arm...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20190924123334.30645-1-arm...@redhat.com
Subject: [PULL 00/37] QAPI patches for 2019-09-24
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
94bb71a qapi: Assert .visit() and .check_clash() run only after .check()
e235213 qapi: Fix excessive QAPISchemaEntity.check() recursion
6abca0e qapi: Fix to .check() empty structs just once
474e373 qapi: Delete useless check_exprs() code for simple union kind
e8046dc qapi: Clean up around check_known_keys()
e04348f qapi: Simplify check_keys()
88e075d qapi: Normalize 'if' in check_exprs(), like other sugar
7aa7ae2 qapi: Fix missing 'if' checks in struct, union, alternate 'data'
85793e7 qapi: Reject blank 'if' conditions in addition to empty ones
a0f2b57 qapi: Fix broken discriminator error messages
300d20c qapi: Remove null from schema language
d7b3044 qapi: Improve reporting of lexical errors
ae7d3b9 qapi: Use quotes more consistently in frontend error messages
c390bd2 tests/qapi-schema: Demonstrate suboptimal lexical errors
fc20fdc tests/qapi-schema: Demonstrate insufficient 'if' checking
06b681b tests/qapi-schema: Demonstrate broken discriminator errors
d042260 tests/qapi-schema: Demonstrate misleading optional tag error
897d892 tests/qapi-schema: Delete two redundant tests
110d38a tests/qapi-schema: Cover unknown pragma
d43d15e qapi: Tweak code to match docs/devel/qapi-code-gen.txt
128b472 docs/devel/qapi-code-gen: Improve QAPI schema language doc
0564225 docs/devel/qapi-code-gen: Rewrite introduction to schema
7596e24 docs/devel/qapi-code-gen: Rewrite compatibility considerations
2dfb8e7 docs/devel/qapi-code-gen: Reorder sections for readability
02f7e54 qapi: Adjust frontend errors to say enum value, not member
432d8bd qapi: Permit omitting all flat union branches
0dcc5f7 qapi: Permit alternates with just one branch
d6e3fbe qapi: Permit 'boxed' with empty type
888703f qapi: Drop support for escape sequences other than \\
0446d61 qapi: Restrict strings to printable ASCII
b1b7662 tests/qapi-schema: Demonstrate bad reporting of funny characters
c106539 docs/devel/qapi-code-gen: Minor specification fixes
9d585b9 qapi: Drop support for boxed alternate arguments
93e25ba qapi: Drop check_type()'s redundant parameter @allow_optional
052b3a4 scripts/git.orderfile: Match QAPI schema more precisely
e44ecb0 make check-unit: use after free in test-opts-visitor
5cf38c1 qapi: Make visit_next_list()'s comment less confusing
=== OUTPUT BEGIN ===
1/37 Checking commit 5cf38c18d9b8 (qapi: Make visit_next_list()'s comment less
confusing)
2/37 Checking commit e44ecb08c63d (make check-unit: use after free in
test-opts-visitor)
3/37 Checking commit 052b3a49a2f3 (scripts/git.orderfile: Match QAPI schema
more precisely)
4/37 Checking commit 93e25bab7a23 (qapi: Drop check_type()'s redundant
parameter @allow_optional)
5/37 Checking commit 9d585b933636 (qapi: Drop support for boxed alternate
arguments)
6/37 Checking commit c1065396940f (docs/devel/qapi-code-gen: Minor
specification fixes)
7/37 Checking commit b1b7662708bf (tests/qapi-schema: Demonstrate bad reporting
of funny characters)
8/37 Checking commit 0446d6114ed7 (qapi: Restrict strings to printable ASCII)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#110:
new file mode 100644
ERROR: Invalid UTF-8, patch and commit message should be encoded in UTF-8
#127: FILE: tests/qapi-schema/string-code-point-127.json:2:
+{ 'command': '⌦' }
^
ERROR: Invalid UTF-8, patch and commit message should be encoded in UTF-8
#153: FILE: tests/qapi-schema/string-code-point-31.json:2:
+{ 'command': '␟' }
^
total: 2 errors, 1 warnings, 76 lines checked
Patch 8/37 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/37 Checking commit 888703f31123 (qapi: Drop support for escape sequences
other than \\)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#75:
deleted file mode 100644
total: 0 errors, 1 warnings, 53 lines checked
Patch 9/37 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/37 Checking commit d6e3fbe4fa99 (qapi: Permit 'boxed' with empty type)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#131:
deleted file mode 100644
total: 0 errors, 1 warnings, 129 lines checked
Patch 10/37 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64
On Tue, Sep 24, 2019 at 4:35 PM Alistair Francis wrote: > > On Tue, Sep 24, 2019 at 1:04 PM Palmer Dabbelt wrote: > > > > On Tue, 24 Sep 2019 11:29:25 PDT (-0700), alistai...@gmail.com wrote: > > > On Mon, Jun 24, 2019 at 11:21 AM Joel Sing wrote: > > >> > > >> On 19-06-17 16:52:44, Richard Henderson wrote: > > >> > On 6/16/19 12:19 PM, Joel Sing wrote: > > >> > > +/* > > >> > > + * Clear the load reservation, since an SC must fail if there is > > >> > > + * an SC to any address, in between an LR and SC pair. > > >> > > + */ > > >> > > +tcg_gen_movi_tl(load_res, 0); > > >> > > + > > >> > > gen_set_label(l2); > > >> > > > >> > This clear needs to be moved down below label l2. > > >> > Otherwise, with lr / sc / sc, the second sc could succeed in error. > > >> > > >> Indeed, thanks. > > >> > > >> > FWIW, other targets have used -1 as the "invalid" load reservation, > > >> > since the > > >> > architecture does not require address 0 to be unmapped. This should > > >> > be quite > > >> > visible in M-mode with paging disabled and ram at offset 0. Often, > > >> > other > > >> > targets require alignment for the lr/sc address, though I don't see > > >> > that for riscv. > > >> > > >> I've switched to -1 as suggested. Regarding the alignment for > > >> reservations, the > > >> specification does require this, although I do not recall seeing any > > >> enforcement > > >> of this by qemu itself. > > >> > > >> New diff follows. > > >> > > >> From 8ef31a2ce8ef1cbeee92995a0b2994f480e9bb6d Mon Sep 17 00:00:00 2001 > > >> From: Joel Sing > > >> Date: Tue, 25 Jun 2019 02:44:24 +1000 > > >> Subject: [PATCH] Clear load reservations on qemu riscv target > > >> > > >> This prevents a load reservation from being placed in one > > >> context/process, > > >> then being used in another, resulting in an SC succeeding incorrectly and > > >> breaking atomics. > > >> > > >> Signed-off-by: Joel Sing > > >> --- > > >> target/riscv/cpu.c | 1 + > > >> target/riscv/cpu_helper.c | 9 + > > >> target/riscv/insn_trans/trans_rva.inc.c | 8 +++- > > >> 3 files changed, 17 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > >> index d61bce6d55..e7c8bf48fc 100644 > > >> --- a/target/riscv/cpu.c > > >> +++ b/target/riscv/cpu.c > > >> @@ -281,6 +281,7 @@ static void riscv_cpu_reset(CPUState *cs) > > >> env->pc = env->resetvec; > > >> #endif > > >> cs->exception_index = EXCP_NONE; > > >> +env->load_res = -1; > > >> set_default_nan_mode(1, >fp_status); > > >> } > > >> > > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > >> index b17f169681..6a07b12e65 100644 > > >> --- a/target/riscv/cpu_helper.c > > >> +++ b/target/riscv/cpu_helper.c > > >> @@ -113,6 +113,15 @@ void riscv_cpu_set_mode(CPURISCVState *env, > > >> target_ulong newpriv) > > >> } > > >> /* tlb_flush is unnecessary as mode is contained in mmu_idx */ > > >> env->priv = newpriv; > > >> + > > >> +/* Clear the load reservation - otherwise a reservation placed in > > >> one > > >> + * context/process can be used by another, resulting in an SC > > >> succeeding > > >> + * incorrectly. Version 2.2 of the ISA specification explicitly > > >> requires > > >> + * this behaviour, while later revisions say that the kernel > > >> "should" use > > >> + * an SC instruction to force the yielding of a load reservation on > > >> a > > >> + * preemptive context switch. As a result, do both. > > >> + */ > > >> +env->load_res = -1; > > >> } > > >> > > >> /* get_physical_address - get the physical address for this virtual > > >> address > > >> diff --git a/target/riscv/insn_trans/trans_rva.inc.c > > >> b/target/riscv/insn_trans/trans_rva.inc.c > > >> index f6dbbc065e..fadd88849e 100644 > > >> --- a/target/riscv/insn_trans/trans_rva.inc.c > > >> +++ b/target/riscv/insn_trans/trans_rva.inc.c > > >> @@ -61,7 +61,7 @@ static inline bool gen_sc(DisasContext *ctx, > > >> arg_atomic *a, TCGMemOp mop) > > >> > > >> gen_set_label(l1); > > >> /* > > >> - * Address comparion failure. However, we still need to > > >> + * Address comparison failure. However, we still need to > > >> * provide the memory barrier implied by AQ/RL. > > >> */ > > >> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + a->rl * > > >> TCG_BAR_STRL); > > >> @@ -69,6 +69,12 @@ static inline bool gen_sc(DisasContext *ctx, > > >> arg_atomic *a, TCGMemOp mop) > > >> gen_set_gpr(a->rd, dat); > > >> > > >> gen_set_label(l2); > > >> +/* > > >> + * Clear the load reservation, since an SC must fail if there is > > >> + * an SC to any address, in between an LR and SC pair. > > >> + */ > > >> +tcg_gen_movi_tl(load_res, -1); > > >> + > > >> tcg_temp_free(dat); > > >> tcg_temp_free(src1); > > >> tcg_temp_free(src2); > > >> -- > > > > > > This
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
在 2019/9/25 上午8:21, Alistair Francis 写道: On Tue, Sep 24, 2019 at 5:13 PM Guo Ren wrote: 在 2019年9月25日，上午7:33，Alistair Francis 写道： On Tue, Sep 24, 2019 at 12:58 AM wrote: From: Guo Ren Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we need to ignore them. They can not be a part of ppn. 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture 4.4 Sv39: Page-Based 39-bit Virtual-Memory System 4.5 Sv48: Page-Based 48-bit Virtual-Memory System Thanks for the patch! The spec says "must be zeroed by software for forward compatibility" so I don't think it's correct for QEMU to zero out the bits. QEMU don’t zero out the bits, QEMU just ignore the bits for ppn. Yes, from reading the spec that seems to be the correct behaviour. Thank you very much :) Does this fix a bug you are seeing? Yes, because we try to reuse these bits as attributes. That isn't really a bug then, the spec says not to do that. Yes, I'll change the tile that "Ignore reserved bits in PTE for RV64" Changelog V2: - Bugfix pte destroyed cause boot fail - Change to AND with a mask instead of shifting both directions The changelog shouldn't be in the commit, it should be kept under the line line below. I just prefer to save them in commit. Fair, but in QEMU we don't commit the change log in the commit. Ok. Signed-off-by: Guo Ren Reviewed-by: Liu Zhiwei --- The change log should go here. OK, but git am we’ll lose them. target/riscv/cpu_bits.h | 3 +++ target/riscv/cpu_helper.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index e998348..ae8aa0f 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -470,6 +470,9 @@ #define PTE_D 0x080 /* Dirty */ #define PTE_SOFT0x300 /* Reserved for Software */ +/* Reserved highest 10 bits in PTE */ +#define PTE_RESERVED((target_ulong)0x3ff << 54) I think it's just easier to define this as 0xFFC0ULL and remove the cast. OK follow your rule, but I still prefer prior. + /* Page table PPN shift amount */ #define PTE_PPN_SHIFT 10 diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 87dd6a6..7a540cc 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -258,10 +258,11 @@ restart: } #if defined(TARGET_RISCV32) target_ulong pte = ldl_phys(cs->as, pte_addr); +hwaddr ppn = pte >> PTE_PPN_SHIFT; #elif defined(TARGET_RISCV64) target_ulong pte = ldq_phys(cs->as, pte_addr); +hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT; #endif -hwaddr ppn = pte >> PTE_PPN_SHIFT; You don't have to move this shift En… Do you want this: ? #if defined(TARGET_RISCV32) target_ulong pte = ldl_phys(cs->as, pte_addr); + hwaddr ppn = pte; #elif defined(TARGET_RISCV64) target_ulong pte = ldq_phys(cs->as, pte_addr); + hwaddr ppn = (pte & ~PTE_RESERVED); #endif +ppn = ppn >> PTE_PPN_SHIFT; Yeah, it seems a little cleaner. :) Best Regards Guo Ren
Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
On Tue, Sep 24, 2019 at 2:32 AM Philippe Mathieu-Daudé wrote: > > On 9/23/19 11:46 PM, Peter Maydell wrote: > > On Fri, 20 Sep 2019 at 23:23, Alistair Francis wrote: > >> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng wrote: > >>> I don't think we should mirror what is used on ARM virt board to > >>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because > >>> they need distinguish secure and non-secure. For sifive_u, only one is > >>> enough. > >> > >> I went back and forward about 1 or 2. Two seems more usable as maybe > >> someone wants to include two pflash files? The Xilinx machine also has > >> two so I'm kind of used to 2, but I'm not really fussed. > > The Xilinx machine has 2 because it matches the hardware. > > > One of the reasons for having 2 on the Arm board (we do this > > even if we're not supporting secure vs non-secure) is that > > then you can use one for a fixed read-only BIOS image (backed > > by a file on the host filesystem shared between all VMs), and > > one backed by a read-write per-VM file providing permanent > > storage for BIOS environment variables. Notably UEFI likes to > > work this way, but the idea applies in theory to other > > boot loader or BIOSes I guess. > > IIRC Laszlo's explanation, the only reason it is that way is because the > pflash_cfi01 model still doesn't implement sector locking. At the OVMF > emerged from EDK2, to have a safe ROM vs DATA storage it was decided to > use 2 different flashes. > When I understood when this config layout started, I suggested Laszlo to > use a real ROM to store the OVMF CODE since it is pointless to do > firmware upgrade in virtualized environment, but he said it was too late > to change the design. > > If you don't plan to run UEFI "Capsule Update" on your Virt board, I > suggest using memory_region_init_rom() with your firmware CODE, and a > parallel/SPI flash for the data VARStore. We might run that one day, who knows :) Alistair > > > I would suggest also checking with Markus that your code > > for instantiating the flash devices follows the current > > recommendations so the backing storage can be configured > > via -blockdev. (This is a fairly recent change from June or > > so; current-in-master virt and sbsa boards provide an example > > of doing the right thing, I think.) > > > > thanks > > -- PMM > >
Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device
On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell wrote: > > On Fri, 20 Sep 2019 at 23:23, Alistair Francis wrote: > > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng wrote: > > > I don't think we should mirror what is used on ARM virt board to > > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because > > > they need distinguish secure and non-secure. For sifive_u, only one is > > > enough. > > > > I went back and forward about 1 or 2. Two seems more usable as maybe > > someone wants to include two pflash files? The Xilinx machine also has > > two so I'm kind of used to 2, but I'm not really fussed. > > One of the reasons for having 2 on the Arm board (we do this > even if we're not supporting secure vs non-secure) is that > then you can use one for a fixed read-only BIOS image (backed > by a file on the host filesystem shared between all VMs), and > one backed by a read-write per-VM file providing permanent > storage for BIOS environment variables. Notably UEFI likes to > work this way, but the idea applies in theory to other > boot loader or BIOSes I guess. This seems like a good reason to have two and there isn't really a disadvantage so I have kept it with two. > > I would suggest also checking with Markus that your code > for instantiating the flash devices follows the current > recommendations so the backing storage can be configured > via -blockdev. (This is a fairly recent change from June or > so; current-in-master virt and sbsa boards provide an example > of doing the right thing, I think.) I have updated the code to more closely match the ARM virt machine, so I think I'm doing it correctly. Alistair > > thanks > -- PMM
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
> 在 2019年9月25日，上午7:33，Alistair Francis 写道： > > On Tue, Sep 24, 2019 at 12:58 AM wrote: >> >> From: Guo Ren >> >> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we >> need to ignore them. They can not be a part of ppn. >> >> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture >> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System >> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System > > Thanks for the patch! > > The spec says "must be zeroed by software for forward compatibility" > so I don't think it's correct for QEMU to zero out the bits. QEMU don’t zero out the bits, QEMU just ignore the bits for ppn. > > Does this fix a bug you are seeing? Yes, because we try to reuse these bits as attributes. > >> >> Changelog V2: >> - Bugfix pte destroyed cause boot fail >> - Change to AND with a mask instead of shifting both directions > > The changelog shouldn't be in the commit, it should be kept under the > line line below. I just prefer to save them in commit. > >> >> Signed-off-by: Guo Ren >> Reviewed-by: Liu Zhiwei >> --- > > The change log should go here. OK, but git am we’ll lose them. > >> target/riscv/cpu_bits.h | 3 +++ >> target/riscv/cpu_helper.c | 3 ++- >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h >> index e998348..ae8aa0f 100644 >> --- a/target/riscv/cpu_bits.h >> +++ b/target/riscv/cpu_bits.h >> @@ -470,6 +470,9 @@ >> #define PTE_D 0x080 /* Dirty */ >> #define PTE_SOFT0x300 /* Reserved for Software */ >> >> +/* Reserved highest 10 bits in PTE */ >> +#define PTE_RESERVED((target_ulong)0x3ff << 54) > > I think it's just easier to define this as 0xFFC0ULL and > remove the cast. OK follow your rule, but I still prefer prior. > >> + >> /* Page table PPN shift amount */ >> #define PTE_PPN_SHIFT 10 >> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index 87dd6a6..7a540cc 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -258,10 +258,11 @@ restart: >> } >> #if defined(TARGET_RISCV32) >> target_ulong pte = ldl_phys(cs->as, pte_addr); >> +hwaddr ppn = pte >> PTE_PPN_SHIFT; >> #elif defined(TARGET_RISCV64) >> target_ulong pte = ldq_phys(cs->as, pte_addr); >> +hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT; >> #endif >> -hwaddr ppn = pte >> PTE_PPN_SHIFT; > > You don't have to move this shift En… Do you want this: ? #if defined(TARGET_RISCV32) target_ulong pte = ldl_phys(cs->as, pte_addr); + hwaddr ppn = pte; #elif defined(TARGET_RISCV64) target_ulong pte = ldq_phys(cs->as, pte_addr); + hwaddr ppn = (pte & ~PTE_RESERVED); #endif +ppn = ppn >> PTE_PPN_SHIFT; The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
On Tue, Sep 24, 2019 at 5:13 PM Guo Ren wrote: > > > > 在 2019年9月25日，上午7:33，Alistair Francis 写道： > > > > On Tue, Sep 24, 2019 at 12:58 AM wrote: > >> > >> From: Guo Ren > >> > >> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we > >> need to ignore them. They can not be a part of ppn. > >> > >> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture > >> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System > >> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System > > > > Thanks for the patch! > > > > The spec says "must be zeroed by software for forward compatibility" > > so I don't think it's correct for QEMU to zero out the bits. > QEMU don’t zero out the bits, QEMU just ignore the bits for ppn. Yes, from reading the spec that seems to be the correct behaviour. > > > > > Does this fix a bug you are seeing? > Yes, because we try to reuse these bits as attributes. That isn't really a bug then, the spec says not to do that. > > > > >> > >> Changelog V2: > >> - Bugfix pte destroyed cause boot fail > >> - Change to AND with a mask instead of shifting both directions > > > > The changelog shouldn't be in the commit, it should be kept under the > > line line below. > I just prefer to save them in commit. Fair, but in QEMU we don't commit the change log in the commit. > > > > >> > >> Signed-off-by: Guo Ren > >> Reviewed-by: Liu Zhiwei > >> --- > > > > The change log should go here. > OK, but git am we’ll lose them. > > > > >> target/riscv/cpu_bits.h | 3 +++ > >> target/riscv/cpu_helper.c | 3 ++- > >> 2 files changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > >> index e998348..ae8aa0f 100644 > >> --- a/target/riscv/cpu_bits.h > >> +++ b/target/riscv/cpu_bits.h > >> @@ -470,6 +470,9 @@ > >> #define PTE_D 0x080 /* Dirty */ > >> #define PTE_SOFT0x300 /* Reserved for Software */ > >> > >> +/* Reserved highest 10 bits in PTE */ > >> +#define PTE_RESERVED((target_ulong)0x3ff << 54) > > > > I think it's just easier to define this as 0xFFC0ULL and > > remove the cast. > OK follow your rule, but I still prefer prior. > > > > >> + > >> /* Page table PPN shift amount */ > >> #define PTE_PPN_SHIFT 10 > >> > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >> index 87dd6a6..7a540cc 100644 > >> --- a/target/riscv/cpu_helper.c > >> +++ b/target/riscv/cpu_helper.c > >> @@ -258,10 +258,11 @@ restart: > >> } > >> #if defined(TARGET_RISCV32) > >> target_ulong pte = ldl_phys(cs->as, pte_addr); > >> +hwaddr ppn = pte >> PTE_PPN_SHIFT; > >> #elif defined(TARGET_RISCV64) > >> target_ulong pte = ldq_phys(cs->as, pte_addr); > >> +hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT; > >> #endif > >> -hwaddr ppn = pte >> PTE_PPN_SHIFT; > > > > You don't have to move this shift > En… Do you want this: ? > > #if defined(TARGET_RISCV32) > target_ulong pte = ldl_phys(cs->as, pte_addr); > + hwaddr ppn = pte; > #elif defined(TARGET_RISCV64) > target_ulong pte = ldq_phys(cs->as, pte_addr); > + hwaddr ppn = (pte & ~PTE_RESERVED); > #endif > +ppn = ppn >> PTE_PPN_SHIFT; > Yeah, it seems a little cleaner. Alistair > The pte couldn’t be destroyed, just ppn ignore the RESERVED bits. > >
Re: [PATCH 1/3] migration/postcopy: not necessary to do discard when canonicalizing bitmap
On Tue, Sep 24, 2019 at 11:02:08AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> All pages, either partially sent or partially dirty, will be discarded in
>> postcopy_send_discard_bm_ram(), since we update the unsentmap to be
>> unsentmap = unsentmap | dirty in ram_postcopy_send_discard_bitmap().
>>
>> This is not necessary to do discard when canonicalizing bitmap. And by
>> doing so, we separate the page discard into two individual steps:
>>
>> * canonicalize bitmap
>> * discard page
>>
>> Signed-off-by: Wei Yang
>
>Yes, I think when I originally wrote it, the set of pages that was
>discarded was different; I think it was actually the set of
>!unsent & dirty - i.e. only pages that had been sent and then redirtied;
>it later got reworked to include unsent pages as well - so this lot can
>be simplified.
>
>
Thanks for your time :-)
>
>Reviewed-by: Dr. David Alan Gilbert
>
>> ---
>> migration/ram.c | 14 +-
>> 1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 35552c090b..075ddc468c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2928,7 +2928,7 @@ static int
>> postcopy_each_ram_send_discard(MigrationState *ms)
>> }
>>
>> /**
>> - * postcopy_chunk_hostpages_pass: canocalize bitmap in hostpages
>> + * postcopy_chunk_hostpages_pass: canonicalize bitmap in hostpages
>> *
>> * Helper for postcopy_chunk_hostpages; it's called twice to
>> * canonicalize the two bitmaps, that are similar, but one is
>> @@ -2991,18 +2991,6 @@ static void
>> postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>> host_ratio);
>> run_start = QEMU_ALIGN_UP(run_start, host_ratio);
>>
>> -/* Tell the destination to discard this page */
>> -if (unsent_pass || !test_bit(fixup_start_addr, unsentmap)) {
>> -/* For the unsent_pass we:
>> - * discard partially sent pages
>> - * For the !unsent_pass (dirty) we:
>> - * discard partially dirty pages that were sent
>> - * (any partially sent pages were already discarded
>> - * by the previous unsent_pass)
>> - */
>> -postcopy_discard_send_range(ms, fixup_start_addr,
>> host_ratio);
>> -}
>> -
>> /* Clean up the bitmap */
>> for (page = fixup_start_addr;
>> page < fixup_start_addr + host_ratio; page++) {
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
[PATCH v3 31/33] docker: remove unused debian-sid
From: John Snow
debian-sid is listed as a partial image, so we cannot run tests against it.
Since it isn't used by any other testable image, remove it for now as it
is prone to bitrot.
Signed-off-by: John Snow
Message-Id: <20190923181140.7235-6-js...@redhat.com>
Signed-off-by: Alex Bennée
---
tests/docker/Makefile.include | 2 +-
tests/docker/dockerfiles/debian-sid.docker | 35 --
2 files changed, 1 insertion(+), 36 deletions(-)
delete mode 100644 tests/docker/dockerfiles/debian-sid.docker
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 053c418d8cd..180e5439ef9 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -5,7 +5,7 @@
DOCKER_SUFFIX := .docker
DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles
# we don't run tests on intermediate images (used as base by another image)
-DOCKER_PARTIAL_IMAGES := debian9 debian10 debian-sid
+DOCKER_PARTIAL_IMAGES := debian9 debian10
DOCKER_PARTIAL_IMAGES += debian9-mxe debian-bootstrap
DOCKER_IMAGES := $(sort $(notdir $(basename $(wildcard
$(DOCKER_FILES_DIR)/*.docker
DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES))
diff --git a/tests/docker/dockerfiles/debian-sid.docker
b/tests/docker/dockerfiles/debian-sid.docker
deleted file mode 100644
index 2a1bcc33b24..000
--- a/tests/docker/dockerfiles/debian-sid.docker
+++ /dev/null
@@ -1,35 +0,0 @@
-#
-# Debian Sid Base
-#
-# Currently we can build all our guests with cross-compilers in the
-# latest Debian release (Buster). However new compilers will first
-# arrive in Sid. However Sid is a rolling distro which may be broken
-# at any particular time. To try and mitigate this we use Debian's
-# snapshot archive which provides a "stable" view of what state Sid
-# was in.
-#
-
-# This must be earlier than the snapshot date we are aiming for
-FROM debian:sid-20190812-slim
-
- # Use a snapshot known to work (see http://snapshot.debian.org/#Usage)
-ENV DEBIAN_SNAPSHOT_DATE "20190820"
-RUN sed -i "s%^deb \(https\?://\)deb.debian.org/debian/\? \(.*\)%deb
[check-valid-until=no]
\1snapshot.debian.org/archive/debian/${DEBIAN_SNAPSHOT_DATE} \2%"
/etc/apt/sources.list
-
-# Duplicate deb line as deb-src
-RUN cat /etc/apt/sources.list | sed "s/^deb\ /deb-src /" >>
/etc/apt/sources.list
-
-# Install common build utilities
-RUN apt update && \
-DEBIAN_FRONTEND=noninteractive apt install -yy eatmydata && \
-DEBIAN_FRONTEND=noninteractive eatmydata \
-apt install -y --no-install-recommends \
-bison \
-build-essential \
-ca-certificates \
-flex \
-git \
-pkg-config \
-psmisc \
-python \
-texinfo || { echo "Failed to build - see debian-sid.docker notes";
exit 1; }
--
2.20.1
Re: [PATCH v4 08/16] cputlb: Move ROM handling from I/O path to TLB path
Richard Henderson writes:
> It does not require going through the whole I/O path
> in order to discard a write.
>
> Reviewed-by: David Hildenbrand
> Signed-off-by: Richard Henderson
> ---
> include/exec/cpu-all.h| 5 -
> include/exec/cpu-common.h | 1 -
> accel/tcg/cputlb.c| 35 +++--
> exec.c| 41 +--
> 4 files changed, 25 insertions(+), 57 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index d148bded35..26547cd6dd 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -822,16 +821,17 @@ void tlb_set_page_with_attrs(CPUState *cpu,
> target_ulong vaddr,
>
> tn.addr_write = -1;
> if (prot & PAGE_WRITE) {
> -if ((memory_region_is_ram(section->mr) && section->readonly)
> -|| memory_region_is_romd(section->mr)) {
> -/* Write access calls the I/O callback. */
> -tn.addr_write = address | TLB_MMIO;
> -} else if (memory_region_is_ram(section->mr)
> - && cpu_physical_memory_is_clean(
> - memory_region_get_ram_addr(section->mr) + xlat)) {
> -tn.addr_write = address | TLB_NOTDIRTY;
> -} else {
> -tn.addr_write = address;
> +tn.addr_write = address;
> +if (memory_region_is_romd(section->mr)) {
> +/* Use the MMIO path so that the device can switch states. */
> +tn.addr_write |= TLB_MMIO;
> +} else if (memory_region_is_ram(section->mr)) {
> +if (section->readonly) {
> +tn.addr_write |= TLB_ROM;
> +} else if (cpu_physical_memory_is_clean(
> +memory_region_get_ram_addr(section->mr) + xlat)) {
> +tn.addr_write |= TLB_NOTDIRTY;
> +}
This reads a bit weird because we are saying romd isn't a ROM but
something that identifies as RAM can be ROM rather than just a memory
protected piece of RAM.
> }
> if (prot & PAGE_WRITE_INV) {
> tn.addr_write |= TLB_INVALID_MASK;
So at the moment I don't see what the TLB_ROM flag gives us that setting
TLB_INVALID doesn't - either way we won't make the write to our
ram-not-ram-rom.
> @@ -904,7 +904,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry
> *iotlbentry,
> mr = section->mr;
> mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> cpu->mem_io_pc = retaddr;
> -if (mr != _mem_rom && mr != _mem_notdirty && !cpu->can_do_io) {
> +if (mr != _mem_notdirty && !cpu->can_do_io) {
> cpu_io_recompile(cpu, retaddr);
> }
>
> @@ -945,7 +945,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry
> *iotlbentry,
> section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> mr = section->mr;
> mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> -if (mr != _mem_rom && mr != _mem_notdirty && !cpu->can_do_io) {
> +if (mr != _mem_notdirty && !cpu->can_do_io) {
> cpu_io_recompile(cpu, retaddr);
> }
> cpu->mem_io_vaddr = addr;
> @@ -1125,7 +1125,7 @@ void *probe_access(CPUArchState *env, target_ulong
> addr, int size,
> }
>
> /* Reject I/O access, or other required slow-path. */
> -if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP)) {
> +if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP | TLB_ROM)) {
> return NULL;
> }
>
> @@ -1613,6 +1613,11 @@ store_helper(CPUArchState *env, target_ulong addr,
> uint64_t val,
> return;
> }
>
> +/* Ignore writes to ROM. */
> +if (unlikely(tlb_addr & TLB_ROM)) {
> +return;
> +}
> +
> haddr = (void *)((uintptr_t)addr + entry->addend);
>
> if (unlikely(need_swap)) {
> diff --git a/exec.c b/exec.c
> index 5f2587b621..ea8c0b18ac 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -88,7 +88,7 @@ static MemoryRegion *system_io;
> AddressSpace address_space_io;
> AddressSpace address_space_memory;
>
> -MemoryRegion io_mem_rom, io_mem_notdirty;
> +MemoryRegion io_mem_notdirty;
> static MemoryRegion io_mem_unassigned;
> #endif
>
> @@ -192,7 +192,6 @@ typedef struct subpage_t {
>
> #define PHYS_SECTION_UNASSIGNED 0
> #define PHYS_SECTION_NOTDIRTY 1
> -#define PHYS_SECTION_ROM 2
>
> static void io_mem_init(void);
> static void memory_map_init(void);
> @@ -1475,8 +1474,6 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
> iotlb = memory_region_get_ram_addr(section->mr) + xlat;
> if (!section->readonly) {
> iotlb |= PHYS_SECTION_NOTDIRTY;
> -} else {
> -iotlb |= PHYS_SECTION_ROM;
> }
> } else {
> AddressSpaceDispatch *d;
> @@ -3002,38 +2999,6 @@ static uint16_t dummy_section(PhysPageMap *map,
> FlatView *fv, MemoryRegion *mr)
> return phys_section_add(map, );
> }
>
> -static void readonly_mem_write(void
[PATCH 0/1] dirty-bitmaps: remove deprecated autoload parameter
I'm going to be honest, here. There's actually no real reason to remove this now, but we could, so I'm going to. Also, in terms of the API serving as documentation, it's nicer to not pretend this is an option that does anything, so out it goes. This will serve as a little smoke test to see what happens if we actually stop dropping features we claimed were deprecated. John Snow (1): dirty-bitmaps: remove deprecated autoload parameter qemu-deprecated.texi | 20 +++- qapi/block-core.json | 6 +- blockdev.c | 6 -- 3 files changed, 16 insertions(+), 16 deletions(-) -- 2.21.0
[PATCH] qemu-options.hx: remove stray quote
Signed-off-by: John Snow
---
qemu-options.hx | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index 2a04ca6ac5..629a7b1186 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1192,7 +1192,7 @@ Instead of @option{-fda}, @option{-fdb}, you can use:
By default, @var{interface} is "ide" and @var{index} is automatically
incremented:
@example
-@value{qemu_system_x86} -drive file=a -drive file=b"
+@value{qemu_system_x86} -drive file=a -drive file=b
@end example
is interpreted like:
@example
--
2.21.0
[PATCH v3 20/33] tests/tcg: add generic version of float_convs
This is broadly similar to the existing fcvt test for ARM but using the generic float testing framework. We should be able to pare down the ARM fcvt test case to purely half-precision with or without the Alt HP provision. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson --- tests/tcg/aarch64/float_convs.ref | 748 tests/tcg/arm/float_convs.ref | 748 tests/tcg/multiarch/Makefile.target | 6 +- tests/tcg/multiarch/float_convs.c | 105 4 files changed, 1604 insertions(+), 3 deletions(-) create mode 100755 tests/tcg/aarch64/float_convs.ref create mode 100644 tests/tcg/arm/float_convs.ref create mode 100644 tests/tcg/multiarch/float_convs.c diff --git a/tests/tcg/aarch64/float_convs.ref b/tests/tcg/aarch64/float_convs.ref new file mode 100755 index 000..23c062ae361 --- /dev/null +++ b/tests/tcg/aarch64/float_convs.ref @@ -0,0 +1,748 @@ +### Rounding to nearest +from single: f32(-nan:0xffa0) + to double: f64(-nan:0x00fffc) (INVALID) + to int32: 0 (INVALID) + to int64: 0 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-nan:0xffc0) + to double: f64(-nan:0x00fff8) (OK) + to int32: 0 (INVALID) + to int64: 0 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-inf:0xff80) + to double: f64(-inf:0x00fff0) (OK) + to int32: -2147483648 (INVALID) + to int64: -9223372036854775808 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-0x1.fe00p+127:0xff7f) + to double: f64(-0x1.fe00p+127:0x00c7efe000) (OK) + to int32: -2147483648 (INVALID) + to int64: -9223372036854775808 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-0x1.1874b200p+103:0xf30c3a59) + to double: f64(-0x1.1874b200p+103:0x00c661874b2000) (OK) + to int32: -2147483648 (INVALID) + to int64: -9223372036854775808 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-0x1.c0bab600p+99:0xf1605d5b) + to double: f64(-0x1.c0bab600p+99:0x00c62c0bab6000) (OK) + to int32: -2147483648 (INVALID) + to int64: -9223372036854775808 (INVALID) + to uint32: 0 (INVALID) + to uint64: 0 (INVALID) +from single: f32(-0x1.31f75000p-40:0xab98fba8) + to double: f64(-0x1.31f75000p-40:0x00bd731f75) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(-0x1.50544400p-66:0x9ea82a22) + to double: f64(-0x1.50544400p-66:0x00bbd505444000) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(-0x1.p-126:0x8080) + to double: f64(-0x1.p-126:0x00b810) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x0.p+0:00) + to double: f64(0x0.p+0:) (OK) + to int32: 0 (OK) + to int64: 0 (OK) + to uint32: 0 (OK) + to uint64: 0 (OK) +from single: f32(0x1.p-126:0x0080) + to double: f64(0x1.p-126:0x003810) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x1.p-25:0x3300) + to double: f64(0x1.p-25:0x003e60) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x1.e600p-25:0x3373) + to double: f64(0x1.e600p-25:0x003e6e6000) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x1.ff801a00p-15:0x387fc00d) + to double: f64(0x1.ff801a00p-15:0x003f0ff801a000) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x1.0c00p-14:0x3886) + to double: f64(0x1.0c00p-14:0x003f10c000) (OK) + to int32: 0 (INEXACT ) + to int64: 0 (INEXACT ) + to uint32: 0 (INEXACT ) + to uint64: 0 (INEXACT ) +from single: f32(0x1.p+0:0x3f80) + to double: f64(0x1.p+0:0x003ff0) (OK) + to int32: 1 (OK) + to int64: 1 (OK) + to uint32: 1 (OK) + to uint64: 1 (OK) +from single: f32(0x1.0040p+0:0x3f802000) + to double: f64(0x1.0040p+0:0x003ff00400) (OK) + to int32: 1 (INEXACT ) + to int64: 1 (INEXACT ) + to uint32: 1 (INEXACT ) + to uint64: 1 (INEXACT ) +from
Re: [PATCH 1/7] target/ppc: introduce get_dfp{64,128}() helper functions
On 9/24/19 2:05 PM, Mark Cave-Ayland wrote: > With the full patchset applied you'll see that get_dfp64() and friends are > in exactly the same form you show above, and if I swap the arguments then > the compiler does actually complain, although somewhat cryptically. Oh, good. I'll finish reading the whole set before making too many more comments ahead of your actual steps. r~
Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64
On Tue, Sep 24, 2019 at 1:04 PM Palmer Dabbelt wrote: > > On Tue, 24 Sep 2019 11:29:25 PDT (-0700), alistai...@gmail.com wrote: > > On Mon, Jun 24, 2019 at 11:21 AM Joel Sing wrote: > >> > >> On 19-06-17 16:52:44, Richard Henderson wrote: > >> > On 6/16/19 12:19 PM, Joel Sing wrote: > >> > > +/* > >> > > + * Clear the load reservation, since an SC must fail if there is > >> > > + * an SC to any address, in between an LR and SC pair. > >> > > + */ > >> > > +tcg_gen_movi_tl(load_res, 0); > >> > > + > >> > > gen_set_label(l2); > >> > > >> > This clear needs to be moved down below label l2. > >> > Otherwise, with lr / sc / sc, the second sc could succeed in error. > >> > >> Indeed, thanks. > >> > >> > FWIW, other targets have used -1 as the "invalid" load reservation, > >> > since the > >> > architecture does not require address 0 to be unmapped. This should be > >> > quite > >> > visible in M-mode with paging disabled and ram at offset 0. Often, other > >> > targets require alignment for the lr/sc address, though I don't see that > >> > for riscv. > >> > >> I've switched to -1 as suggested. Regarding the alignment for > >> reservations, the > >> specification does require this, although I do not recall seeing any > >> enforcement > >> of this by qemu itself. > >> > >> New diff follows. > >> > >> From 8ef31a2ce8ef1cbeee92995a0b2994f480e9bb6d Mon Sep 17 00:00:00 2001 > >> From: Joel Sing > >> Date: Tue, 25 Jun 2019 02:44:24 +1000 > >> Subject: [PATCH] Clear load reservations on qemu riscv target > >> > >> This prevents a load reservation from being placed in one context/process, > >> then being used in another, resulting in an SC succeeding incorrectly and > >> breaking atomics. > >> > >> Signed-off-by: Joel Sing > >> --- > >> target/riscv/cpu.c | 1 + > >> target/riscv/cpu_helper.c | 9 + > >> target/riscv/insn_trans/trans_rva.inc.c | 8 +++- > >> 3 files changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > >> index d61bce6d55..e7c8bf48fc 100644 > >> --- a/target/riscv/cpu.c > >> +++ b/target/riscv/cpu.c > >> @@ -281,6 +281,7 @@ static void riscv_cpu_reset(CPUState *cs) > >> env->pc = env->resetvec; > >> #endif > >> cs->exception_index = EXCP_NONE; > >> +env->load_res = -1; > >> set_default_nan_mode(1, >fp_status); > >> } > >> > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >> index b17f169681..6a07b12e65 100644 > >> --- a/target/riscv/cpu_helper.c > >> +++ b/target/riscv/cpu_helper.c > >> @@ -113,6 +113,15 @@ void riscv_cpu_set_mode(CPURISCVState *env, > >> target_ulong newpriv) > >> } > >> /* tlb_flush is unnecessary as mode is contained in mmu_idx */ > >> env->priv = newpriv; > >> + > >> +/* Clear the load reservation - otherwise a reservation placed in one > >> + * context/process can be used by another, resulting in an SC > >> succeeding > >> + * incorrectly. Version 2.2 of the ISA specification explicitly > >> requires > >> + * this behaviour, while later revisions say that the kernel "should" > >> use > >> + * an SC instruction to force the yielding of a load reservation on a > >> + * preemptive context switch. As a result, do both. > >> + */ > >> +env->load_res = -1; > >> } > >> > >> /* get_physical_address - get the physical address for this virtual > >> address > >> diff --git a/target/riscv/insn_trans/trans_rva.inc.c > >> b/target/riscv/insn_trans/trans_rva.inc.c > >> index f6dbbc065e..fadd88849e 100644 > >> --- a/target/riscv/insn_trans/trans_rva.inc.c > >> +++ b/target/riscv/insn_trans/trans_rva.inc.c > >> @@ -61,7 +61,7 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic > >> *a, TCGMemOp mop) > >> > >> gen_set_label(l1); > >> /* > >> - * Address comparion failure. However, we still need to > >> + * Address comparison failure. However, we still need to > >> * provide the memory barrier implied by AQ/RL. > >> */ > >> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + a->rl * TCG_BAR_STRL); > >> @@ -69,6 +69,12 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic > >> *a, TCGMemOp mop) > >> gen_set_gpr(a->rd, dat); > >> > >> gen_set_label(l2); > >> +/* > >> + * Clear the load reservation, since an SC must fail if there is > >> + * an SC to any address, in between an LR and SC pair. > >> + */ > >> +tcg_gen_movi_tl(load_res, -1); > >> + > >> tcg_temp_free(dat); > >> tcg_temp_free(src1); > >> tcg_temp_free(src2); > >> -- > > > > This patch causes boot failures when booting systemd built with musl on > > RV64. > > > > It could possibly be a musl bug, but I wanted to throw that out here > > first to see what people think. > > Looking at the musl port, I see at least one bug in their atomics jumping out > at me: > > diff --git a/arch/riscv64/atomic_arch.h
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
On Tue, Sep 24, 2019 at 12:58 AM wrote:
>
> From: Guo Ren
>
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Thanks for the patch!
The spec says "must be zeroed by software for forward compatibility"
so I don't think it's correct for QEMU to zero out the bits.
Does this fix a bug you are seeing?
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
The changelog shouldn't be in the commit, it should be kept under the
line line below.
>
> Signed-off-by: Guo Ren
> Reviewed-by: Liu Zhiwei
> ---
The change log should go here.
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..ae8aa0f 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED((target_ulong)0x3ff << 54)
I think it's just easier to define this as 0xFFC0ULL and
remove the cast.
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7a540cc 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,11 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> +hwaddr ppn = pte >> PTE_PPN_SHIFT;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> +hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> #endif
> -hwaddr ppn = pte >> PTE_PPN_SHIFT;
You don't have to move this shift
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
> 2.7.4
>
Re: [PATCH v2] qga: add command guest-get-devices for reporting VirtIO devices
Patchew URL:
https://patchew.org/QEMU/95b2f9d76104ee09b43159528b35b96eb01bbd8c.1569329826.git.tgole...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id:
95b2f9d76104ee09b43159528b35b96eb01bbd8c.1569329826.git.tgole...@redhat.com
Subject: [PATCH v2] qga: add command guest-get-devices for reporting VirtIO
devices
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
666d5ad qga: add command guest-get-devices for reporting VirtIO devices
=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#67: FILE: qga/commands-win32.c:47:
+DEFINE_DEVPROPKEY(qga_DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1,
0x02,
WARNING: line over 80 characters
#69: FILE: qga/commands-win32.c:49:
+DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd,
0x80,
WARNING: line over 80 characters
#72: FILE: qga/commands-win32.c:52:
+DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094,
0xad,
ERROR: spaces required around that '*' (ctx:WxV)
#81: FILE: qga/commands-win32.c:61:
+CONST DEVPROPKEY *PropertyKey,
^
ERROR: spaces required around that '*' (ctx:WxV)
#82: FILE: qga/commands-win32.c:62:
+DEVPROPTYPE *PropertyType,
^
ERROR: spaces required around that '+' (ctx:VxV)
#126: FILE: qga/commands-win32.c:2288:
+buffer = g_malloc(buffer_len+1);
^
ERROR: spaces required around that '+' (ctx:VxV)
#159: FILE: qga/commands-win32.c:2321:
+hw_ids = g_new(char*, count+1);
^
ERROR: "foo* bar" should be "foo *bar"
#205: FILE: qga/commands-win32.c:2367:
+g_autofree char* vendor_id = NULL;
ERROR: "foo* bar" should be "foo *bar"
#206: FILE: qga/commands-win32.c:2368:
+g_autofree char* device_id = NULL;
ERROR: spaces required around that '=' (ctx:VxV)
#224: FILE: qga/commands-win32.c:2386:
+for (j=0; hw_ids[j] != NULL; j++) {
^
total: 7 errors, 3 warnings, 286 lines checked
Commit 666d5ad0d197 (qga: add command guest-get-devices for reporting VirtIO
devices) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/95b2f9d76104ee09b43159528b35b96eb01bbd8c.1569329826.git.tgole...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail
On Tue, 24 Sep 2019 10:25:15 +0200
Eric Auger wrote:
> This series allows the memory_region_register_iommu_notifier()
> to fail. As of now, when a MAP notifier is attempted to be
> registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
> MR notify_flag_changed() callback.
>
> In case of VFIO assigned device hotplug, this could be handled
> more nicely directly within the VFIO code, simply rejecting
> the hotplug without exiting. This is what the series achieves
> by handling the memory_region_register_iommu_notifier() returned
> value and Error object.
>
> To propagate errors collected during vfio_listener_region_add()
> we now store the error handle inside the VFIO container instead
> of a returned value.
>
> The message now is:
> (QEMU) device_add id=hot0 driver=vfio-pci host=:89:00.0 bus=pcie.1
> {"error": {"class": "GenericError", "desc": "vfio :89:00.0: failed
> to setup container for group 2: memory listener initialization failed:
> Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
> notifier which is not currently supported"}}
>
> Best Regards
>
> Eric
>
> This series can be found at:
> https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v4
>
> History:
>
> v3 -> v4:
> - added Peter's R-b on 2d patch
> - 1st patch: restore hw_error, remove useless ret assignment, improve
> DMA host window error message, remove local mr variable
>
> v2 -> v3:
> - also pass an Error handle (suggested by Peter)
>
> v1 -> v2:
> - Intel IOMMU now handles the problem differently with machine init done
> notifier and machine hotplug allowed hook.
> - use assert(!ret)
> - message rewording in SMMUv3
>
> Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
> https://patchew.org/QEMU/20190829090141.21821-1-eric.au...@redhat.com/
>
>
> Eric Auger (2):
> vfio: Turn the container error into an Error handle
> memory: allow memory_region_register_iommu_notifier() to fail
>
> exec.c| 10 +--
> hw/arm/smmuv3.c | 18 ++--
> hw/i386/amd_iommu.c | 17 +++-
> hw/i386/intel_iommu.c | 8 --
> hw/ppc/spapr_iommu.c | 8 --
> hw/vfio/common.c | 52 +++
> hw/vfio/spapr.c | 4 ++-
> hw/virtio/vhost.c | 9 --
> include/exec/memory.h | 21 ++
> include/hw/vfio/vfio-common.h | 2 +-
> memory.c | 31 +
> 11 files changed, 120 insertions(+), 60 deletions(-)
For series,
Acked-by: Alex Williamson
Paolo, would this go in through you given the memory API changes and
greater girth in patch 2/2? Thanks,
Alex
Re: [PATCH 5/7] target/ppc: change struct PPC_DFP decimal storage from uint64[2] to ppc_vsr_t
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote: > There are several places in dfp_helper.c that access the decimal number > representations in struct PPC_DFP via HI_IDX and LO_IDX defines which are set > at the top of dfp_helper.c according to the host endian. > > However we can instead switch to using ppc_vsr_t for decimal numbers and then > make subsequent use of the existing VsrD() macros to access the correct > element regardless of host endian. Note that 64-bit decimals are stored in the > LSB of ppc_vsr_t (equivalent to VsrD(1)). > > Signed-off-by: Mark Cave-Ayland > --- > target/ppc/dfp_helper.c | 210 +--- > 1 file changed, 108 insertions(+), 102 deletions(-) Reviewed-by: Richard Henderson r~
RE: [PATCH v8 01/13] vfio: KABI for migration interface
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, September 25, 2019 2:03 AM
>
> On Tue, 24 Sep 2019 02:19:15 +
> "Tian, Kevin" wrote:
>
> > > From: Tian, Kevin
> > > Sent: Friday, September 13, 2019 7:00 AM
> > >
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Thursday, September 12, 2019 10:41 PM
> > > >
> > > > On Tue, 3 Sep 2019 06:57:27 +
> > > > "Tian, Kevin" wrote:
> > > >
> > > > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > > > Sent: Saturday, August 31, 2019 12:33 AM
> > > > > >
> > > > > > On Fri, 30 Aug 2019 08:06:32 +
> > > > > > "Tian, Kevin" wrote:
> > > > > >
> > > > > > > > From: Tian, Kevin
> > > > > > > > Sent: Friday, August 30, 2019 3:26 PM
> > > > > > > >
> > > > > > > [...]
> > > > > > > > > How does QEMU handle the fact that IOVAs are potentially
> > > > dynamic
> > > > > > while
> > > > > > > > > performing the live portion of a migration? For example,
> each
> > > > time a
> > > > > > > > > guest driver calls dma_map_page() or dma_unmap_page(), a
> > > > > > > > > MemoryRegionSection pops in or out of the AddressSpace for
> > > the
> > > > device
> > > > > > > > > (I'm assuming a vIOMMU where the device AddressSpace is
> not
> > > > > > > > > system_memory). I don't see any QEMU code that intercepts
> > > that
> > > > > > change
> > > > > > > > > in the AddressSpace such that the IOVA dirty pfns could be
> > > > recorded and
> > > > > > > > > translated to GFNs. The vendor driver can't track these
> beyond
> > > > getting
> > > > > > > > > an unmap notification since it only knows the IOVA pfns,
> which
> > > > can be
> > > > > > > > > re-used with different GFN backing. Once the DMA mapping
> is
> > > > torn
> > > > > > down,
> > > > > > > > > it seems those dirty pfns are lost in the ether. If this
> > > > > > > > > works in
> > > > QEMU,
> > > > > > > > > please help me find the code that handles it.
> > > > > > > >
> > > > > > > > I'm curious about this part too. Interestingly, I didn't find
> > > > > > > > any
> > > > log_sync
> > > > > > > > callback registered by emulated devices in Qemu. Looks dirty
> > > pages
> > > > > > > > by emulated DMAs are recorded in some implicit way. But KVM
> > > > always
> > > > > > > > reports dirty page in GFN instead of IOVA, regardless of the
> > > > presence of
> > > > > > > > vIOMMU. If Qemu also tracks dirty pages in GFN for emulated
> > > DMAs
> > > > > > > > (translation can be done when DMA happens), then we don't
> > > need
> > > > > > > > worry about transient mapping from IOVA to GFN. Along this
> way
> > > > we
> > > > > > > > also want GFN-based dirty bitmap being reported through VFIO,
> > > > > > > > similar to what KVM does. For vendor drivers, it needs to
> translate
> > > > > > > > from IOVA to HVA to GFN when tracking DMA activities on
> VFIO
> > > > > > > > devices. IOVA->HVA is provided by VFIO. for HVA->GFN, it can
> be
> > > > > > > > provided by KVM but I'm not sure whether it's exposed now.
> > > > > > > >
> > > > > > >
> > > > > > > HVA->GFN can be done through hva_to_gfn_memslot in
> kvm_host.h.
> > > > > >
> > > > > > I thought it was bad enough that we have vendor drivers that
> depend
> > > > on
> > > > > > KVM, but designing a vfio interface that only supports a KVM
> interface
> > > > > > is more undesirable. I also note without comment that
> > > > gfn_to_memslot()
> > > > > > is a GPL symbol. Thanks,
> > > > >
> > > > > yes it is bad, but sometimes inevitable. If you recall our discussions
> > > > > back to 3yrs (when discussing the 1st mdev framework), there were
> > > > similar
> > > > > hypervisor dependencies in GVT-g, e.g. querying gpa->hpa when
> > > > > creating some shadow structures. gpa->hpa is definitely hypervisor
> > > > > specific knowledge, which is easy in KVM (gpa->hva->hpa), but
> needs
> > > > > hypercall in Xen. but VFIO already makes assumption based on
> KVM-
> > > > > only flavor when implementing vfio_{un}pin_page_external.
> > > >
> > > > Where's the KVM assumption there? The MAP_DMA ioctl takes an
> IOVA
> > > > and
> > > > HVA. When an mdev vendor driver calls vfio_pin_pages(), we GUP the
> > > HVA
> > > > to get an HPA and provide an array of HPA pfns back to the caller. The
> > > > other vGPU mdev vendor manages to make use of this without KVM...
> the
> > > > KVM interface used by GVT-g is GPL-only.
> > >
> > > To be clear it's the assumption on the host-based hypervisors e.g. KVM.
> > > GUP is a perfect example, which doesn't work for Xen since DomU's
> > > memory doesn't belong to Dom0. VFIO in Dom0 has to find the HPA
> > > through Xen specific hypercalls.
> > >
> > > >
> > > > > So GVT-g
> > > > > has to maintain an internal abstraction layer to support both Xen
> and
> > > > > KVM. Maybe someday we will re-consider introducing some
> hypervisor
> > > > > abstraction layer in VFIO, if this issue starts to hurt other devices
> and
> > > > > Xen guys are willing
Re: [PATCH 7/7] target/ppc: remove unnecessary if() around calls to set_dfp{64, 128}() in DFP macros
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote: > Now that the parameters to both set_dfp64() and set_dfp128() are exactly the > same, there is no need for an explicit if() statement to determine which > function should be called based upon size. Instead we can simply use the > preprocessor to generate the call to set_dfp##size() directly. > > Signed-off-by: Mark Cave-Ayland > --- > target/ppc/dfp_helper.c | 60 +++-- > 1 file changed, 10 insertions(+), 50 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 3/7] target/ppc: update {get, set}_dfp{64, 128}() helper functions to read/write DFP numbers correctly
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
> Since commit ef96e3ae96 "target/ppc: move FP and VMX registers into aligned
> vsr
> register array" FP registers are no longer stored consecutively in memory and
> so
> the current method of combining FP register pairs into DFP numbers is
> incorrect.
>
> Firstly update the definition of the dh_*_fprp defines in helper.h to reflect
> that FP registers are now stored as part of an array of ppc_vsr_t elements
> rather than plain uint64_t elements, and then introduce a new ppc_fprp_t type
> which conceptually represents a DFP even-odd register pair to be consumed by
> the
> DFP helper functions.
>
> Finally update the new DFP {get,set}_dfp{64,128}() helper functions to convert
> between DFP numbers and DFP even-odd register pairs correctly, making use of
> the
> existing VsrD() macro to access the correct elements regardless of host
> endian.
>
> Fixes: ef96e3ae96 "target/ppc: move FP and VMX registers into aligned vsr
> register array"
> Signed-off-by: Mark Cave-Ayland
> ---
> target/ppc/cpu.h| 1 +
> target/ppc/dfp_helper.c | 80 +
> target/ppc/helper.h | 2 +-
> 3 files changed, 44 insertions(+), 39 deletions(-)
Yay! I really was getting ahead of things.
Reviewed-by: Richard Henderson
r~
[PATCH 1/1] dirty-bitmaps: remove deprecated autoload parameter
This parameter has been deprecated since 2.12.0 and is eligible for
removal. Remove this parameter as it is actually completely ignored;
let's not give false hope.
Signed-off-by: John Snow
---
qemu-deprecated.texi | 20 +++-
qapi/block-core.json | 6 +-
blockdev.c | 6 --
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 01245e0b1c..d60246d5d6 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -149,11 +149,6 @@ QEMU 4.1 has three options, please migrate to one of these
three:
@section QEMU Machine Protocol (QMP) commands
-@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-
-"autoload" parameter is now ignored. All bitmaps are automatically loaded
-from qcow2 images.
-
@subsection query-block result field dirty-bitmaps[i].status (since 4.0)
The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
@@ -356,3 +351,18 @@ existing CPU models. Management software that needs
runnability
guarantees must resolve the CPU model aliases using te
``alias-of'' field returned by the ``query-cpu-definitions'' QMP
command.
+
+
+@node Recently removed features
+@appendix Recently removed features
+
+What follows is a record of recently removed, formerly deprecated
+features that serves as a record for users who have encountered
+trouble after a recent upgrade.
+
+@section QEMU Machine Protocol (QMP) commands
+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
+
+"autoload" parameter is now ignored. All bitmaps are automatically loaded
+from qcow2 images.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e6edd641f1..e4975ece4a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1987,10 +1987,6 @@
# Qcow2 disks support persistent bitmaps. Default is false for
# block-dirty-bitmap-add. (Since: 2.10)
#
-# @autoload: ignored and deprecated since 2.12.
-#Currently, all dirty tracking bitmaps are loaded from Qcow2 on
-#open.
-#
# @disabled: the bitmap is created in the disabled state, which means that
#it will not track drive changes. The bitmap may be enabled with
#block-dirty-bitmap-enable. Default is false. (Since: 4.0)
@@ -1999,7 +1995,7 @@
##
{ 'struct': 'BlockDirtyBitmapAdd',
'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
+'*persistent': 'bool', '*disabled': 'bool' } }
##
# @BlockDirtyBitmapMergeSource:
diff --git a/blockdev.c b/blockdev.c
index fbef6845c8..93804da840 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,6 @@ static void block_dirty_bitmap_add_prepare(BlkActionState
*common,
qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
- action->has_autoload, action->autoload,
action->has_disabled, action->disabled,
_err);
@@ -2858,7 +2857,6 @@ out:
void qmp_block_dirty_bitmap_add(const char *node, const char *name,
bool has_granularity, uint32_t granularity,
bool has_persistent, bool persistent,
-bool has_autoload, bool autoload,
bool has_disabled, bool disabled,
Error **errp)
{
@@ -2890,10 +2888,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const
char *name,
persistent = false;
}
-if (has_autoload) {
-warn_report("Autoload option is deprecated and its value is ignored");
-}
-
if (!has_disabled) {
disabled = false;
}
--
2.21.0
Re: [PATCH 6/7] target/ppc: use existing VsrD() macro to eliminate HI_IDX and LO_IDX from dfp_helper.c
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote: > Switch over all accesses to the decimal numbers held in struct PPC_DFP from > using HI_IDX and LO_IDX to using the VsrD() macro instead. Not only does this > allow the compiler to ensure that the various dfp_* functions are being passed > a ppc_vsr_t rather than an arbitrary uint64_t pointer, but also allows the > host endian-specific HI_IDX and LO_IDX to be completely removed from > dfp_helper.c. > > Signed-off-by: Mark Cave-Ayland > --- > target/ppc/dfp_helper.c | 70 ++--- > 1 file changed, 31 insertions(+), 39 deletions(-) Ho hum, vs patch 5 that was me not realizing how many different places really want to manipulate a 128-bit value. Do go ahead and keep ppc_vsr_t for now. It does look like we might be well served by using Int128 at some point, so that these operations can expand to int128_t on appropriate hw so that the compiler can DTRT with these. Reviewed-by: Richard Henderson r~
Re: [PATCH 2/7] target/ppc: introduce set_dfp{64,128}() helper functions
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
> The existing functions (now incorrectly) assume that the MSB and LSB of DFP
> numbers are stored as consecutive 64-bit words in memory. Instead of accessing
> the DFP numbers directly, introduce set_dfp{64,128}() helper functions to ease
> the switch to the correct representation.
>
> Signed-off-by: Mark Cave-Ayland
> ---
> target/ppc/dfp_helper.c | 90 ++---
> 1 file changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
> index 354a4aa877..cb98780d20 100644
> --- a/target/ppc/dfp_helper.c
> +++ b/target/ppc/dfp_helper.c
> @@ -47,6 +47,17 @@ static void get_dfp128(uint64_t *dst, uint64_t *dfp)
> dst[1] = dfp[LO_IDX];
> }
>
> +static void set_dfp64(uint64_t *dfp, uint64_t *src)
> +{
> +dfp[0] = src[0];
> +}
> +
> +static void set_dfp128(uint64_t *dfp, uint64_t *src)
> +{
> +dfp[0] = src[HI_IDX];
> +dfp[1] = src[LO_IDX];
> +}
...
> decimal##size##FromNumber((decimal##size *)dfp.t64, ,
> );\
> postprocs(); \
> if (size == 64) { \
> -t[0] = dfp.t64[0]; \
> +set_dfp64(t, dfp.t64); \
> } else if (size == 128) { \
> -t[0] = dfp.t64[HI_IDX];\
> -t[1] = dfp.t64[LO_IDX];\
> +set_dfp128(t, dfp.t64);\
> }
This is perhaps a bit harder to see than the get case, because POSTPROCS is in
the way.
However, we can guess, because postprocs has not been passed GETPC(), that it
cannot longjmp out, and therefore not interfere with writing back of the result
to the register file. And, as it turns out from inspection, the set of
postprocs also does not modify dfp->t64; only looks at dfp->context.status.
Therefore, as a first step we can move postprocs down, then as a second step
combine the conversion from decNumber (dfp->t) to decimal128, and then into the
register file (t).
r~
Re: [PATCH 4/7] target/ppc: introduce dfp_finalize_decimal{64, 128}() helper functions
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
> Most of the DFP helper functions call decimal{64,128}FromNumber() just before
> returning in order to convert the decNumber stored in dfp.t64 back to a
> Decimal{64,128} to write back to the FP registers.
>
> Introduce new dfp_finalize_decimal{64,128}() helper functions which both
> enable
> the parameter list to be reduced considerably, and also help minimise the
> changes required in the next patch.
>
> Signed-off-by: Mark Cave-Ayland
> ---
> target/ppc/dfp_helper.c | 42 ++---
> 1 file changed, 23 insertions(+), 19 deletions(-)
Reviewed-by: Richard Henderson
r~
[PATCH v3 22/33] configure: preserve PKG_CONFIG for subdir builds
The slirp sub-module complains about not being able to find the glib library on cross-compiles because it is using the default pkg-config tool (which isn't installed in our cross-build docker images). Preserve PKG_CONFIG in our host config and pass it down to slirp. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson --- Makefile | 6 +- configure | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a0c1430b407..8da33595edd 100644 --- a/Makefile +++ b/Makefile @@ -510,7 +510,11 @@ capstone/all: .git-submodule-status .PHONY: slirp/all slirp/all: .git-submodule-status - $(call quiet-command,$(MAKE) -C $(SRC_PATH)/slirp BUILD_DIR="$(BUILD_DIR)/slirp" CC="$(CC)" AR="$(AR)" LD="$(LD)" RANLIB="$(RANLIB)" CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(LDFLAGS)") + $(call quiet-command,$(MAKE) -C $(SRC_PATH)/slirp \ + BUILD_DIR="$(BUILD_DIR)/slirp" \ + PKG_CONFIG="$(PKG_CONFIG)" \ + CC="$(CC)" AR="$(AR)" LD="$(LD)" RANLIB="$(RANLIB)" \ + CFLAGS="$(QEMU_CFLAGS) $(CFLAGS)" LDFLAGS="$(LDFLAGS)") # Compatibility gunk to keep make working across the rename of targets # for recursion, to be removed some time after 4.1. diff --git a/configure b/configure index 397bb476e19..542f6aea3f6 100755 --- a/configure +++ b/configure @@ -7302,6 +7302,7 @@ echo "OBJCOPY=$objcopy" >> $config_host_mak echo "LD=$ld" >> $config_host_mak echo "RANLIB=$ranlib" >> $config_host_mak echo "NM=$nm" >> $config_host_mak +echo "PKG_CONFIG=$pkg_config_exe" >> $config_host_mak echo "WINDRES=$windres" >> $config_host_mak echo "CFLAGS=$CFLAGS" >> $config_host_mak echo "CFLAGS_NOPIE=$CFLAGS_NOPIE" >> $config_host_mak -- 2.20.1
Re: git format.from (was: 9p: Fix file ID collisions)
On Tue, Sep 24, 2019 at 11:03:38AM +0200, Christian Schoenebeck wrote: > > Yes, the resulting mail would be correct, in the sense that it could be > > applied just fine by git-am. But I think it would be uglier. IOW, I > > consider the presence of the in-body From to be a clue that something > > interesting is going on (like forwarding somebody else's patch). So from > > my perspective, it would just be useless noise. Other communities may > > have different opinions, though (I think I have seen some kernel folks > > always including all of the possible in-body headers, including Date). > > But it seems like it makes sense to keep both possibilities. > > Exactly, current git behaviour is solely "prettier" (at first thought only > though), but does not address anything useful in real life. I wouldn't agree with that. By being pretty, it also is functionally more useful (I can tell at a glance whether somebody is sending a patch from another author). > Current git behaviour does cause real life problems though: Many email lists > are munging emails of patch senders whose domain is configured for requiring > domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a > DMARC). So original sender's From: header is then automatically replaced by > an > alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > For instance the email header: > > From: "Bob Bold" > > is automatically replaced by lists by something like > > From: "Bob Bold via Somelist" > > And since git currently always drops the From: line from the email's body if > sender == author, as a consequence maintainers applying patches from such > lists, always need to rewrite git history subsequently and have to replace > patch author's identity manually for each commit to have their correct, real > email address and real name in git history instead of something like > "Bob Bold via Somelist" > > So what do you find "uglier"? I prefer key info not being lost as default > behaviour. :-) Sure, for your list that munges From headers, always including an in-body From is way better. But for those of us _not_ on such lists, I'd much prefer not to force the in-body version on them. -Peff
[PATCH v3 17/33] tests/tcg: clean-up some comments after the de-tangling
These were missed in the recent de-tangling so have been updated to be more actuate. I've also built up ARM_TESTS in a manner similar to AARCH64_TESTS for better consistency. Signed-off-by: Alex Bennée Reviewed-by: Peter Maydell --- tests/tcg/Makefile.target | 7 +-- tests/tcg/aarch64/Makefile.target | 3 ++- tests/tcg/arm/Makefile.target | 15 --- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target index 8808beaf74b..679eb56bd37 100644 --- a/tests/tcg/Makefile.target +++ b/tests/tcg/Makefile.target @@ -74,8 +74,11 @@ TIMEOUT=15 endif ifdef CONFIG_USER_ONLY -# The order we include is important. We include multiarch, base arch -# and finally arch if it's not the same as base arch. +# The order we include is important. We include multiarch first and +# then the target. If there are common tests shared between +# sub-targets (e.g. ARM & AArch64) then it is up to +# $(TARGET_NAME)/Makefile.target to include the common parent +# architecture in its VPATH. -include $(SRC_PATH)/tests/tcg/multiarch/Makefile.target -include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.target diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target index e763dd9da37..9758f89f905 100644 --- a/tests/tcg/aarch64/Makefile.target +++ b/tests/tcg/aarch64/Makefile.target @@ -8,7 +8,7 @@ VPATH += $(ARM_SRC) AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64 VPATH += $(AARCH64_SRC) -# we don't build any other ARM test +# Float-convert Tests AARCH64_TESTS=fcvt fcvt: LDFLAGS+=-lm @@ -17,6 +17,7 @@ run-fcvt: fcvt $(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)") $(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref) +# Pauth Tests AARCH64_TESTS += pauth-1 pauth-2 run-pauth-%: QEMU_OPTS += -cpu max diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target index aa4e4e3782c..7347d3d0adb 100644 --- a/tests/tcg/arm/Makefile.target +++ b/tests/tcg/arm/Makefile.target @@ -8,25 +8,26 @@ ARM_SRC=$(SRC_PATH)/tests/tcg/arm # Set search path for all sources VPATH += $(ARM_SRC) -ARM_TESTS=hello-arm test-arm-iwmmxt - -TESTS += $(ARM_TESTS) fcvt - +# Basic Hello World +ARM_TESTS = hello-arm hello-arm: CFLAGS+=-marm -ffreestanding hello-arm: LDFLAGS+=-nostdlib +# IWMXT floating point extensions +ARM_TESTS += test-arm-iwmmxt test-arm-iwmmxt: CFLAGS+=-marm -march=iwmmxt -mabi=aapcs -mfpu=fpv4-sp-d16 test-arm-iwmmxt: test-arm-iwmmxt.S $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS) -ifeq ($(TARGET_NAME), arm) +# Float-convert Tests +ARM_TESTS += fcvt fcvt: LDFLAGS+=-lm # fcvt: CFLAGS+=-march=armv8.2-a+fp16 -mfpu=neon-fp-armv8 - run-fcvt: fcvt $(call run-test,fcvt,$(QEMU) $<,"$< on $(TARGET_NAME)") $(call diff-out,fcvt,$(ARM_SRC)/fcvt.ref) -endif + +TESTS += $(ARM_TESTS) # On ARM Linux only supports 4k pages EXTRA_RUNS+=run-test-mmap-4096 -- 2.20.1
[PATCH v3 27/33] docker: remove debian8-mxe definitions
From: John Snow We don't have a debian8-mxe dockerfile anymore. Fixes: 67bd36beda1ae Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20190923181140.7235-2-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index e85e73025ba..47d2273f29d 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -7,7 +7,7 @@ DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles DOCKER_DEPRECATED_IMAGES := debian # we don't run tests on intermediate images (used as base by another image) DOCKER_PARTIAL_IMAGES := debian debian8 debian9 debian10 debian-sid -DOCKER_PARTIAL_IMAGES += debian8-mxe debian9-mxe debian-ports debian-bootstrap +DOCKER_PARTIAL_IMAGES += debian9-mxe debian-ports debian-bootstrap DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker) DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES)) # Use a global constant ccache directory to speed up repetitive builds -- 2.20.1
[PATCH v3 24/33] target/i386: Fix broken build with WHPX enabled
From: Philippe Mathieu-Daudé
The WHPX build is broken since commit 12e9493df92 which removed the
"hw/boards.h" where MachineState is declared:
$ ./configure \
--enable-hax --enable-whpx
$ make x86_64-softmmu/all
[...]
CC x86_64-softmmu/target/i386/whpx-all.o
target/i386/whpx-all.c: In function 'whpx_accel_init':
target/i386/whpx-all.c:1378:25: error: dereferencing pointer to
incomplete type 'MachineState' {aka 'struct MachineState'}
whpx->mem_quota = ms->ram_size;
^~
make[1]: *** [rules.mak:69: target/i386/whpx-all.o] Error 1
CC x86_64-softmmu/trace/generated-helpers.o
make[1]: Target 'all' not remade because of errors.
make: *** [Makefile:471: x86_64-softmmu/all] Error 2
Restore this header, partially reverting commit 12e9493df92.
Fixes: 12e9493df92
Reported-by: Ilias Maratos
Reviewed-by: Stefan Weil
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
Message-Id: <20190920113329.16787-2-phi...@redhat.com>
---
target/i386/whpx-all.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 0c15241ae49..def0c284801 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -18,6 +18,7 @@
#include "sysemu/cpus.h"
#include "sysemu/runstate.h"
#include "qemu/main-loop.h"
+#include "hw/boards.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
#include "migration/blocker.h"
--
2.20.1
Re: [PATCH v4 07/16] exec: Adjust notdirty tracing
Richard Henderson writes:
> The memory_region_tb_read tracepoint is unreachable, since notdirty
> is supposed to apply only to writes. The memory_region_tb_write
> tracepoint is mis-named, because notdirty is not only used for TB
> invalidation. It is also used for e.g. VGA RAM updates and migration.
>
> Replace memory_region_tb_write with memory_notdirty_write_access,
> and place it in memory_notdirty_write_prepare where it can catch
> all of the instances. Add memory_notdirty_set_dirty to log when
> we no longer intercept writes to a page.
>
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: David Hildenbrand
> Signed-off-by: Richard Henderson
Reviewed-by: Alex Bennée
> ---
> exec.c | 3 +++
> memory.c | 4
> trace-events | 4 ++--
> 3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 8b998974f8..5f2587b621 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2755,6 +2755,8 @@ void memory_notdirty_write_prepare(NotDirtyInfo *ndi,
> ndi->size = size;
> ndi->pages = NULL;
>
> +trace_memory_notdirty_write_access(mem_vaddr, ram_addr, size);
> +
> assert(tcg_enabled());
> if (!cpu_physical_memory_get_dirty_flag(ram_addr, DIRTY_MEMORY_CODE)) {
> ndi->pages = page_collection_lock(ram_addr, ram_addr + size);
> @@ -2779,6 +2781,7 @@ void memory_notdirty_write_complete(NotDirtyInfo *ndi)
> /* we remove the notdirty callback only if the code has been
> flushed */
> if (!cpu_physical_memory_is_clean(ndi->ram_addr)) {
> +trace_memory_notdirty_set_dirty(ndi->mem_vaddr);
> tlb_set_dirty(ndi->cpu, ndi->mem_vaddr);
> }
> }
> diff --git a/memory.c b/memory.c
> index b9dd6b94ca..57c44c97db 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -438,7 +438,6 @@ static MemTxResult
> memory_region_read_accessor(MemoryRegion *mr,
> /* Accesses to code which has previously been translated into a TB
> show
> * up in the MMIO path, as accesses to the io_mem_notdirty
> * MemoryRegion. */
> -trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size);
> } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp,
> size);
> @@ -465,7 +464,6 @@ static MemTxResult
> memory_region_read_with_attrs_accessor(MemoryRegion *mr,
> /* Accesses to code which has previously been translated into a TB
> show
> * up in the MMIO path, as accesses to the io_mem_notdirty
> * MemoryRegion. */
> -trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size);
> } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp,
> size);
> @@ -490,7 +488,6 @@ static MemTxResult
> memory_region_write_accessor(MemoryRegion *mr,
> /* Accesses to code which has previously been translated into a TB
> show
> * up in the MMIO path, as accesses to the io_mem_notdirty
> * MemoryRegion. */
> -trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size);
> } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp,
> size);
> @@ -515,7 +512,6 @@ static MemTxResult
> memory_region_write_with_attrs_accessor(MemoryRegion *mr,
> /* Accesses to code which has previously been translated into a TB
> show
> * up in the MMIO path, as accesses to the io_mem_notdirty
> * MemoryRegion. */
> -trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size);
> } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
> hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp,
> size);
> diff --git a/trace-events b/trace-events
> index 823a4ae64e..20821ba545 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -52,14 +52,14 @@ dma_map_wait(void *dbs) "dbs=%p"
> find_ram_offset(uint64_t size, uint64_t offset) "size: 0x%" PRIx64 " @ 0x%"
> PRIx64
> find_ram_offset_loop(uint64_t size, uint64_t candidate, uint64_t offset,
> uint64_t next, uint64_t mingap) "trying size: 0x%" PRIx64 " @ 0x%" PRIx64 ",
> offset: 0x%" PRIx64" next: 0x%" PRIx64 " mingap: 0x%" PRIx64
> ram_block_discard_range(const char *rbname, void *hva, size_t length, bool
> need_madvise, bool need_fallocate, int ret) "%s@%p + 0x%zx: madvise: %d
> fallocate: %d ret: %d"
> +memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned
> size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
> +memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
>
> # memory.c
> memory_region_ops_read(int cpu_index, void *mr,
Re: [PATCH 1/7] target/ppc: introduce get_dfp{64,128}() helper functions
On 24/09/2019 20:21, Richard Henderson wrote:
> On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
>> +static void get_dfp64(uint64_t *dst, uint64_t *dfp)
>> +{
>> +dst[0] = dfp[0];
>> +}
>> +
>> +static void get_dfp128(uint64_t *dst, uint64_t *dfp)
>> +{
>> +dst[0] = dfp[HI_IDX];
>> +dst[1] = dfp[LO_IDX];
>> +}
>
> I'm not keen on this. I would prefer some type difference that prevents you
> from getting the arguments the wrong way around.
>
> I'm thinking that a combination helper like
>
> static void get_dfp64(decNumber *out, uint64_t *in)
> {
> union {
>uint64_t i;
>decimal64 d;
> } u;
>
> u.i = *in;
> decimal64ToNumber(, out);
> }
>
>> @@ -129,7 +140,7 @@ static void dfp_prepare_decimal64(struct PPC_DFP *dfp,
>> uint64_t *a,
>> dfp->env = env;
>>
>> if (a) {
>> -dfp->a64[0] = *a;
>> +get_dfp64(dfp->a64, a);
>> decimal64ToNumber((decimal64 *)dfp->a64, >a);
>> } else {
>> dfp->a64[0] = 0;
>
> becomes
>
> get_dfp64(>a, a);
>
> and that might clean up a lot of the code.
Right, and in fact I had a similar thought myself regarding type safety since
one of
the issues with working with the existing code in dfp_helper.c is that
everything
uses a uint64_t * which makes it really difficult to figure out why things are
crashing if you make a typo :/
Note that this patch simply introduces the helpers without making changes to the
existing logic which is why both arguments are still uint64_t *. The real work
is
done in patch 3 where ppc_fptr_t type is introduced, and also see the switch
from
uint64_t * to ppc_vsr_t in patch 5.
With the full patchset applied you'll see that get_dfp64() and friends are in
exactly
the same form you show above, and if I swap the arguments then the compiler does
actually complain, although somewhat cryptically.
>> @@ -617,10 +626,12 @@ uint32_t helper_##op(CPUPPCState *env, uint64_t *a,
>> uint64_t *b) \
>> {\
>> struct PPC_DFP dfp; \
>> unsigned k; \
>> +uint64_t a64;\
>> \
>> dfp_prepare_decimal##size(, 0, b, env); \
>> \
>> -k = *a & 0x3F; \
>> +get_dfp64(, a); \
>> +k = a64 & 0x3F; \
>> \
>> if (unlikely(decNumberIsSpecial())) { \
>> dfp.crbf = 1;\
>
> Whereas cases like this, where a scalar is being passed in, I don't think that
> re-using the same helper is appropriate. Ideally, they would be passed in by
> value, and not by reference at all. That, of course, requires changes to the
> translator beyond the scope of this patch.
Agreed. I was really keen to avoid any translator changes if possible since the
PPC
translator code tends to be quite tricky which is why I went with the above
approach.
>> void helper_dctqpq(CPUPPCState *env, uint64_t *t, uint64_t *b)
>> {
>> struct PPC_DFP dfp;
>> +uint64_t b64;
>> dfp_prepare_decimal128(, 0, 0, env);
>> -decimal64ToNumber((decimal64 *)b, );
>> +get_dfp64(, b);
>> +decimal64ToNumber((decimal64 *), );
>
> This would become
>
> get_dfp64(, b);
>
> with no intermediate b64 temp.
Funnily enough that did cross my mind, but I wasn't 100% sure that this wasn't
doing
something extra within libdecnumber. If you think it makes sense then I can
easily
add it into a v2.
ATB,
Mark.
Re: [PATCH v3 24/25] nbd: Fix error_append_hint usage
On 9/24/19 3:09 PM, Vladimir Sementsov-Ogievskiy wrote:
> If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
> Otherwise hint will not be appended in case of errp == _err
> (program will exit before error_append_hint() call). Fix such cases.
>
Copy-and-pasted, but if you want to tweak the grammar to all of the
patches with identical bodies:
If we want to append a hint to errp, we must use the ERRP_FUNCTION_BEGIN
macro. Otherwise, the hint will not be appended when errp == _err
(the program will exit prior to the error_append_hint() call). Fix such
cases.
> This commit (together with its neighbors) was generated by
>
> git grep -l 'error_append_hint(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
> --in-place $f; done
>
> and then
>
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>
> (auto-msg was a file with this commit message)
>
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
>
> Reported-by: Greg Kurz
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
Should the commit-per-subsystem.py script append a distinct CC: line as
long as it is already grouping files by maintainer?
> nbd/client.c | 1 +
> nbd/server.c | 1 +
> 2 files changed, 2 insertions(+)
Reviewed-by: Eric Blake
It's probably easier to take this entire series through one maintainer
(Markus, since it is error-related), than to have me pick up this patch
by itself through the NBD tree.
>
> diff --git a/nbd/client.c b/nbd/client.c
> index b9dc829175..4d90a26c00 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -153,6 +153,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc,
> uint32_t opt,
> static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> bool strict, Error **errp)
> {
> +ERRP_FUNCTION_BEGIN();
> g_autofree char *msg = NULL;
>
> if (!(reply->type & (1 << 31))) {
> diff --git a/nbd/server.c b/nbd/server.c
> index 28c3c8be85..09565ad8dc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1616,6 +1616,7 @@ void nbd_export_close(NBDExport *exp)
>
> void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error
> **errp)
> {
> +ERRP_FUNCTION_BEGIN();
> if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(>clients)) {
> nbd_export_close(exp);
> return;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 5/7] target/ppc: change struct PPC_DFP decimal storage from uint64[2] to ppc_vsr_t
On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
> struct PPC_DFP {
> CPUPPCState *env;
> -uint64_t t64[2], a64[2], b64[2];
> +ppc_vsr_t vt, va, vb;
This I don't think is a good idea. It's not a vsr_t.
I think this step would be clearer with
union {
decimal64 d;
uint64_t i;
} u;
union {
decimal128 d;
uint64_t i[2];
} u;
in the separate dfp_prepare_decimal{64,128} functions. Which is basically what
we have before, only smooshing the a64 and b64 scratch-pads together, and using
a nice union instead of an ugly cast.
r~
[PATCH v3 26/33] .shippable.yml: Build WHPX enabled binaries
From: Philippe Mathieu-Daudé Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Alex Bennée Message-Id: <20190920113329.16787-4-phi...@redhat.com> --- .shippable.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.shippable.yml b/.shippable.yml index bbc6f88510f..01b33bd034e 100644 --- a/.shippable.yml +++ b/.shippable.yml @@ -25,6 +25,8 @@ env: TARGET_LIST=mips64el-softmmu,mips64el-linux-user - IMAGE=debian-ppc64el-cross TARGET_LIST=ppc64-softmmu,ppc64-linux-user,ppc64abi32-linux-user +- IMAGE=fedora-win10sdk-cross + TARGET_LIST=x86_64-softmmu,i386-softmmu build: pre_ci: # usually host ARCH is set by configure -- 2.20.1
[PATCH v3 29/33] docker: remove 'deprecated' image definitions
From: John Snow There isn't a debian.dockerfile anymore, so perform some ghost-busting. Signed-off-by: John Snow Message-Id: <20190923181140.7235-4-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/Makefile.include | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 82d5a8a5393..fd6f470fbf8 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -4,11 +4,10 @@ DOCKER_SUFFIX := .docker DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles -DOCKER_DEPRECATED_IMAGES := debian # we don't run tests on intermediate images (used as base by another image) -DOCKER_PARTIAL_IMAGES := debian debian9 debian10 debian-sid +DOCKER_PARTIAL_IMAGES := debian9 debian10 debian-sid DOCKER_PARTIAL_IMAGES += debian9-mxe debian-ports debian-bootstrap -DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker) +DOCKER_IMAGES := $(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES)) # Use a global constant ccache directory to speed up repetitive builds DOCKER_CCACHE_DIR := $$HOME/.cache/qemu-docker-ccache @@ -160,7 +159,7 @@ docker-image-debian-powerpc-user-cross: docker-binfmt-image-debian-powerpc-user DOCKER_USER_IMAGES += debian-powerpc-user # Expand all the pre-requistes for each docker image and test combination -$(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES)), \ +$(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES)), \ $(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \ $(eval .PHONY: docker-$t@$i) \ $(eval docker-$t@$i: docker-image-$i docker-run-$t@$i) \ -- 2.20.1
[PATCH v3 25/33] tests/docker: Add fedora-win10sdk-cross image
From: Philippe Mathieu-Daudé
To build WHPX (Windows Hypervisor) binaries, we need the WHPX
headers provided by the Windows SDK.
Add a script that fetches the required MSI/CAB files from the
latest SDK (currently 10.0.18362.1).
Headers are accessible under /opt/win10sdk/include.
Set the QEMU_CONFIGURE_OPTS environment variable accordingly,
enabling HAX and WHPX. Due to CPP warnings related to Microsoft
specific #pragmas, we also need to use the '--disable-werror'
configure flag.
Cc: Justin Terry
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
Message-Id: <20190920113329.16787-3-phi...@redhat.com>
---
tests/docker/Makefile.include | 2 ++
.../dockerfiles/fedora-win10sdk-cross.docker | 23
tests/docker/dockerfiles/win10sdk-dl.sh | 27 +++
3 files changed, 52 insertions(+)
create mode 100644 tests/docker/dockerfiles/fedora-win10sdk-cross.docker
create mode 100755 tests/docker/dockerfiles/win10sdk-dl.sh
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 3fc7a863e51..e85e73025ba 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -125,6 +125,8 @@ docker-image-debian-ppc64-cross: docker-image-debian10
docker-image-debian-riscv64-cross: docker-image-debian10
docker-image-debian-sh4-cross: docker-image-debian10
docker-image-debian-sparc64-cross: docker-image-debian10
+docker-image-fedora-win10sdk-cross: docker-image-fedora
+docker-image-fedora-win10sdk-cross:
EXTRA_FILES:=$(DOCKER_FILES_DIR)/win10sdk-dl.sh
docker-image-travis: NOUSER=1
diff --git a/tests/docker/dockerfiles/fedora-win10sdk-cross.docker
b/tests/docker/dockerfiles/fedora-win10sdk-cross.docker
new file mode 100644
index 000..55ca933d40d
--- /dev/null
+++ b/tests/docker/dockerfiles/fedora-win10sdk-cross.docker
@@ -0,0 +1,23 @@
+#
+# Docker MinGW64 cross-compiler target with WHPX header installed
+#
+# This docker target builds on the Fedora 30 base image.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+FROM qemu:fedora
+
+RUN dnf install -y \
+cabextract \
+msitools \
+wget
+
+# Install WHPX headers from Windows Software Development Kit:
+# https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk
+ADD win10sdk-dl.sh /usr/local/bin/win10sdk-dl.sh
+RUN /usr/local/bin/win10sdk-dl.sh
+
+ENV QEMU_CONFIGURE_OPTS ${QEMU_CONFIGURE_OPTS} \
+--cross-prefix=x86_64-w64-mingw32- \
+--extra-cflags=-I/opt/win10sdk/include --disable-werror \
+--enable-hax --enable-whpx
diff --git a/tests/docker/dockerfiles/win10sdk-dl.sh
b/tests/docker/dockerfiles/win10sdk-dl.sh
new file mode 100755
index 000..1c35c2a2524
--- /dev/null
+++ b/tests/docker/dockerfiles/win10sdk-dl.sh
@@ -0,0 +1,27 @@
+#!/bin/bash
+#
+# Install WHPX headers from Windows Software Development Kit
+# https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+WINDIR=/opt/win10sdk
+mkdir -p ${WINDIR}
+pushd ${WINDIR}
+# Get the bundle base for Windows SDK v10.0.18362.1
+BASE_URL=$(curl --silent --include
'http://go.microsoft.com/fwlink/?prd=11966=1.0=0x409=0x409=Windows10=SDK=10.0.18362.1'
| sed -nE 's_Location: (.*)/\r_\1_p')/Installers
+# Fetch the MSI containing the headers
+wget --no-verbose ${BASE_URL}/'Windows SDK Desktop Headers x86-x86_en-us.msi'
+while true; do
+# Fetch all cabinets required by this MSI
+CAB_NAME=$(msiextract Windows\ SDK\ Desktop\ Headers\ x86-x86_en-us.msi
3>&1 2>&3 3>&-| sed -nE "s_.*Error opening file $PWD/(.*): No such file or
directory_\1_p")
+test -z "${CAB_NAME}" && break
+wget --no-verbose ${BASE_URL}/${CAB_NAME}
+done
+rm *.{cab,msi}
+mkdir /opt/win10sdk/include
+# Only keep the WHPX headers
+for inc in "${WINDIR}/Program Files/Windows
Kits/10/Include/10.0.18362.0/um"/WinHv*; do
+ln -s "${inc}" /opt/win10sdk/include
+done
+popd > /dev/null
--
2.20.1
[PATCH v3 30/33] docker: remove unused debian-ports
From: John Snow debian-ports is listed as a partial image, so we cannot run tests against it. Since it isn't used by any other testable image, remove it for now as it is prone to bitrot. Signed-off-by: John Snow Message-Id: <20190923181140.7235-5-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/Makefile.include| 2 +- tests/docker/dockerfiles/debian-ports.docker | 36 2 files changed, 1 insertion(+), 37 deletions(-) delete mode 100644 tests/docker/dockerfiles/debian-ports.docker diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index fd6f470fbf8..053c418d8cd 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -6,7 +6,7 @@ DOCKER_SUFFIX := .docker DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles # we don't run tests on intermediate images (used as base by another image) DOCKER_PARTIAL_IMAGES := debian9 debian10 debian-sid -DOCKER_PARTIAL_IMAGES += debian9-mxe debian-ports debian-bootstrap +DOCKER_PARTIAL_IMAGES += debian9-mxe debian-bootstrap DOCKER_IMAGES := $(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES)) # Use a global constant ccache directory to speed up repetitive builds diff --git a/tests/docker/dockerfiles/debian-ports.docker b/tests/docker/dockerfiles/debian-ports.docker deleted file mode 100644 index 61bc3f2993a..000 --- a/tests/docker/dockerfiles/debian-ports.docker +++ /dev/null @@ -1,36 +0,0 @@ -# -# Docker multiarch cross-compiler target -# -# This docker target is builds on Debian Ports cross compiler targets -# to build distro with a selection of cross compilers for building test binaries. -# -# On its own you can't build much but the docker-foo-cross targets -# build on top of the base debian image. -# -FROM debian:unstable - -MAINTAINER Philippe Mathieu-Daudé - -RUN echo "deb [arch=amd64] http://deb.debian.org/debian unstable main" > /etc/apt/sources.list - -# Duplicate deb line as deb-src -RUN cat /etc/apt/sources.list | sed -ne "s/^deb\ \(\[.*\]\ \)\?\(.*\)/deb-src \2/p" >> /etc/apt/sources.list - -# Setup some basic tools we need -RUN apt-get update && \ -DEBIAN_FRONTEND=noninteractive apt install -yy eatmydata && \ -DEBIAN_FRONTEND=noninteractive eatmydata \ -apt-get install -y --no-install-recommends \ -bison \ -build-essential \ -ca-certificates \ -clang \ -debian-ports-archive-keyring \ -flex \ -gettext \ -git \ -pkg-config \ -psmisc \ -python \ -texinfo \ -$(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\ -f2) -- 2.20.1
Re: [PATCH v3 23/25] Sockets: Fix error_append_hint usage
On 9/24/19 3:09 PM, Vladimir Sementsov-Ogievskiy wrote: > If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro. > Otherwise hint will not be appended in case of errp == _err > (program will exit before error_append_hint() call). Fix such cases. > > This commit (together with its neighbors) was generated by > > git grep -l 'error_append_hint(errp' | while read f; do \ > spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \ > --in-place $f; done > > and then > > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Greg Kurz > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > util/qemu-sockets.c | 2 ++ > 1 file changed, 2 insertions(+) git grep -p 'error_append_hint(errp' util/*sockets* nicely shows these same two functions. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[PATCH v3 28/33] docker: removed unused debian8 partial image
From: John Snow debian8 partial base is also not consumed by any image, so remove it. For QEMU's development cycle, we only support debian9 (stretch) and debian10 (buster). Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20190923181140.7235-3-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/Makefile.include | 2 +- tests/docker/dockerfiles/debian8.docker | 34 - 2 files changed, 1 insertion(+), 35 deletions(-) delete mode 100644 tests/docker/dockerfiles/debian8.docker diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 47d2273f29d..82d5a8a5393 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -6,7 +6,7 @@ DOCKER_SUFFIX := .docker DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles DOCKER_DEPRECATED_IMAGES := debian # we don't run tests on intermediate images (used as base by another image) -DOCKER_PARTIAL_IMAGES := debian debian8 debian9 debian10 debian-sid +DOCKER_PARTIAL_IMAGES := debian debian9 debian10 debian-sid DOCKER_PARTIAL_IMAGES += debian9-mxe debian-ports debian-bootstrap DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker) DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES)) diff --git a/tests/docker/dockerfiles/debian8.docker b/tests/docker/dockerfiles/debian8.docker deleted file mode 100644 index 1212a85c35b..000 --- a/tests/docker/dockerfiles/debian8.docker +++ /dev/null @@ -1,34 +0,0 @@ -# -# Docker multiarch cross-compiler target -# -# This docker target is builds on Debian and Emdebian's cross compiler targets -# to build distro with a selection of cross compilers for building test binaries. -# -# On its own you can't build much but the docker-foo-cross targets -# build on top of the base debian image. -# -FROM debian:jessie-slim - -MAINTAINER Philippe Mathieu-Daudé - -# Duplicate deb line as deb-src -RUN cat /etc/apt/sources.list | sed "s/^deb\ /deb-src /" >> /etc/apt/sources.list - -# Setup some basic tools we need -RUN apt update && \ -DEBIAN_FRONTEND=noninteractive apt-get install -yy eatmydata && \ -DEBIAN_FRONTEND=noninteractive eatmydata \ -apt-get install -y --no-install-recommends \ -bison \ -binutils-multiarch \ -build-essential \ -ca-certificates \ -clang \ -curl \ -flex \ -gettext \ -git \ -gnupg \ -pkg-config \ -python-minimal - -- 2.20.1
Re: [PATCH 6/7] target/ppc: use existing VsrD() macro to eliminate HI_IDX and LO_IDX from dfp_helper.c
On 24/09/2019 16:35, Mark Cave-Ayland wrote:
> Switch over all accesses to the decimal numbers held in struct PPC_DFP from
> using HI_IDX and LO_IDX to using the VsrD() macro instead. Not only does this
> allow the compiler to ensure that the various dfp_* functions are being passed
> a ppc_vsr_t rather than an arbitrary uint64_t pointer, but also allows the
> host endian-specific HI_IDX and LO_IDX to be completely removed from
> dfp_helper.c.
>
> Signed-off-by: Mark Cave-Ayland
> ---
> target/ppc/dfp_helper.c | 70 ++---
> 1 file changed, 31 insertions(+), 39 deletions(-)
>
> diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
> index ed437f97da..c2d335e928 100644
> --- a/target/ppc/dfp_helper.c
> +++ b/target/ppc/dfp_helper.c
> @@ -28,13 +28,6 @@
> #include "libdecnumber/dpd/decimal64.h"
> #include "libdecnumber/dpd/decimal128.h"
>
> -#if defined(HOST_WORDS_BIGENDIAN)
> -#define HI_IDX 0
> -#define LO_IDX 1
> -#else
> -#define HI_IDX 1
> -#define LO_IDX 0
> -#endif
>
> static void get_dfp64(ppc_vsr_t *dst, ppc_fprp_t *dfp)
> {
> @@ -1039,31 +1032,31 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t,
> ppc_fprp_t *b) \
> DFP_HELPER_CTFIX(dctfix, 64)
> DFP_HELPER_CTFIX(dctfixq, 128)
>
> -static inline void dfp_set_bcd_digit_64(uint64_t *t, uint8_t digit,
> -unsigned n)
> +static inline void dfp_set_bcd_digit_64(ppc_vsr_t *t, uint8_t digit,
> +unsigned n)
> {
> -*t |= ((uint64_t)(digit & 0xF) << (n << 2));
> +t->VsrD(1) |= ((uint64_t)(digit & 0xF) << (n << 2));
> }
>
> -static inline void dfp_set_bcd_digit_128(uint64_t *t, uint8_t digit,
> - unsigned n)
> +static inline void dfp_set_bcd_digit_128(ppc_vsr_t *t, uint8_t digit,
> + unsigned n)
> {
> -t[(n & 0x10) ? HI_IDX : LO_IDX] |=
> +t->VsrD((n & 0x10) ? 0 : 1) |=
> ((uint64_t)(digit & 0xF) << ((n & 15) << 2));
> }
>
> -static inline void dfp_set_sign_64(uint64_t *t, uint8_t sgn)
> +static inline void dfp_set_sign_64(ppc_vsr_t *t, uint8_t sgn)
> {
> -*t <<= 4;
> -*t |= (sgn & 0xF);
> +t->VsrD(1) <<= 4;
> +t->VsrD(1) |= (sgn & 0xF);
> }
>
> -static inline void dfp_set_sign_128(uint64_t *t, uint8_t sgn)
> +static inline void dfp_set_sign_128(ppc_vsr_t *t, uint8_t sgn)
> {
> -t[HI_IDX] <<= 4;
> -t[HI_IDX] |= (t[LO_IDX] >> 60);
> -t[LO_IDX] <<= 4;
> -t[LO_IDX] |= (sgn & 0xF);
> +t->VsrD(0) <<= 4;
> +t->VsrD(0) |= (t->VsrD(0) >> 60) ^^
I've just noticed that I've made a typo here: the line above should of course
read:
t->VsrD(0) |= (t->VsrD(1) >> 60);
> +t->VsrD(1) <<= 4;
> +t->VsrD(1) |= (sgn & 0xF);
> }
>
> #define DFP_HELPER_DEDPD(op, size)\
> @@ -1081,7 +1074,7 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t,
> ppc_fprp_t *b, \
> N = dfp.b.digits; \
>\
> for (i = 0; (i < N) && (i < (size)/4); i++) { \
> -dfp_set_bcd_digit_##size([0], digits[N - i - 1], i); \
> +dfp_set_bcd_digit_##size(, digits[N - i - 1], i); \
> } \
>\
> if (sp & 2) { \
> @@ -1092,7 +1085,7 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t,
> ppc_fprp_t *b, \
> } else { \
> sgn = ((sp & 1) ? 0xF : 0xC); \
> } \
> -dfp_set_sign_##size([0], sgn); \
> +dfp_set_sign_##size(, sgn);\
> } \
>\
> if (size == 64) { \
> @@ -1105,14 +1098,14 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t,
> ppc_fprp_t *b, \
> DFP_HELPER_DEDPD(ddedpd, 64)
> DFP_HELPER_DEDPD(ddedpdq, 128)
>
> -static inline uint8_t dfp_get_bcd_digit_64(uint64_t *t, unsigned n)
> +static inline uint8_t dfp_get_bcd_digit_64(ppc_vsr_t *t, unsigned n)
> {
> -return *t >> ((n << 2) & 63) & 15;
> +return t->VsrD(1) >> ((n << 2) & 63) & 15;
> }
>
> -static inline uint8_t dfp_get_bcd_digit_128(uint64_t *t, unsigned n)
> +static inline uint8_t dfp_get_bcd_digit_128(ppc_vsr_t *t, unsigned n)
> {
> -return t[(n & 0x10) ? HI_IDX :
[PATCH v3 32/33] docker: move tests from python2 to python3
From: John Snow As part of the push to drop python2 support, replace any explicit python2 dependencies with python3 versions. For centos, python2 still exists as an implicit dependency, but by adding python3 we will be able to build even if the configure script begins to require python 3.5+. Tested with centos7, fedora, ubuntu, ubuntu1804, and debian 9 (amd64). Tested under a custom configure script that requires Python 3.5+. the travis dockerfile is also moved to using python3, which was tested by running `make docker-test-build@travis`, which I hope is sufficient. Signed-off-by: John Snow Message-Id: <20190923181140.7235-7-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/dockerfiles/centos7.docker | 2 +- tests/docker/dockerfiles/debian-xtensa-cross.docker | 2 +- tests/docker/dockerfiles/debian10.docker| 2 +- tests/docker/dockerfiles/debian9.docker | 2 +- tests/docker/dockerfiles/travis.docker | 2 +- tests/docker/dockerfiles/ubuntu.docker | 2 +- tests/docker/dockerfiles/ubuntu1804.docker | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/docker/dockerfiles/centos7.docker b/tests/docker/dockerfiles/centos7.docker index e0b9d7dbe9f..953637065c4 100644 --- a/tests/docker/dockerfiles/centos7.docker +++ b/tests/docker/dockerfiles/centos7.docker @@ -25,6 +25,7 @@ ENV PACKAGES \ nettle-devel \ perl-Test-Harness \ pixman-devel \ +python3 \ SDL-devel \ spice-glib-devel \ spice-server-devel \ @@ -34,4 +35,3 @@ ENV PACKAGES \ zlib-devel RUN yum install -y $PACKAGES RUN rpm -q $PACKAGES | sort > /packages.txt - diff --git a/tests/docker/dockerfiles/debian-xtensa-cross.docker b/tests/docker/dockerfiles/debian-xtensa-cross.docker index b9c2e2e5317..e6f93f65ee2 100644 --- a/tests/docker/dockerfiles/debian-xtensa-cross.docker +++ b/tests/docker/dockerfiles/debian-xtensa-cross.docker @@ -18,7 +18,7 @@ RUN apt-get update && \ flex \ gettext \ git \ -python-minimal +python3-minimal ENV CPU_LIST csp dc232b dc233c ENV TOOLCHAIN_RELEASE 2018.02 diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker index 30a78813f27..dad498b52e3 100644 --- a/tests/docker/dockerfiles/debian10.docker +++ b/tests/docker/dockerfiles/debian10.docker @@ -26,7 +26,7 @@ RUN apt update && \ git \ pkg-config \ psmisc \ -python \ +python3 \ python3-sphinx \ texinfo \ $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\ -f2) diff --git a/tests/docker/dockerfiles/debian9.docker b/tests/docker/dockerfiles/debian9.docker index b36f1d4ed83..8cbd742bb5f 100644 --- a/tests/docker/dockerfiles/debian9.docker +++ b/tests/docker/dockerfiles/debian9.docker @@ -26,7 +26,7 @@ RUN apt update && \ git \ pkg-config \ psmisc \ -python \ +python3 \ python3-sphinx \ texinfo \ $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\ -f2) diff --git a/tests/docker/dockerfiles/travis.docker b/tests/docker/dockerfiles/travis.docker index e72dc85ca7a..ea14da29d97 100644 --- a/tests/docker/dockerfiles/travis.docker +++ b/tests/docker/dockerfiles/travis.docker @@ -5,7 +5,7 @@ ENV LC_ALL en_US.UTF-8 RUN sed -i "s/# deb-src/deb-src/" /etc/apt/sources.list RUN apt-get update RUN apt-get -y build-dep qemu -RUN apt-get -y install device-tree-compiler python2.7 python-yaml dh-autoreconf gdb strace lsof net-tools gcovr +RUN apt-get -y install device-tree-compiler python3 python3-yaml dh-autoreconf gdb strace lsof net-tools gcovr # Travis tools require PhantomJS / Neo4j / Maven accessible # in their PATH (QEMU build won't access them). ENV PATH /usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker index a4f601395c8..f4864922240 100644 --- a/tests/docker/dockerfiles/ubuntu.docker +++ b/tests/docker/dockerfiles/ubuntu.docker @@ -60,7 +60,7 @@ ENV PACKAGES flex bison \ libvte-2.91-dev \ libxen-dev \ make \ -python-yaml \ +python3-yaml \ python3-sphinx \ sparse \ texinfo \ diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker index 883f9bcf31c..3cc4f492c4a 100644 --- a/tests/docker/dockerfiles/ubuntu1804.docker +++ b/tests/docker/dockerfiles/ubuntu1804.docker @@ -47,7 +47,7 @@ ENV PACKAGES flex bison \ libvte-2.91-dev \ libxen-dev \ make \ -python-yaml \ +python3-yaml \ python3-sphinx \ sparse \ texinfo \ -- 2.20.1
[PATCH v3 14/33] tests/docker: remove python2.7 from debian9-mxe
From: John Snow When it was based on debian8 which uses python-minimal, it needed this. It no longer does. Goodbye, python2.7. Signed-off-by: John Snow Message-Id: <20190918222546.11696-1-js...@redhat.com> [AJB: fixed up commit message] Signed-off-by: Alex Bennée --- tests/docker/dockerfiles/debian9-mxe.docker | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/docker/dockerfiles/debian9-mxe.docker b/tests/docker/dockerfiles/debian9-mxe.docker index 7431168dad9..62ff1cecf2d 100644 --- a/tests/docker/dockerfiles/debian9-mxe.docker +++ b/tests/docker/dockerfiles/debian9-mxe.docker @@ -16,7 +16,6 @@ RUN apt-key adv --keyserver keyserver.ubuntu.com --recv-keys C6BF758A33A3A276 && RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive eatmydata \ apt-get install -y --no-install-recommends \ -libpython2.7-stdlib \ $(apt-get -s install -y --no-install-recommends gw32.shared-mingw-w64 | egrep "^Inst mxe-x86-64-unknown-" | cut -d\ -f2) -ENV PATH $PATH:/usr/lib/mxe/usr/bin/ +ENV PATH $PATH:/usr/lib/mxe/usr/bin/ -- 2.20.1
[PATCH v3 33/33] tests/docker: remove debian-powerpc-user-cross
Despite our attempts in 4d26c7fef4 to keep this going it still gets in
the way of "make docker-test-build" completing because of course we
can't build a modern QEMU with the image. Let's put the thing out of
it's misery and remove it.
People who really care about building on powerpc can still use the
binfmt_misc support to manually build an image (or just run the build
from pre this commit).
Signed-off-by: Alex Bennée
Cc: Mark Cave-Ayland
---
tests/docker/Makefile.include | 9
.../debian-powerpc-user-cross.docker | 21 ---
2 files changed, 30 deletions(-)
delete mode 100644 tests/docker/dockerfiles/debian-powerpc-user-cross.docker
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 180e5439ef9..dcc37093138 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -149,15 +149,6 @@ DOCKER_PARTIAL_IMAGES += fedora-i386-cross
fedora-cris-cross
# work around issues with poorly working multi-arch systems and broken
# packages.
-# Jessie is the last supported release for powerpc, but multi-arch is
-# broken so we need a qemu-linux-user for this target
-docker-binfmt-image-debian-powerpc-user: DEB_ARCH = powerpc
-docker-binfmt-image-debian-powerpc-user: DEB_TYPE = jessie
-docker-binfmt-image-debian-powerpc-user: DEB_URL =
http://snapshot.debian.org/archive/debian/20180615T211437Z
-docker-binfmt-image-debian-powerpc-user: EXECUTABLE =
${BUILD_DIR}/ppc-linux-user/qemu-ppc
-docker-image-debian-powerpc-user-cross: docker-binfmt-image-debian-powerpc-user
-DOCKER_USER_IMAGES += debian-powerpc-user
-
# Expand all the pre-requistes for each docker image and test combination
$(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES)), \
$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
diff --git a/tests/docker/dockerfiles/debian-powerpc-user-cross.docker
b/tests/docker/dockerfiles/debian-powerpc-user-cross.docker
deleted file mode 100644
index 83749b0abb8..000
--- a/tests/docker/dockerfiles/debian-powerpc-user-cross.docker
+++ /dev/null
@@ -1,21 +0,0 @@
-#
-# Docker powerpc cross-compiler target for QEMU
-#
-# We can't use current Debian stable cross-compilers to build powerpc
-# as it has been dropped as a release architecture. Using Debian Sid
-# is just far too sketchy a build environment. This leaves us the
-# final option of using linux-user. This image is based of the
-# debootstrapped qemu:debian-powerpc-user but doesn't need any extra
-# magic once it is setup.
-#
-# It can be used to build old versions of QEMU, current versions need
-# newer dependencies than Jessie provides.
-#
-FROM qemu:debian-powerpc-user
-
-RUN echo man-db man-db/auto-update boolean false | debconf-set-selections
-RUN apt-get update && \
-DEBIAN_FRONTEND=noninteractive apt-get build-dep -yy qemu
-
-ENV QEMU_CONFIGURE_OPTS --disable-werror
-ENV DEF_TARGET_LIST powerpc-softmmu,arm-linux-user,aarch64-linux-user
--
2.20.1
[PATCH v3 18/33] tests/tcg: re-enable linux-test for ppc64abi32
Now we have fixed the signal delivary bug we can remove this horrible hack from the system. Cc: Richard Henderson Signed-off-by: Alex Bennée --- v2 - drop un-needed cflags --- tests/tcg/multiarch/Makefile.target | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target index 6b1e30e2fec..657a04f802d 100644 --- a/tests/tcg/multiarch/Makefile.target +++ b/tests/tcg/multiarch/Makefile.target @@ -12,14 +12,6 @@ VPATH+= $(MULTIARCH_SRC) MULTIARCH_SRCS =$(notdir $(wildcard $(MULTIARCH_SRC)/*.c)) MULTIARCH_TESTS =$(MULTIARCH_SRCS:.c=) -# FIXME: ppc64abi32 linux-test seems to have issues but the other basic tests work -ifeq ($(TARGET_NAME),ppc64abi32) -BROKEN_TESTS = linux-test -endif - -# Update TESTS -TESTS += $(filter-out $(BROKEN_TESTS), $(MULTIARCH_TESTS)) - # # The following are any additional rules needed to build things # @@ -39,3 +31,6 @@ run-test-mmap: test-mmap run-test-mmap-%: test-mmap $(call run-test, test-mmap-$*, $(QEMU) -p $* $<,\ "$< ($* byte pages) on $(TARGET_NAME)") + +# Update TESTS +TESTS += $(MULTIARCH_TESTS) -- 2.20.1
[PATCH v3 16/33] podman: fix command invocation
From: John Snow Oops; there's no argv here. Signed-off-by: John Snow Message-Id: <20190913193821.17756-1-js...@redhat.com> Signed-off-by: Alex Bennée Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa --- tests/docker/docker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/docker.py b/tests/docker/docker.py index 3112892fdf5..31d8adf836e 100755 --- a/tests/docker/docker.py +++ b/tests/docker/docker.py @@ -332,7 +332,7 @@ class Docker(object): cmd = [ "-u", str(uid) ] + cmd # podman requires a bit more fiddling if self._command[0] == "podman": -argv.insert(0, '--userns=keep-id') +cmd.insert(0, '--userns=keep-id') ret = self._do_check(["run", "--label", "com.qemu.instance.uuid=" + label] + cmd, -- 2.20.1
[PATCH v3 15/33] tests/docker: reduce scary warnings by cleaning up clean up
There was in the clean-up code caused by attempting to inspect images
which finished before we got there. Clean up the clean up code by:
- only track the one instance at a time
- use --filter for docker ps instead of doing it by hand
- just call docker rm -f to be done with it
- use uuid.uuid4() for a random uid
Signed-off-by: Alex Bennée
---
v2
- drop the try/except approach and be smarter
- use uuid4 as uuid1 can generate clashes in parallel builds
fixup! tests/docker: reduce scary warnings by cleaning up clean up
---
tests/docker/docker.py | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 29613afd489..3112892fdf5 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -215,7 +215,7 @@ class Docker(object):
""" Running Docker commands """
def __init__(self):
self._command = _guess_engine_command()
-self._instances = []
+self._instance = None
atexit.register(self._kill_instances)
signal.signal(signal.SIGTERM, self._kill_instances)
signal.signal(signal.SIGHUP, self._kill_instances)
@@ -234,21 +234,19 @@ class Docker(object):
cmd = ["ps", "-q"]
if not only_active:
cmd.append("-a")
+
+filter = "--filter=label=com.qemu.instance.uuid"
+if only_known:
+if self._instance:
+filter += "=%s" % (self._instance)
+else:
+# no point trying to kill, we finished
+return
+
+print("filter=%s" % (filter))
+cmd.append(filter)
for i in self._output(cmd).split():
-resp = self._output(["inspect", i])
-labels = json.loads(resp)[0]["Config"]["Labels"]
-active = json.loads(resp)[0]["State"]["Running"]
-if not labels:
-continue
-instance_uuid = labels.get("com.qemu.instance.uuid", None)
-if not instance_uuid:
-continue
-if only_known and instance_uuid not in self._instances:
-continue
-print("Terminating", i)
-if active:
-self._do(["kill", i])
-self._do(["rm", i])
+self._do(["rm", "-f", i])
def clean(self):
self._do_kill_instances(False, False)
@@ -325,9 +323,9 @@ class Docker(object):
return checksum == _text_checksum(_dockerfile_preprocess(dockerfile))
def run(self, cmd, keep, quiet, as_user=False):
-label = uuid.uuid1().hex
+label = uuid.uuid4().hex
if not keep:
-self._instances.append(label)
+self._instance = label
if as_user:
uid = os.getuid()
@@ -340,7 +338,7 @@ class Docker(object):
"com.qemu.instance.uuid=" + label] + cmd,
quiet=quiet)
if not keep:
-self._instances.remove(label)
+self._instance = None
return ret
def command(self, cmd, argv, quiet):
--
2.20.1
[PATCH v3 23/33] docs/devel: add "check-tcg" to testing.rst
It was pointed out we haven't documented the check-tcg part of the build system. Attempt to rectify that now. Signed-off-by: Alex Bennée --- docs/devel/testing.rst | 62 ++ 1 file changed, 62 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index bf75675fb04..1feee3ad101 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -266,6 +266,8 @@ another application on the host may have locked the file, possibly leading to a test failure. If using such devices are explicitly desired, consider adding ``locking=off`` option to disable image locking. +.. _docker-ref: + Docker based tests == @@ -799,3 +801,63 @@ And remove any package you want with:: If you've used ``make check-acceptance``, the Python virtual environment where Avocado is installed will be cleaned up as part of ``make check-clean``. + +Testing with "make check-tcg" += + +The check-tcg tests are intended for simple smoke tests of both +linux-user and softmmu TCG functionality. However to build test +programs for guest targets you need to have cross compilers available. +If your distribution supports cross compilers you can do something as +simple as:: + + apt install gcc-aarch64-linux-gnu + +The configure script will automatically pick up their presence. +Sometimes compilers have slightly odd names so the availability of +them can be prompted by passing in the appropriate configure option +for the architecture in question, for example:: + + $(configure) --cross-cc-aarch64=aarch64-cc + +There is also a ``--cross-cc-flags-ARCH`` flag in case additional +compiler flags are needed to build for a given target. + +If you have the ability to run containers as the user you can also +take advantage of the build systems "Docker" support. It will then use +containers to build any test case for an enabled guest where there is +no system compiler available. See :ref: `_docker-ref` for details. + +TCG test dependencies +- + +The TCG tests are deliberately very light on dependencies and are +either totally bare with minimal gcc lib support (for softmmu tests) +or just glibc (for linux-user tests). This is because getting a cross +compiler to work with additional libraries can be challenging. + +Other TCG Tests +--- + +There are a number of out-of-tree test suites that are used for more +extensive testing of processor features. + +KVM Unit Tests +~~ + +The KVM unit tests are designed to run as a Guest OS under KVM but +there is no reason why they can't exercise the TCG as well. It +provides a minimal OS kernel with hooks for enabling the MMU as well +as reporting test results via a special device:: + + https://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git + +Linux Test Project +~~ + +The LTP is focused on exercising the syscall interface of a Linux +kernel. It checks that syscalls behave as documented and strives to +exercise as many corner cases as possible. It is a useful test suite +to run to exercise QEMU's linux-user code:: + + https://linux-test-project.github.io/ -- 2.20.1
[PATCH v3 21/33] tests/tcg: add simple record/replay smoke test for aarch64
This adds two new tests that re-use the memory test to check basic record replay functionality is still working. We have to define our own runners rather than using the default pattern as we want to change the test name but re-use the memory binary. We declare the test binaries as PHONY as they don't rely exist. [AJB: A better test would output some sort of timer value or other otherwise variable value so we could compare the record and replay outputs and ensure they match] Signed-off-by: Alex Bennée Cc: Pavel Dovgalyuk --- tests/tcg/aarch64/Makefile.softmmu-target | 21 + 1 file changed, 21 insertions(+) diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target index 4c4aaf61dd3..b4b39579634 100644 --- a/tests/tcg/aarch64/Makefile.softmmu-target +++ b/tests/tcg/aarch64/Makefile.softmmu-target @@ -32,3 +32,24 @@ memory: CFLAGS+=-DCHECK_UNALIGNED=1 # Running QEMU_OPTS+=-M virt -cpu max -display none -semihosting-config enable=on,target=native,chardev=output -kernel + +# Simple Record/Replay Test +.PHONY: memory-record +run-memory-record: memory-record memory + $(call run-test, $<, \ + $(QEMU) -monitor none -display none \ + -chardev file$(COMMA)path=$<.out$(COMMA)id=output \ + -icount shift=5$(COMMA)rr=record$(COMMA)rrfile=record.bin \ + $(QEMU_OPTS) memory, \ + "$< on $(TARGET_NAME)") + +.PHONY: memory-replay +run-memory-replay: memory-replay run-memory-record + $(call run-test, $<, \ + $(QEMU) -monitor none -display none \ + -chardev file$(COMMA)path=$<.out$(COMMA)id=output \ + -icount shift=5$(COMMA)rr=replay$(COMMA)rrfile=record.bin \ + $(QEMU_OPTS) memory, \ + "$< on $(TARGET_NAME)") + +TESTS+=memory-record memory-replay -- 2.20.1
Re: [PATCH v3 18/25] block: Fix error_append_hint usage
On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote: > If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro. > Otherwise hint will not be appended in case of errp == _err > (program will exit before error_append_hint() call). Fix such cases. > > This commit (together with its neighbors) was generated by > > git grep -l 'error_append_hint(errp' | while read f; do \ > spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \ > --in-place $f; done > > and then > > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > Your automation is cool! > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Greg Kurz > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/backup.c | 1 + > block/dirty-bitmap.c | 1 + > block/file-posix.c | 3 +++ > block/gluster.c | 2 ++ > block/qcow.c | 1 + > block/qcow2.c| 1 + > block/vhdx-log.c | 1 + > block/vpc.c | 1 + > 8 files changed, 11 insertions(+) $ git grep -p 'error_append_hint(errp' block/ | grep '\.c=' | wc -l produces 11 hits, very nicely matching up with your patch. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[PATCH v3 10/33] migration/postcopy: Recognise the recovery states as 'in_postcopy'
From: "Dr. David Alan Gilbert"
Various parts of the migration code do different things when they're
in postcopy mode; prior to this patch this has been 'postcopy-active'.
This patch extends 'in_postcopy' to include 'postcopy-paused' and
'postcopy-recover'.
In particular, when you set the max-postcopy-bandwidth parameter, this
only affects the current migration fd if we're 'in_postcopy';
this leads to a race in the postcopy recovery test where it increases
the speed from 4k/sec to unlimited, but that increase can get ignored
if the change is made between the point at which the reconnection
happens and it transitions back to active.
Signed-off-by: Dr. David Alan Gilbert
Signed-off-by: Alex Bennée
Message-Id: <20190923174942.12182-1-dgilb...@redhat.com>
---
migration/migration.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 01863a95f5f..5f7e4d15e95 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1659,7 +1659,14 @@ bool migration_in_postcopy(void)
{
MigrationState *s = migrate_get_current();
-return (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
+switch (s->state) {
+case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+case MIGRATION_STATUS_POSTCOPY_PAUSED:
+case MIGRATION_STATUS_POSTCOPY_RECOVER:
+return true;
+default:
+return false;
+}
}
bool migration_in_postcopy_after_devices(MigrationState *s)
--
2.20.1
[PATCH v3 11/33] target/ppc: fix signal delivery for ppc64abi32
We were incorrectly using the 64-bit AIX ABI instead of the 32-bit
SYSV ABI for setting NIP for the signal handler.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Reviewed-by: Laurent Vivier
---
v2
- change to wording
---
linux-user/ppc/signal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index 619a56950df..5b82af6cb62 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -501,7 +501,9 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
int i, err = 0;
#if defined(TARGET_PPC64)
struct target_sigcontext *sc = 0;
+#if !defined(TARGET_ABI32)
struct image_info *image = ((TaskState *)thread_cpu->opaque)->info;
+#endif
#endif
rt_sf_addr = get_sigframe(ka, env, sizeof(*rt_sf));
@@ -557,7 +559,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
env->gpr[5] = (target_ulong) h2g(_sf->uc);
env->gpr[6] = (target_ulong) h2g(rt_sf);
-#if defined(TARGET_PPC64)
+#if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
if (get_ppc64_abi(image) < 2) {
/* ELFv1 PPC64 function pointers are pointers to OPD entries. */
struct target_func_ptr *handler =
--
2.20.1
[PATCH v3 07/33] target/alpha: Tidy helper_fp_exc_raise_s
From: Richard Henderson
Remove a redundant masking of ignore. Once that's gone it is
obvious that the system-mode inner test is redundant with the
outer test. Move the fpcr_exc_enable masking up and tidy.
No functional change.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-8-richard.hender...@linaro.org>
---
target/alpha/fpu_helper.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/target/alpha/fpu_helper.c b/target/alpha/fpu_helper.c
index 62a066d9020..df8b58963ba 100644
--- a/target/alpha/fpu_helper.c
+++ b/target/alpha/fpu_helper.c
@@ -90,25 +90,18 @@ void helper_fp_exc_raise_s(CPUAlphaState *env, uint32_t
ignore, uint32_t regno)
uint32_t exc = env->error_code & ~ignore;
if (exc) {
env->fpcr |= exc;
-exc &= ~ignore;
-#ifdef CONFIG_USER_ONLY
-/*
- * In user mode, the kernel's software handler only
- * delivers a signal if the exception is enabled.
- */
-if (!(exc & env->fpcr_exc_enable)) {
-return;
-}
-#else
+exc &= env->fpcr_exc_enable;
/*
* In system mode, the software handler gets invoked
* for any non-ignored exception.
+ * In user mode, the kernel's software handler only
+ * delivers a signal if the exception is enabled.
*/
+#ifdef CONFIG_USER_ONLY
if (!exc) {
return;
}
#endif
-exc &= env->fpcr_exc_enable;
fp_exc_raise1(env, GETPC(), exc, regno, EXC_M_SWC);
}
}
--
2.20.1
[PATCH v3 04/33] target/alpha: Handle SWCR_MAP_DMZ earlier
From: Richard Henderson
Since we're converting the swcr to fpcr format for exceptions,
it's trivial to add FPCR_DNZ to the set of fpcr bits overriden.
No functional change.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-5-richard.hender...@linaro.org>
---
target/alpha/helper.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index e21c488aa32..2f959c65efb 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -57,7 +57,7 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
* the software exception mask.
*/
uint32_t soft_fpcr = alpha_ieee_swcr_to_fpcr(env->swcr) >> 32;
-fpcr |= soft_fpcr & FPCR_STATUS_MASK;
+fpcr |= soft_fpcr & (FPCR_STATUS_MASK | FPCR_DNZ);
#endif
t |= CONVERT_BIT(fpcr, FPCR_INED, FPCR_INE);
@@ -73,9 +73,6 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
env->fpcr_flush_to_zero = (fpcr & FPCR_UNFD) && (fpcr & FPCR_UNDZ);
env->fp_status.flush_inputs_to_zero = (fpcr & FPCR_DNZ) != 0;
#ifdef CONFIG_USER_ONLY
-if (env->swcr & SWCR_MAP_DMZ) {
-env->fp_status.flush_inputs_to_zero = 1;
-}
if (env->swcr & SWCR_MAP_UMZ) {
env->fpcr_flush_to_zero = 1;
}
--
2.20.1
[PATCH v3 08/33] tests/migration: Fail on unexpected migration states
From: "Dr. David Alan Gilbert"
We've got various places where we wait for a migration to enter
a given state; but if we enter an unexpected state we tend to fail
in odd ways; add a mechanism for explicitly testing for any state
which we shouldn't be in.
Signed-off-by: Dr. David Alan Gilbert
Signed-off-by: Alex Bennée
Message-Id: <20190923131022.15498-2-dgilb...@redhat.com>
---
tests/migration-test.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/tests/migration-test.c b/tests/migration-test.c
index 258aa064d48..9c62ee5331b 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -255,15 +255,19 @@ static void read_blocktime(QTestState *who)
}
static void wait_for_migration_status(QTestState *who,
- const char *goal)
+ const char *goal,
+ const char **ungoals)
{
while (true) {
bool completed;
char *status;
+const char **ungoal;
status = migrate_query_status(who);
completed = strcmp(status, goal) == 0;
-g_assert_cmpstr(status, !=, "failed");
+for (ungoal = ungoals; *ungoal; ungoal++) {
+g_assert_cmpstr(status, !=, *ungoal);
+}
g_free(status);
if (completed) {
return;
@@ -274,7 +278,8 @@ static void wait_for_migration_status(QTestState *who,
static void wait_for_migration_complete(QTestState *who)
{
-wait_for_migration_status(who, "completed");
+wait_for_migration_status(who, "completed",
+ (const char * []) { "failed", NULL });
}
static void wait_for_migration_pass(QTestState *who)
@@ -809,7 +814,9 @@ static void test_postcopy_recovery(void)
* Wait until postcopy is really started; we can only run the
* migrate-pause command during a postcopy
*/
-wait_for_migration_status(from, "postcopy-active");
+wait_for_migration_status(from, "postcopy-active",
+ (const char * []) { "failed",
+ "completed", NULL });
/*
* Manually stop the postcopy migration. This emulates a network
@@ -822,7 +829,9 @@ static void test_postcopy_recovery(void)
* migrate-recover command can only succeed if destination machine
* is in the paused state
*/
-wait_for_migration_status(to, "postcopy-paused");
+wait_for_migration_status(to, "postcopy-paused",
+ (const char * []) { "failed", "active",
+ "completed", NULL });
/*
* Create a new socket to emulate a new channel that is different
@@ -836,7 +845,9 @@ static void test_postcopy_recovery(void)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-wait_for_migration_status(from, "postcopy-paused");
+wait_for_migration_status(from, "postcopy-paused",
+ (const char * []) { "failed", "active",
+ "completed", NULL });
migrate(from, uri, "{'resume': true}");
g_free(uri);
--
2.20.1
[PATCH v3 13/33] tests/docker: fix DOCKER_PARTIAL_IMAGES
Finger trouble in a previous clean-up inadvertently set DEBIAN_PARTIAL_IMAGES instead of DOCKER_PARTIAL_IMAGES. Also fix the typo to debian-9-mxe. Fixes: 44d5a8bf5d2 Signed-off-by: John Snow [AJB: merged fix from Message-Id: <20190917185537.25417-1-js...@redhat.com>] Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa --- tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 50a400b573a..3fc7a863e51 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -7,7 +7,7 @@ DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles DOCKER_DEPRECATED_IMAGES := debian # we don't run tests on intermediate images (used as base by another image) DOCKER_PARTIAL_IMAGES := debian debian8 debian9 debian10 debian-sid -DEBIAN_PARTIAL_IMAGES += debian8-mxe debian-9-mxe debian-ports debian-bootstrap +DOCKER_PARTIAL_IMAGES += debian8-mxe debian9-mxe debian-ports debian-bootstrap DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker) DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES)) # Use a global constant ccache directory to speed up repetitive builds -- 2.20.1
[PATCH v3 06/33] target/alpha: Mask IOV exception with INV for user-only
From: Richard Henderson The kernel masks the integer overflow exception with the software invalid exception mask. Include IOV in the set of exception bits masked by fpcr_exc_enable. Fixes the new float_convs test. Signed-off-by: Richard Henderson Signed-off-by: Alex Bennée Message-Id: <20190921043256.4575-7-richard.hender...@linaro.org> --- target/alpha/helper.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/target/alpha/helper.c b/target/alpha/helper.c index 1b3479738b7..55d7274d946 100644 --- a/target/alpha/helper.c +++ b/target/alpha/helper.c @@ -58,6 +58,13 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val) */ uint32_t soft_fpcr = alpha_ieee_swcr_to_fpcr(env->swcr) >> 32; fpcr |= soft_fpcr & (FPCR_STATUS_MASK | FPCR_DNZ); + +/* + * The IOV exception is disabled by the kernel with SWCR_TRAP_ENABLE_INV, + * which got mapped by alpha_ieee_swcr_to_fpcr to FPCR_INVD. + * Add FPCR_IOV to fpcr_exc_enable so that it is handled identically. + */ +t |= CONVERT_BIT(soft_fpcr, FPCR_INVD, FPCR_IOV); #endif t |= CONVERT_BIT(fpcr, FPCR_INED, FPCR_INE); -- 2.20.1
[PATCH v3 03/33] target/alpha: Fix SWCR_TRAP_ENABLE_MASK
From: Richard Henderson
The CONFIG_USER_ONLY adjustment blindly mashed the swcr
exception enable bits into the fpcr exception disable bits.
However, fpcr_exc_enable has already converted the exception
disable bits into the exception status bits in order to make
it easier to mask status bits at runtime.
Instead, merge the swcr enable bits with the fpcr before we
convert to status bits.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-4-richard.hender...@linaro.org>
---
target/alpha/helper.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 10602fb3394..e21c488aa32 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -46,34 +46,39 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
uint32_t fpcr = val >> 32;
uint32_t t = 0;
+/* Record the raw value before adjusting for linux-user. */
+env->fpcr = fpcr;
+
+#ifdef CONFIG_USER_ONLY
+/*
+ * Override some of these bits with the contents of ENV->SWCR.
+ * In system mode, some of these would trap to the kernel, at
+ * which point the kernel's handler would emulate and apply
+ * the software exception mask.
+ */
+uint32_t soft_fpcr = alpha_ieee_swcr_to_fpcr(env->swcr) >> 32;
+fpcr |= soft_fpcr & FPCR_STATUS_MASK;
+#endif
+
t |= CONVERT_BIT(fpcr, FPCR_INED, FPCR_INE);
t |= CONVERT_BIT(fpcr, FPCR_UNFD, FPCR_UNF);
t |= CONVERT_BIT(fpcr, FPCR_OVFD, FPCR_OVF);
t |= CONVERT_BIT(fpcr, FPCR_DZED, FPCR_DZE);
t |= CONVERT_BIT(fpcr, FPCR_INVD, FPCR_INV);
-env->fpcr = fpcr;
env->fpcr_exc_enable = ~t & FPCR_STATUS_MASK;
env->fpcr_dyn_round = rm_map[(fpcr & FPCR_DYN_MASK) >> FPCR_DYN_SHIFT];
env->fpcr_flush_to_zero = (fpcr & FPCR_UNFD) && (fpcr & FPCR_UNDZ);
env->fp_status.flush_inputs_to_zero = (fpcr & FPCR_DNZ) != 0;
-
#ifdef CONFIG_USER_ONLY
-/*
- * Override some of these bits with the contents of ENV->SWCR.
- * In system mode, some of these would trap to the kernel, at
- * which point the kernel's handler would emulate and apply
- * the software exception mask.
- */
if (env->swcr & SWCR_MAP_DMZ) {
env->fp_status.flush_inputs_to_zero = 1;
}
if (env->swcr & SWCR_MAP_UMZ) {
env->fpcr_flush_to_zero = 1;
}
-env->fpcr_exc_enable &= ~(alpha_ieee_swcr_to_fpcr(env->swcr) >> 32);
#endif
}
--
2.20.1
[PATCH v3 09/33] tests/migration/postcopy: trim migration bandwidth
From: "Dr. David Alan Gilbert" On slow hosts with tcg we were sometimes finding that the migration would complete during precopy and never get into the postcopy test. Trim back the bandwidth a bit to make that much less likely. Signed-off-by: Dr. David Alan Gilbert Signed-off-by: Alex Bennée Message-Id: <20190923131022.15498-3-dgilb...@redhat.com> --- tests/migration-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/migration-test.c b/tests/migration-test.c index 9c62ee5331b..221a33d0834 100644 --- a/tests/migration-test.c +++ b/tests/migration-test.c @@ -753,7 +753,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, * quickly, but that it doesn't complete precopy even on a slow * machine, so also set the downtime. */ -migrate_set_parameter_int(from, "max-bandwidth", 1); +migrate_set_parameter_int(from, "max-bandwidth", 3000); migrate_set_parameter_int(from, "downtime-limit", 1); /* Wait for the first serial output from the source */ -- 2.20.1
[PATCH v3 12/33] tests/docker: add sanitizers back to clang build
From: John Snow Fedora23 is but a distant twinkle. The sanitizer works again, and even if not, we have --enable-sanitizers now. Signed-off-by: John Snow Message-Id: <20190912014442.5757-1-js...@redhat.com> Signed-off-by: Alex Bennée --- tests/docker/test-clang | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/docker/test-clang b/tests/docker/test-clang index 324e341cea9..db9e6970b78 100755 --- a/tests/docker/test-clang +++ b/tests/docker/test-clang @@ -17,11 +17,7 @@ requires clang cd "$BUILD_DIR" -OPTS="--cxx=clang++ --cc=clang --host-cc=clang" -# -fsanitize=undefined is broken on Fedora 23, skip it for now -# See also: https://bugzilla.redhat.com/show_bug.cgi?id=1263834 -#OPTS="$OPTS --extra-cflags=-fsanitize=undefined \ -#--extra-cflags=-fno-sanitize=float-divide-by-zero" +OPTS="--cxx=clang++ --cc=clang --host-cc=clang --enable-sanitizers" build_qemu $OPTS check_qemu install_qemu -- 2.20.1
[PATCH v3 00/33] testing/next (docker,tcg, alpha ;-)
Hi, This should be the final iteration of the current testing/next queue before I send the Pull Request. Currently it contains some fixes from both David and Richard for migration and alpha respectively. I'm assuming David will get his PR in before I send mine but Richard is happy for me to merge the alpha changes. I've included the latest iteration of Jon's docker cleanups. There is also another attempt at reducing the warnings from docker due to the race between inspect and rm. Whilst digging into that I think I've also fixed another race caused by shared uuids while running very parallel builds. I also finally killed the powerpc-user-cross image which was still causing problems when trying to run all the test builds. I could have used the PARTIAL_IMAGE hack but then it would only been useful for testcases and we already have a nice cross compiler for those. The following patches need review 15 - tests docker reduce scary warnings by cleaning up 18 - tests tcg re enable linux test for ppc64abi32 21 - tests tcg add simple record replay smoke test for 23 - docs devel add check tcg to testing.rst 33 - tests docker remove debian powerpc user cross Alex Bennée (11): target/ppc: fix signal delivery for ppc64abi32 tests/docker: fix DOCKER_PARTIAL_IMAGES tests/docker: reduce scary warnings by cleaning up clean up tests/tcg: clean-up some comments after the de-tangling tests/tcg: re-enable linux-test for ppc64abi32 tests/tcg: add float_madds test to multiarch tests/tcg: add generic version of float_convs tests/tcg: add simple record/replay smoke test for aarch64 configure: preserve PKG_CONFIG for subdir builds docs/devel: add "check-tcg" to testing.rst tests/docker: remove debian-powerpc-user-cross Dr. David Alan Gilbert (3): tests/migration: Fail on unexpected migration states tests/migration/postcopy: trim migration bandwidth migration/postcopy: Recognise the recovery states as 'in_postcopy' John Snow (9): tests/docker: add sanitizers back to clang build tests/docker: remove python2.7 from debian9-mxe podman: fix command invocation docker: remove debian8-mxe definitions docker: removed unused debian8 partial image docker: remove 'deprecated' image definitions docker: remove unused debian-ports docker: remove unused debian-sid docker: move tests from python2 to python3 Philippe Mathieu-Daudé (3): target/i386: Fix broken build with WHPX enabled tests/docker: Add fedora-win10sdk-cross image .shippable.yml: Build WHPX enabled binaries Richard Henderson (7): target/alpha: Use array for FPCR_DYN conversion target/alpha: Fix SWCR_MAP_UMZ target/alpha: Fix SWCR_TRAP_ENABLE_MASK target/alpha: Handle SWCR_MAP_DMZ earlier target/alpha: Write to fpcr_flush_to_zero once target/alpha: Mask IOV exception with INV for user-only target/alpha: Tidy helper_fp_exc_raise_s .shippable.yml| 2 + Makefile | 6 +- configure | 1 + docs/devel/testing.rst| 62 ++ linux-user/ppc/signal.c | 4 +- migration/migration.c | 9 +- target/alpha/fpu_helper.c | 15 +- target/alpha/helper.c | 64 +- target/i386/whpx-all.c| 1 + tests/docker/Makefile.include | 20 +- tests/docker/docker.py| 36 +- tests/docker/dockerfiles/centos7.docker | 2 +- tests/docker/dockerfiles/debian-ports.docker | 36 - .../debian-powerpc-user-cross.docker | 21 - tests/docker/dockerfiles/debian-sid.docker| 35 - .../dockerfiles/debian-xtensa-cross.docker| 2 +- tests/docker/dockerfiles/debian10.docker | 2 +- tests/docker/dockerfiles/debian8.docker | 34 - tests/docker/dockerfiles/debian9-mxe.docker | 3 +- tests/docker/dockerfiles/debian9.docker | 2 +- .../dockerfiles/fedora-win10sdk-cross.docker | 23 + tests/docker/dockerfiles/travis.docker| 2 +- tests/docker/dockerfiles/ubuntu.docker| 2 +- tests/docker/dockerfiles/ubuntu1804.docker| 2 +- tests/docker/dockerfiles/win10sdk-dl.sh | 27 + tests/docker/test-clang | 6 +- tests/migration-test.c| 25 +- tests/tcg/Makefile.target | 16 +- tests/tcg/aarch64/Makefile.softmmu-target | 21 + tests/tcg/aarch64/Makefile.target | 3 +- tests/tcg/aarch64/float_convs.ref | 748 + tests/tcg/aarch64/float_madds.ref | 768 ++ tests/tcg/arm/Makefile.target | 16 +- tests/tcg/arm/float_convs.ref | 748 + tests/tcg/arm/float_madds.ref | 768 ++ tests/tcg/multiarch/Makefile.target | 23 +- tests/tcg/multiarch/float_convs.c
[PATCH v3 01/33] target/alpha: Use array for FPCR_DYN conversion
From: Richard Henderson
This is a bit more straight-forward than using a switch statement.
No functional change.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-2-richard.hender...@linaro.org>
---
target/alpha/helper.c | 24
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 19cda0a2db5..6c1703682e0 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -36,6 +36,13 @@ uint64_t cpu_alpha_load_fpcr(CPUAlphaState *env)
void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
{
+static const uint8_t rm_map[] = {
+[FPCR_DYN_NORMAL >> FPCR_DYN_SHIFT] = float_round_nearest_even,
+[FPCR_DYN_CHOPPED >> FPCR_DYN_SHIFT] = float_round_to_zero,
+[FPCR_DYN_MINUS >> FPCR_DYN_SHIFT] = float_round_down,
+[FPCR_DYN_PLUS >> FPCR_DYN_SHIFT] = float_round_up,
+};
+
uint32_t fpcr = val >> 32;
uint32_t t = 0;
@@ -48,22 +55,7 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
env->fpcr = fpcr;
env->fpcr_exc_enable = ~t & FPCR_STATUS_MASK;
-switch (fpcr & FPCR_DYN_MASK) {
-case FPCR_DYN_NORMAL:
-default:
-t = float_round_nearest_even;
-break;
-case FPCR_DYN_CHOPPED:
-t = float_round_to_zero;
-break;
-case FPCR_DYN_MINUS:
-t = float_round_down;
-break;
-case FPCR_DYN_PLUS:
-t = float_round_up;
-break;
-}
-env->fpcr_dyn_round = t;
+env->fpcr_dyn_round = rm_map[(fpcr & FPCR_DYN_MASK) >> FPCR_DYN_SHIFT];
env->fpcr_flush_to_zero = (fpcr & FPCR_UNFD) && (fpcr & FPCR_UNDZ);
env->fp_status.flush_inputs_to_zero = (fpcr & FPCR_DNZ) != 0;
--
2.20.1
[PATCH v3 02/33] target/alpha: Fix SWCR_MAP_UMZ
From: Richard Henderson
We were setting the wrong bit. The fp_status.flush_to_zero
setting is overwritten by either the constant 1 or the value
of fpcr_flush_to_zero depending on bits within an fp insn.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-3-richard.hender...@linaro.org>
---
target/alpha/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 6c1703682e0..10602fb3394 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -71,7 +71,7 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
env->fp_status.flush_inputs_to_zero = 1;
}
if (env->swcr & SWCR_MAP_UMZ) {
-env->fp_status.flush_to_zero = 1;
+env->fpcr_flush_to_zero = 1;
}
env->fpcr_exc_enable &= ~(alpha_ieee_swcr_to_fpcr(env->swcr) >> 32);
#endif
--
2.20.1
[PATCH v3 05/33] target/alpha: Write to fpcr_flush_to_zero once
From: Richard Henderson
Tidy the computation of the value; no functional change.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Message-Id: <20190921043256.4575-6-richard.hender...@linaro.org>
---
target/alpha/helper.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 2f959c65efb..1b3479738b7 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -69,14 +69,13 @@ void cpu_alpha_store_fpcr(CPUAlphaState *env, uint64_t val)
env->fpcr_exc_enable = ~t & FPCR_STATUS_MASK;
env->fpcr_dyn_round = rm_map[(fpcr & FPCR_DYN_MASK) >> FPCR_DYN_SHIFT];
-
-env->fpcr_flush_to_zero = (fpcr & FPCR_UNFD) && (fpcr & FPCR_UNDZ);
env->fp_status.flush_inputs_to_zero = (fpcr & FPCR_DNZ) != 0;
+
+t = (fpcr & FPCR_UNFD) && (fpcr & FPCR_UNDZ);
#ifdef CONFIG_USER_ONLY
-if (env->swcr & SWCR_MAP_UMZ) {
-env->fpcr_flush_to_zero = 1;
-}
+t |= (env->swcr & SWCR_MAP_UMZ) != 0;
#endif
+env->fpcr_flush_to_zero = t;
}
uint64_t helper_load_fpcr(CPUAlphaState *env)
--
2.20.1
Re: [PATCH 25/25] qapi: Improve source file read error handling
Eric Blake writes: > On 9/24/19 8:28 AM, Markus Armbruster wrote: >> qap-gen.py crashes when it can't open the main schema file, and when > > qapi-gen.py Will fix. >> it can't read from any schema file. Lazy. >> >> Change QAPISchema.__init__() to take a file name instead of a file >> object. Move the open code from _include() to __init__(), so it's >> used for the main schema file, too. >> >> Move the read into the try for good measure, and rephrase the error >> message. >> >> Reporting open or read failure for the main schema file needs a >> QAPISourceInfo representing "no source". Make QAPISourceInfo cope >> with fname=None. >> >> Signed-off-by: Markus Armbruster >> --- >> scripts/qapi/common.py| 46 +++ >> tests/qapi-schema/include-no-file.err | 2 +- >> 2 files changed, 27 insertions(+), 21 deletions(-) >> > > Reviewed-by: Eric Blake Thanks!
Re: [PATCH 21/25] qapi: Avoid redundant definition references in error messages
Eric Blake writes:
> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> Many error messages refer to the offending definition even though
>> they're preceded by an "in definition" line. Rephrase them.
>
> This is the cleanup promised earlier in the series.
>
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> scripts/qapi/common.py| 113 +++---
>> tests/qapi-schema/alternate-array.err | 2 +-
>
>> def check_command(expr, info):
>> -name = expr['command']
>> args = expr.get('data')
>> boxed = expr.get('boxed', False)
>>
>> if boxed and args is None:
>> raise QAPISemError(info, "'boxed': true requires 'data'")
>> -check_type(args, info, "'data' for command '%s'" % name,
>> - allow_dict=not boxed)
>> -check_type(expr.get('returns'), info,
>> - "'returns' for command '%s'" % name,
>> - allow_array=True)
>> +check_type(expr.get('data'), info, "'data'", allow_dict=not boxed)
>> +check_type(expr.get('returns'), info, "'returns'", allow_array=True)
>
> Why are you repeating expr.get('dat') here instead of reusing args? I
> guess it adds consistency with the expr.get('returns') in the next line.
Don't remember, might have been an accident. If I want consistency, I
guess I should add rets = expr.get('returns').
>>
>>
>> def check_event(expr, info):
>> -name = expr['event']
>> args = expr.get('data')
>> boxed = expr.get('boxed', False)
>>
>> if boxed and args is None:
>> raise QAPISemError(info, "'boxed': true requires 'data'")
>> -check_type(args, info, "'data' for event '%s'" % name,
>> - allow_dict=not boxed)
>> +check_type(expr.get('data'), info, "'data'", allow_dict=not boxed)
> Again, why not reuse args?
>
>
>> +++ b/tests/qapi-schema/args-member-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/args-member-case.json: In command
>> 'no-way-this-will-get-whitelisted':
>> -tests/qapi-schema/args-member-case.json:2: member of 'data' for command
>> 'no-way-this-will-get-whitelisted' uses uppercase in name 'Arg'
>> +tests/qapi-schema/args-member-case.json:2: 'data' member 'Arg' uses
>> uppercase in name 'Arg'
>
> Better, but still feels redundant for calling out 'Arg' twice. Maybe
> you further clean this one later?
check_type()'s error messages interpolate @source received from by
caller. Need to review all its messages and callers.
>> +++ b/tests/qapi-schema/enum-member-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/enum-member-case.json: In enum
>> 'NoWayThisWillGetWhitelisted':
>> -tests/qapi-schema/enum-member-case.json:4: member of enum
>> 'NoWayThisWillGetWhitelisted' uses uppercase in name 'Value'
>> +tests/qapi-schema/enum-member-case.json:4: 'data' member uses uppercase in
>> name 'Value'
>
> Here's a similar error about uppercase that does not have the
> redundancy, for comparison.
>
>
>> +++ b/tests/qapi-schema/union-branch-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/union-branch-case.json: In union 'Uni':
>> -tests/qapi-schema/union-branch-case.json:2: member of union 'Uni' uses
>> uppercase in name 'Branch'
>> +tests/qapi-schema/union-branch-case.json:2: 'data' member 'Branch' uses
>> uppercase in name 'Branch'
>
> Another related one.
>
>> +++ b/tests/qapi-schema/union-optional-branch.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/union-optional-branch.json: In union 'Union':
>> -tests/qapi-schema/union-optional-branch.json:2: member of union 'Union'
>> uses invalid name '*a'
>> +tests/qapi-schema/union-optional-branch.json:2: 'data' member '*a' uses
>> invalid name '*a'
>
> Similar type of redundancy, but this time not related to uppercase.
Same problem, different function: check_name_str().
> Overall an improvement.
>
> Reviewed-by: Eric Blake
Thanks!
Re: [PATCH 20/25] qapi: Improve reporting of missing / unknown definition keys
Eric Blake writes: > On 9/24/19 8:28 AM, Markus Armbruster wrote: >> Have check_exprs() call check_keys() later, so its error messages gain >> an "in definition" line. >> >> Both check_keys() and check_name_is_str() check the definition's name >> is a string. Since check_keys() now runs after check_name_is_str() >> rather than before, its check is dead. Bury it. Checking values in >> check_keys() is unclean anyway. >> >> Signed-off-by: Markus Armbruster >> --- > >> +++ b/scripts/qapi/common.py >> @@ -905,8 +905,6 @@ def check_known_keys(value, info, source, required, >> optional): >> >> def check_keys(expr, info, meta, required, optional=[]): >> name = expr[meta] >> -if not isinstance(name, str): >> -raise QAPISemError(info, "'%s' key must have a string value" % meta) > > Should this be replaced with an assert? But I'm also okay just dropping > it, since our testsuite shows that we still flag the problems that this > message was originally used for. I'd prefer not to assert, because as of this patch, check_keys() *only* checks keys, just like its name suggests. > Reviewed-by: Eric Blake Thanks!
Re: [PATCH v3 05/25] scripts: add coccinelle script to fix error_append_hint usage
On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote: > error_append_hint will not work, if errp == _error, as program > will exit before error_append_hint call. Fix this by use of special > macro ERRP_FUNCTION_BEGIN. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > .../fix-error_append_hint-usage.cocci | 25 +++ > 1 file changed, 25 insertions(+) > create mode 100644 scripts/coccinelle/fix-error_append_hint-usage.cocci > > +} > + > Why are you creating a file with a trailing blank line? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 05/25] scripts: add coccinelle script to fix error_append_hint usage
On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> error_append_hint will not work, if errp == _error, as program
> will exit before error_append_hint call. Fix this by use of special
> macro ERRP_FUNCTION_BEGIN.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
With the approach of a partial cleanup (rather than globally enforcing
it for all functions with errp parameter), we'll probably be rerunning
this Coccinelle script regularly, to track down any regressions.
> +++ b/scripts/coccinelle/fix-error_append_hint-usage.cocci
> @@ -0,0 +1,25 @@
> +@rule0@
> +// Add invocation to errp-functions
> +identifier fn;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> ++ ERRP_FUNCTION_BEGIN();
> +<+...
> +error_append_hint(errp, ...);
> +...+>
> + }
Does not catch the case that we want to also use the macro for any use
of *errp, but we can augment that later.
> +
> +@@
> +// Drop doubled invocation
> +identifier rule0.fn;
> +@@
> +
> + fn(...)
> +{
> +ERRP_FUNCTION_BEGIN();
> +- ERRP_FUNCTION_BEGIN();
> +...
> +}
This is smaller than the script you posted in v2, and thus I'm a bit
more confident in stating that it looks correct and idempotent.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 04/25] error: auto propagated local_err
On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> functions with errp parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
> +/*
> + * ERRP_FUNCTION_BEGIN
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * parameter.
Since you're going with the reduced scope of only touching functions
with error_append_hint, we may want to tweak this sentence to call out
only the functions that REQUIRE the use of this macro (if they contain
*errp or error_apend_hint), while mentioning that it is still safe to
use on any function with an errp parameter.
I'm hoping Markus will be able to chime in with his preferences soon.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hints (by error_append_hint)
> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).
> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
> + */
> +#define ERRP_FUNCTION_BEGIN() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +&__auto_errp_prop.local_err : errp)
> +
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: docker: how to use it when developing on QEMU?
John Snow writes: > On 9/24/19 11:25 AM, Philippe Mathieu-Daudé wrote: >> Recently more developers are enthusiast to use Docker/Podman, >> and have been confused by the different configurations currently in the >> QEMU repository. >> > > I think it's good evidence we need to improve the abstractions for this > testing module. It's not easy to know what to do without getting direct > feedback from the maintainers directly, which won't scale. It has certainly grown more warty as we've gone on. For one thing we don't cleanly handle different host architectures. > >> There are at least 3 kind of categories I use: >> >> 1/ Image used to build QEMU >> >> These images should be restricted/updated to our "supported targets". >> They are useful to (cross)build QEMU on variety of >> host/target/distributions/distrib_versions. >> >> Example: >> >> - cross build Cris binary using the Fedora 30 toolchain on a Ubuntu >> 18.04 x86_64. >> host:Ubuntu18.04/x86_64 docker_image:fedora-cris-cross >> > > Is this supposed to be a command invocation? Can we see full command > lines instead? make run-tcg-tests-cris-softmmu V=1 and you'll see the docker invocation underneath. > >> - cross build MinGW64 binary using Debian 9 MXE toolchain on a Ubuntu >> aarch64: >> host:Ubuntu18.04/x86_64 docker_image:debian-win64-cross >> >> An image can not be meant to use on a daily basis, but to avoid >> regression previous to release (I'd run them only on release candidate). >> > > Do you mean to say "These images are not meant to be used on a daily > basis, but instead on occasion to prevent regressions during release > candidate testing." ? > > They can build QEMU, but I assume they can't run any tests. Is there a > special "build-only" target that you can invoke from the Makefile to get > these to run? make docker-test-build Should run all QEMU builds on all images that support it. Works in testing/next. > >> Example: building QEMU for the Gentoo PlayStation2 port: >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg574468.html >> >> 2/ Image used to build test program used by QEMU >> > > I think this is the category most people are wanting to get their hands > on, usually. > >> These images provide enough to build binaries you can then use to test >> QEMU. If you want to build more of these binaries, there is probably a >> better way. Here we are only interested in testing. >> >> Example: >> >> - Test PowerPC Linux-user binaries with qemu-powerpc-linux-user >> docker_image:debian-powerpc-user-cross >> >> - Build EDK2 payload for Virt/AArch64 >> It currently doesn't build with Fedora 30 and I'v to use a Fedora 29 >> image. >> >> Another case I had is when I tried to build a kernel for the Mipssim >> machine (supported by QEMU). The Linux kernel code has been removed, so >> I had to checkout an old kernel which is not buildable with my recent >> host GCC. Using a docker based on a very old distribution worked. Anyway >> Thomas Huth found it is easier to use buildroot for pre-3.6 kernels. >> >> Similarly, I am testing QEMU port from Stefan Weil, and he shared a >> working binary supporting the MIPS AR7 target. To be able to use this >> QEMU I use Debian Lenny and set >> DEB_URL=https://snapshot.debian.org/archive/debian/20091004T111800Z. >> Yes, this will instanciate a Debian from 10 years ago. So generally once built you can re-use an image with cross compilers to build any random source tree. Usually you invoke docker directly with something like: docker run --rm -it -u $(id -u) -v $(pwd):$(pwd) -w $(pwd) qemu:debian-ubuntu-bionic-arm64 /bin/bash Which basically drops you into a shell with your build directory mapped into the docker image. However this usage is out of scope for the QEMU build machinery although a useful way to use the images. >> >> 3/ Bisecting >> >> Another of my docker uses is when bisecting before QEMU v3. I use image >> using snapshot slighly older than the QEMU release, so my bisect script >> can run without worrying about the library API incompatibilities or >> newer GCC warnings. >> > > Which images, tests, or invocations are useful for this? > >> So not all image have the same use. While they might not be useful to >> build the latest QEMU, there are still useful for day-to-day development. >> > > I would say that this has grown beyond the abstractions presented by the > Docker makefile, which makes it hard to figure out how to use any of the > tools in this directory. > >> Anyhow I agree we should document that better. Maybe the wiki is a good >> starting point. >> > > That might be a good place to organize your thoughts, but the source of > truth must be the Makefile help invocation. Agreed. > >> Regards, >> >> Phil. >> > > > We should split the images into a few categories in the makefile > directly, in a programmatic way; > > 1. Partial Images > > These images are not meant for building or testing anything directly. > They are dependencies; they should be better
Re: [PATCH 19/25] qapi: Improve reporting of invalid flags
Eric Blake writes: > On 9/24/19 8:28 AM, Markus Armbruster wrote: >> Split check_flags() off check_keys() and have check_exprs() call it >> later, so its error messages gain an "in definition" line. Tweak the >> error messages. >> >> Checking values in a function named check_keys() is unclean anyway. >> >> Signed-off-by: Markus Armbruster >> --- >> scripts/qapi/common.py | 22 -- >> tests/qapi-schema/allow-preconfig-test.err | 3 ++- >> tests/qapi-schema/args-bad-boxed.err | 3 ++- >> tests/qapi-schema/oob-test.err | 3 ++- >> tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- >> 5 files changed, 20 insertions(+), 14 deletions(-) >> > >> + >> +def check_flags(expr, info): >> +for key in ['gen', 'success-response']: >> +if key in expr and expr[key] is not False: > > Is it any more pythonic and/or a micro-optimization to compress this to: > > if expr.get(key, False) is not False: > >> +raise QAPISemError( >> +info, "flag '%s' may only use false value" % key) >> +for key in ['boxed', 'allow-oob', 'allow-preconfig']: >> +if key in expr and expr[key] is not True: > > and here too. Will do. > Reviewed-by: Eric Blake Thanks!
Re: [Qemu-devel] [PATCH 2/2] roms/Makefile.edk2: don't pull in submodules when building from tarball
On 09/20/19 21:50, Philippe Mathieu-Daudé wrote: > On 9/13/19 1:12 AM, Michael Roth wrote: >> Currently the `make efi` target pulls submodules nested under the >> roms/edk2 submodule as dependencies. However, when we attempt to build >> from a tarball this fails since we are no longer in a git tree. >> >> A preceding patch will pre-populate these submodules in the tarball, >> so assume this build dependency is only needed when building from a >> git tree. >> >> Reported-by: Bruce Rogers >> Cc: Laszlo Ersek >> Cc: Bruce Rogers >> Cc: qemu-sta...@nongnu.org # v4.1.0 >> Signed-off-by: Michael Roth >> --- >> roms/Makefile.edk2 | 7 ++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/roms/Makefile.edk2 b/roms/Makefile.edk2 >> index c2f2ff59d5..33a074d3a4 100644 >> --- a/roms/Makefile.edk2 >> +++ b/roms/Makefile.edk2 >> @@ -46,8 +46,13 @@ all: $(foreach >> flashdev,$(flashdevs),../pc-bios/edk2-$(flashdev).fd.bz2) \ >> # files. >> .INTERMEDIATE: $(foreach >> flashdev,$(flashdevs),../pc-bios/edk2-$(flashdev).fd) >> >> +# Fetch edk2 submodule's submodules. If it is not in a git tree, assume >> +# we're building from a tarball and that they've already been fetched by >> +# make-release/tarball scripts. > > Annoying, without using the make-release tool in a fresh clone, I get > qemu/roms/edk2/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf(-1): > error 000E: File/directory not found in workspace > > I vaguely remember there was a thread about not using 'git submodule > update --init --recursive' in the root directory but can't find it, any > idea? I think that was a point made in the first patch of this series -- the "--recursive" flag is unbounded, and it might pull in several unneeded submodules. For example, openssl itself appears to have three submodules (boringssl, krb5, pyca-cryptography), and none of those is needed for building openssl the way edk2 consumes it. I seem to remember that patch#1 in this series stated: we'd be handling the submodules on a case-by-case basis. Thanks Laszlo > >> submodules: >> -cd edk2 && git submodule update --init --force >> +if test -d edk2/.git; then \ >> +cd edk2 && git submodule update --init --force; \ >> +fi >> >> # See notes on the ".NOTPARALLEL" target and the "+" indicator in >> # "tests/uefi-test-tools/Makefile". >>
[PATCH v3 25/25] PVRDMA: Fix error_append_hint usage
If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == _err
(program will exit before error_append_hint() call). Fix such cases.
This commit (together with its neighbors) was generated by
git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done
and then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
(auto-msg was a file with this commit message)
Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.
Reported-by: Greg Kurz
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
hw/rdma/vmw/pvrdma_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3e36e13013..3cc02311bf 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -592,6 +592,7 @@ static void pvrdma_shutdown_notifier(Notifier *n, void
*opaque)
static void pvrdma_realize(PCIDevice *pdev, Error **errp)
{
+ERRP_FUNCTION_BEGIN();
int rc = 0;
PVRDMADev *dev = PVRDMA_DEV(pdev);
Object *memdev_root;
--
2.21.0
Re: [PATCH 16/25] qapi: Move context-sensitive checking to the proper place
Eric Blake writes:
> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> When we introduced the QAPISchema intermediate representation (commit
>> ac88219a6c7), we took a shortcut: we left check_exprs() & friends
>> alone instead of moving semantic checks into the
>> QAPISchemaFOO.check(). The .check() assert check_exprs() did its job.
>>
>> Time to finish the conversion job. Move exactly the context-sensitive
>> checks to the .check(). They replace assertions there. Context-free
>> checks stay put.
>>
>> Fixes the misleading optional tag error demonstrated by test
>> flat-union-optional-discriminator.
>>
>> A few other error message improve.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index f5599559ac..ac4c898e51 100644
>> --- a/scripts/qapi/common.py
>
> Thankfully, our large coverage of tests goes a long way to show that the
> conversion is correct. I didn't notice anything obvious that might have
> been overlooked (we may still find things down the road, but I'm not
> going to hold up this patch trying to find those things). Meanwhile,
> the conversion from assert to conditionals inside .check() looks complete.
Additional arguments supporting correctness:
* Every deleted check gets added back.
* Every added check replaces an assertion.
>> +++ b/tests/qapi-schema/args-union.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/args-union.json: In command 'oops':
>> -tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use
>> union type 'Uni'
>> +tests/qapi-schema/args-union.json:3: command's 'data' can take union type
>> 'Uni' only with 'boxed': true
>
> This one is definitely nicer.
>
>> +++ b/tests/qapi-schema/flat-union-discriminator-bad-name.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/flat-union-discriminator-bad-name.json: In union
>> 'MyUnion':
>> -tests/qapi-schema/flat-union-discriminator-bad-name.json:7: discriminator
>> of flat union 'MyUnion' uses invalid name '*switch'
>> +tests/qapi-schema/flat-union-discriminator-bad-name.json:6: discriminator
>> '*switch' is not a member of 'base'
>> diff --git a/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> b/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> index ea84b75cac..3ae8c06a89 100644
>> --- a/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> +++ b/tests/qapi-schema/flat-union-discriminator-bad-name.json
>> @@ -1,5 +1,4 @@
>> # discriminator '*switch' isn't a member of base, 'switch' is
>> -# reports "uses invalid name", which is good enough
>> { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
>> { 'struct': 'Base',
>>'data': { '*switch': 'Enum' } }
>
> I find this one to be borderline in quality (if we have '*switch' in the
> base, claiming that '*switch' is not a member of base is confusing until
> you realize that base actually has an optional member named 'switch') -
> but anyone that actually stumbles into this one will probably quickly
> figure out their problem, and we may be revisiting it later anyways when
> we finally include patches for a default discriminator.
>
>> +++ b/tests/qapi-schema/flat-union-optional-discriminator.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/flat-union-optional-discriminator.json: In union
>> 'MyUnion':
>> -tests/qapi-schema/flat-union-optional-discriminator.json:7: discriminator
>> 'switch' is not a member of 'base'
>> +tests/qapi-schema/flat-union-optional-discriminator.json:6: discriminator
>> member 'switch' of base type 'Base' must not be optional
>> diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json
>> b/tests/qapi-schema/flat-union-optional-discriminator.json
>> index 143ab23a0d..2e7f766f60 100644
>> --- a/tests/qapi-schema/flat-union-optional-discriminator.json
>> +++ b/tests/qapi-schema/flat-union-optional-discriminator.json
>> @@ -1,5 +1,4 @@
>> # we require the discriminator to be non-optional
>> -# FIXME reports "discriminator 'switch' is not a member of base struct
>> 'Base'"
>> { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
>> { 'struct': 'Base',
>>'data': { '*switch': 'Enum' } }
>
> And while the other one is borderline, I agree that this one is better.
>
>> +++ b/tests/qapi-schema/union-unknown.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/union-unknown.json: In union 'Union':
>> -tests/qapi-schema/union-unknown.json:2: member 'unknown' of union 'Union'
>> uses unknown type 'MissingType'
>> +tests/qapi-schema/union-unknown.json:2: union uses unknown type
>> 'MissingType'
>> diff --git a/tests/qapi-schema/union-unknown.json
>> b/tests/qapi-schema/union-unknown.json
>> index aa7e8143d8..64d3666176 100644
>> --- a/tests/qapi-schema/union-unknown.json
>> +++ b/tests/qapi-schema/union-unknown.json
>> @@ -1,3 +1,3 @@
>> # we reject a union with unknown type in branch
>> { 'union': 'Union',
>> - 'data': { 'unknown': 'MissingType' } }
>> + 'data': { 'unknown': ['MissingType'] } }
>>
>
> And here you covered one more code path by
[PATCH v3 20/25] cmdline: Fix error_append_hint usage
If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == _err
(program will exit before error_append_hint() call). Fix such cases.
This commit (together with its neighbors) was generated by
git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done
and then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
(auto-msg was a file with this commit message)
Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.
Reported-by: Greg Kurz
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
util/qemu-option.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..7136d83b1d 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -145,6 +145,7 @@ static const QemuOptDesc *find_desc_by_name(const
QemuOptDesc *desc,
void parse_option_size(const char *name, const char *value,
uint64_t *ret, Error **errp)
{
+ERRP_FUNCTION_BEGIN();
uint64_t size;
int err;
@@ -660,6 +661,7 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
int fail_if_exists, Error **errp)
{
+ERRP_FUNCTION_BEGIN();
QemuOpts *opts = NULL;
if (id) {
--
2.21.0
