Re: [PATCH for-6.1? v2 7/9] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write
On Sun, 2021-07-25 at 23:27 +0200, Philippe Mathieu-Daudé wrote:
> +Cédric/Benjamin
>
> On 7/25/21 2:24 PM, Richard Henderson wrote:
> > From clang-13:
> > hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used
> > \
> > [-Werror,-Wunused-but-set-variable]
> >
> > It's pretty clear that we meant to write back 'v' after
> > all that computation and not 'val'.
> >
>
> Fixes: 4f9924c4d4c ("ppc/pnv: Add models for POWER9 PHB4 PCIe Host
> bridge")
Acked-by: Benjamin Herrenschmidt
>
> > Acked-by: David Gibson
> > Signed-off-by: Richard Henderson
> > ---
> > hw/pci-host/pnv_phb4.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> > index 54f57c660a..5c375a9f28 100644
> > --- a/hw/pci-host/pnv_phb4.c
> > +++ b/hw/pci-host/pnv_phb4.c
> > @@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb,
> > uint64_t val)
> > v &= 0xull;
> > v |= 0xcfffull & val;
> > }
> > -*tptr = val;
> > +*tptr = v;
> > break;
> > }
> > case IODA3_TBL_MBT:
> >
Re: [PATCH v5 2/3] target/ppc: moved ppc_store_sdr1 to mmu_common.c
On Fri, Jul 23, 2021 at 02:56:26PM -0300, Lucas Mateus Castro (alqotel) wrote:
> ppc_store_sdr1 was at first in mmu_helper.c and was moved as part
> the patches to enable the disable-tcg option, now it's being moved
> back to a file that will be compiled with that option
>
> Signed-off-by: Lucas Mateus Castro (alqotel)
>
Applied to ppc-for-6.2, thanks.
> ---
> target/ppc/cpu.c| 28
> target/ppc/mmu_common.c | 26 ++
> 2 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index a29299882a..7ad9bd6044 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -67,34 +67,6 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
> return env->vscr | (sat << VSCR_SAT);
> }
>
> -#ifdef CONFIG_SOFTMMU
> -void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> -{
> -PowerPCCPU *cpu = env_archcpu(env);
> -qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
> -assert(!cpu->env.has_hv_mode || !cpu->vhyp);
> -#if defined(TARGET_PPC64)
> -if (mmu_is_64bit(env->mmu_model)) {
> -target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> -target_ulong htabsize = value & SDR_64_HTABSIZE;
> -
> -if (value & ~sdr_mask) {
> -qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
> - " set in SDR1", value & ~sdr_mask);
> -value &= sdr_mask;
> -}
> -if (htabsize > 28) {
> -qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x"
> TARGET_FMT_lx
> - " stored in SDR1", htabsize);
> -return;
> -}
> -}
> -#endif /* defined(TARGET_PPC64) */
> -/* FIXME: Should check for valid HTABMASK values in 32-bit case */
> -env->spr[SPR_SDR1] = value;
> -}
> -#endif /* CONFIG_SOFTMMU */
> -
> /* GDBstub can read and write MSR... */
> void ppc_store_msr(CPUPPCState *env, target_ulong value)
> {
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index ec4dce4ddc..a0518f611b 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -58,6 +58,32 @@
> # define LOG_BATS(...) do { } while (0)
> #endif
>
> +void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> +{
> +PowerPCCPU *cpu = env_archcpu(env);
> +qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
> +assert(!cpu->env.has_hv_mode || !cpu->vhyp);
> +#if defined(TARGET_PPC64)
> +if (mmu_is_64bit(env->mmu_model)) {
> +target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> +target_ulong htabsize = value & SDR_64_HTABSIZE;
> +
> +if (value & ~sdr_mask) {
> +qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
> + " set in SDR1", value & ~sdr_mask);
> +value &= sdr_mask;
> +}
> +if (htabsize > 28) {
> +qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x"
> TARGET_FMT_lx
> + " stored in SDR1", htabsize);
> +return;
> +}
> +}
> +#endif /* defined(TARGET_PPC64) */
> +/* FIXME: Should check for valid HTABMASK values in 32-bit case */
> +env->spr[SPR_SDR1] = value;
> +}
> +
>
> /*/
> /* PowerPC MMU emulation */
>
--
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 v5 1/3] target/ppc: divided mmu_helper.c in 2 files
On Fri, Jul 23, 2021 at 02:56:25PM -0300, Lucas Mateus Castro (alqotel) wrote:
> Divided mmu_helper.c in 2 files, functions inside #ifdef CONFIG_SOFTMMU
> stayed in mmu_helper.c, other functions moved to mmu_common.c. Updated
> meson.build to compile mmu_common.c and only compile mmu_helper.c when
> CONFIG_TCG is set.
> Moved function declarations, #define and structs used by both files to
> internal.h except for functions that use structures defined in cpu.h,
> those were moved to cpu.h.
>
> Signed-off-by: Lucas Mateus Castro (alqotel)
>
Applied to ppc-for-6.2, thanks.
> ---
> Used mmu_helper.c copyright header in mmu_common.c.
> ---
> target/ppc/cpu.h|9 +
> target/ppc/internal.h | 39 +
> target/ppc/meson.build |8 +-
> target/ppc/mmu_common.c | 1604 +++
> target/ppc/mmu_helper.c | 1590 +-
> 5 files changed, 1658 insertions(+), 1592 deletions(-)
> create mode 100644 target/ppc/mmu_common.c
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 93d308ac8f..500205229c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1330,6 +1330,15 @@ void store_booke_tsr(CPUPPCState *env, target_ulong
> val);
> void ppc_tlb_invalidate_all(CPUPPCState *env);
> void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr);
> void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> +int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
> +hwaddr *raddrp, target_ulong address,
> +uint32_t pid);
> +int ppcemb_tlb_check(CPUPPCState *env, ppcemb_tlb_t *tlb,
> +hwaddr *raddrp,
> +target_ulong address, uint32_t pid, int ext,
> +int i);
> +hwaddr booke206_tlb_to_page_size(CPUPPCState *env,
> +ppcmas_tlb_t *tlb);
> #endif
> #endif
>
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index f1fd3c8d04..b71406fa46 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -245,4 +245,43 @@ static inline int prot_for_access_type(MMUAccessType
> access_type)
> g_assert_not_reached();
> }
>
> +/* PowerPC MMU emulation */
> +
> +typedef struct mmu_ctx_t mmu_ctx_t;
> +bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> + hwaddr *raddrp, int *psizep, int *protp,
> + int mmu_idx, bool guest_visible);
> +int get_physical_address_wtlb(CPUPPCState *env, mmu_ctx_t *ctx,
> + target_ulong eaddr,
> + MMUAccessType access_type, int type,
> + int mmu_idx);
> +/* Software driven TLB helpers */
> +int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr,
> +int way, int is_code);
> +/* Context used internally during MMU translations */
> +struct mmu_ctx_t {
> +hwaddr raddr; /* Real address */
> +hwaddr eaddr; /* Effective address */
> +int prot; /* Protection bits */
> +hwaddr hash[2];/* Pagetable hash values */
> +target_ulong ptem; /* Virtual segment ID | API */
> +int key; /* Access key*/
> +int nx;/* Non-execute area */
> +};
> +
> +/* Common routines used by software and hardware TLBs emulation */
> +static inline int pte_is_valid(target_ulong pte0)
> +{
> +return pte0 & 0x8000 ? 1 : 0;
> +}
> +
> +static inline void pte_invalidate(target_ulong *pte0)
> +{
> +*pte0 &= ~0x8000;
> +}
> +
> +#define PTE_PTEM_MASK 0x7FBF
> +#define PTE_CHECK_MASK (TARGET_PAGE_MASK | 0x7B)
> +
> +
> #endif /* PPC_INTERNAL_H */
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index a4f18ff414..b85f295703 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -37,11 +37,13 @@ ppc_softmmu_ss.add(files(
>'arch_dump.c',
>'machine.c',
>'mmu-hash32.c',
> - 'mmu_helper.c',
> + 'mmu_common.c',
>'monitor.c',
> ))
> -ppc_softmmu_ss.add(when: 'CONFIG_TCG', if_false: files(
> - 'tcg-stub.c'
> +ppc_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
> + 'mmu_helper.c',
> +), if_false: files(
> + 'tcg-stub.c',
> ))
>
> ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> new file mode 100644
> index 00..ec4dce4ddc
> --- /dev/null
> +++ b/target/ppc/mmu_common.c
> @@ -0,0 +1,1604 @@
> +/*
> + * PowerPC MMU, TLB, SLB and BAT emulation helpers for QEMU.
> + *
> + * Copyright (c) 2003-2007 Jocelyn Mayer
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software
Re: [PATCH v5 3/3] target/ppc: moved store_40x_sler to helper_regs.c
On Fri, Jul 23, 2021 at 02:56:27PM -0300, Lucas Mateus Castro (alqotel) wrote:
> moved store_40x_sler from mmu_common.c to helper_regs.c as it is
> a function to store a value in a special purpose register, so
> moving it to a file focused in special register manipulation
> is more appropriate.
>
> Signed-off-by: Lucas Mateus Castro (alqotel)
>
Applied to ppc-for-6.2, thanks.
> ---
> target/ppc/helper_regs.c | 12
> target/ppc/mmu_common.c | 10 --
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 3723872aa6..405450d863 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -258,6 +258,18 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value,
> int alter_hv)
> return excp;
> }
>
> +#ifdef CONFIG_SOFTMMU
> +void store_40x_sler(CPUPPCState *env, uint32_t val)
> +{
> +/* XXX: TO BE FIXED */
> +if (val != 0x) {
> +cpu_abort(env_cpu(env),
> + "Little-endian regions are not supported by now\n");
> +}
> +env->spr[SPR_405_SLER] = val;
> +}
> +#endif /* CONFIG_SOFTMMU */
> +
> #ifndef CONFIG_USER_ONLY
> void check_tlb_flush(CPUPPCState *env, bool global)
> {
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index a0518f611b..754509e556 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -622,16 +622,6 @@ static int mmu40x_get_physical_address(CPUPPCState *env,
> mmu_ctx_t *ctx,
> return ret;
> }
>
> -void store_40x_sler(CPUPPCState *env, uint32_t val)
> -{
> -/* XXX: TO BE FIXED */
> -if (val != 0x) {
> -cpu_abort(env_cpu(env),
> - "Little-endian regions are not supported by now\n");
> -}
> -env->spr[SPR_405_SLER] = val;
> -}
> -
> static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
>hwaddr *raddr, int *prot, target_ulong address,
>MMUAccessType access_type, int i)
--
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
[Bug 721825] Re: VDI block driver bugs
[Expired for QEMU because there has been no activity for 60 days.]
** Changed in: qemu
Status: Incomplete => Expired
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/721825
Title:
VDI block driver bugs
Status in QEMU:
Expired
Bug description:
Chunqiang Tang reports the following issues with the VDI block driver,
these are present in QEMU 0.14:
"Bug 1. The most serious bug is caused by race condition in updating a new
bmap entry in memory and on disk. Considering the following operation
sequence.
O1: VM issues a write to sector X
O2: VDI allocates a new bmap entry and updates in-memory s->bmap
O3: VDI writes data to disk
O4: The disk I/O for writing sector X fails
O5: VDI reports error to VM and returns.
Note that the bmap entry is updated in memory, but not persisted on disk.
Now consider another write that immediately follows:
P1: VM issues a write to sector X+1, which locates in the same block as
the previously used sector X.
P2: s->bmap already has one entry for the block, and hence VDI writes
data directly without persisting the new s->bmap entry on disk.
P3: The write disk I/O succeeds
P4: VDI report success to VM, but the bitmap entry is still not
persisted on disk.
Now suppose the VM powers off gracefully (i.e., the QEMU process quits)
and reboots. The second write to sector X+1, which is reported as finished
successfully, is simply lost, because the corresponding in-memory s->bmap
entry is never persisted on disk. This is exactly what FVD's testing tool
discovers. After the block device is closed and then re-opened, disk
content verification fails.
This is just one example of the problem. Race condition plus host crash
also causes problems. Consider another example below.
Q1: VM issues a write to sector X
Q2: VDI allocates a new bmap entry and updates in-memory s->bmap
Q3: VDI writes sector X to disk and waits for the callback
Q4: VM issues a write to another sector X+1, which is in the same block
as sector X.
Q5: VDI sees the bitmap entry in s->bmap is already allocated, and
writes sector X+1 to disk.
Q6: Write to sector X+1 finishes, and VDI's callback is invoked.
Q7: VDI acknowledges to the VM the completion of writing sector X+1
Q8: After observing the completion of writing sector X+1, VM issues a
flush to ensure that sector X+1 is persisted on disk.
Q9: VDI finishes the flush and acknowledge the completion of the
operation.
Q10: ... (some other arbitrary operations, but the disk I/O for writing
sector X is still not finished)
Q11: The host crashes
Now the new bitmap entry is not persisted on disk, while both writing to
sector X+1 and the flush has been acknowledged as finished. Sector X+1 is
lost, which is a corruption. This problem exists even if it uses O_DSYNC.
The root cause of the problem is that, if a request updates in-memory
s->bmap, another request that sees this update assumes that the update is
already persisted on disk, which is not.
Bug 2: Similar to the bugs the FVD testing tool found for QCOW2, there are
several cases of the code below on failure handling path without setting
error return code, which mistakenly reports failure as success. This
mistake is caught by FVD when doing image content validation.
if (acb->hd_aiocb == NULL) {
/* missing ret = -EIO; */
goto done;
}
Bug 3: Similar to the bugs the FVD testing tool found for QCOW2,
vdi_aio_cancel does not perform a complete clean up and there are several
related bugs. First, memory buffer is not freed, acb->orig_buf and
acb->block_buffer. Second, acb->bh is not cancelled. Third,
vdi_aio_setup() does not initialize acb->bh to NULL so that when a request
acb is cancelled and then later reused for another request, its acb->bh !=
NULL and the new request fails in vdi_schedule_bh(). This is caught by
FVD's testing tool, when it observes that no I/O failure is injected but
VDI reports a failed I/O request, which indicates a bug in the driver."
http://permalink.gmane.org/gmane.comp.emulators.qemu/94340
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/721825/+subscriptions
RE: [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon Vector eXtensions (HVX) to core
> -Original Message-
> From: Richard Henderson
> Sent: Sunday, July 25, 2021 8:08 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; a...@rev.ng; Brian Cain ;
> peter.mayd...@linaro.org
> Subject: Re: [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon
> Vector eXtensions (HVX) to core
>
> On 7/5/21 1:34 PM, Taylor Simpson wrote:
> > HVX is a set of wide vector instructions. Machine state includes
> > vector registers (VRegs)
> > vector predicate registers (QRegs)
> > temporary registers for intermediate values
> > store buffer (masked stores and scatter/gather)
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/cpu.h| 35 -
> > target/hexagon/hex_arch_types.h | 5 +++
> > target/hexagon/insn.h | 3 ++
> > target/hexagon/internal.h | 3 ++
> > target/hexagon/mmvec/mmvec.h| 83
> +
> > target/hexagon/cpu.c| 72
> ++-
> > 6 files changed, 198 insertions(+), 3 deletions(-)
> > create mode 100644 target/hexagon/mmvec/mmvec.h
> >
> > diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> > 2855dd3..0b377c3 100644
> > --- a/target/hexagon/cpu.h
> > +++ b/target/hexagon/cpu.h
> > @@ -26,6 +26,7 @@ typedef struct CPUHexagonState CPUHexagonState;
> > #include "qemu-common.h"
> > #include "exec/cpu-defs.h"
> > #include "hex_regs.h"
> > +#include "mmvec/mmvec.h"
> >
> > #define NUM_PREGS 4
> > #define TOTAL_PER_THREAD_REGS 64
> > @@ -34,6 +35,7 @@ typedef struct CPUHexagonState CPUHexagonState;
> > #define STORES_MAX 2
> > #define REG_WRITES_MAX 32
> > #define PRED_WRITES_MAX 5 /* 4 insns + endloop */
> > +#define VSTORES_MAX 2
> >
> > #define TYPE_HEXAGON_CPU "hexagon-cpu"
> >
> > @@ -52,6 +54,13 @@ typedef struct {
> > uint64_t data64;
> > } MemLog;
> >
> > +typedef struct {
> > +target_ulong va;
> > +int size;
> > +MMVector mask QEMU_ALIGNED(16);
> > +MMVector data QEMU_ALIGNED(16);
> > +} VStoreLog;
>
> Do you really need a MMVector mask, or should this be a QRegMask?
You are correct. I'll change this.
>
> > -target_ulong gather_issued;
> > +bool gather_issued;
>
> Surely unrelated to adding state.
This was unintentionally included in the patch series for the scalar core.
Based on previous feedback, I know it should be a bool. However, this can
actually be removed altogether. So, in the next iteration of this series,
you'll see it removed.
>
> > +MMVector VRegs[NUM_VREGS] QEMU_ALIGNED(16);
> > +MMVector future_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
> > +MMVector tmp_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
>
> Ok, this is where I'm not keen on how you handle this for integer code, and
> for vector code has got to be past the realm of acceptable.
>
> You have exactly 4 slots in your vliw packet. You cannot possibly use 32
> future or tmp slots. For integers this wastage was at least small, but for
> vectors these waste just shy of 8k.
>
> All you need to do is to be smarter about mapping e.g. 5 to 8 temp slots
> during translation.
OK
>
> > +MMQReg QRegs[NUM_QREGS] QEMU_ALIGNED(16);
> > +MMQReg future_QRegs[NUM_QREGS] QEMU_ALIGNED(16);
>
> Likewise.
>
> > +/* Temporaries used within instructions */
> > +MMVector zero_vector QEMU_ALIGNED(16);
>
> You must be doing something wrong to need zero in memory.
The architecture specifies that if you use a .new in a packet where the vector
register isn't defined, it gets zero. So, the generator produces the
following for .new references
const intptr_t OsN_off =
test_bit(OsN_num, ctx->vregs_updated)
? offsetof(CPUHexagonState, future_VRegs[OsN_num])
: offsetof(CPUHexagonState, zero_vector);
>
> > +/*
> > + * The HVX register dump takes up a ton of space in the log
> > + * Don't print it unless it is needed */ #define DUMP_HVX 0 #if
> > +DUMP_HVX
> > +qemu_fprintf(f, "Vector Registers = {\n");
> > +for (int i = 0; i < NUM_VREGS; i++) {
> > +print_vreg(f, env, i);
> > +}
> > +for (int i = 0; i < NUM_QREGS; i++) {
> > +print_qreg(f, env, i);
> > +}
> > +qemu_fprintf(f, "}\n");
> > +#endif
>
> Use CPU_DUMP_FPU, controlled by -d fpu.
These aren't FP registers, but OK.
>
> > -cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS;
> > +cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS
> +
> > + NUM_QREGS;
>
> They're not really core regs though are they?
> Surely gdb_register_coprocessor is a better representation.
I'll look into this.
Thanks,
Taylor
RE: [PATCH 12/20] Hexagon HVX (target/hexagon) helper functions
> -Original Message- > From: Richard Henderson > Sent: Sunday, July 25, 2021 8:22 AM > To: Taylor Simpson ; qemu-devel@nongnu.org > Cc: phi...@redhat.com; a...@rev.ng; Brian Cain ; > peter.mayd...@linaro.org > Subject: Re: [PATCH 12/20] Hexagon HVX (target/hexagon) helper functions > > On 7/5/21 1:34 PM, Taylor Simpson wrote: > > +put_user_u8(env->vstore[i].data.ub[j], va + j); > > No put_user. Yes, there's also an include of qemu.h that I'll remove.
Re: [PATCH] kvm: ppc: Print meaningful message on KVM_CREATE_VM failure
On Thu, Jul 22, 2021 at 11:13:40AM -0300, Fabiano Rosas wrote:
> PowerPC has two KVM types (HV, PR) that translate into three kernel
> modules:
>
> kvm.ko - common kvm code
> kvm_hv.ko - kvm running with MSR_HV=1 or MSR_HV|PR=0 in a nested guest.
> kvm_pr.ko - kvm running in usermode MSR_PR=1.
>
> Since the two KVM types can both be running at the same time, this
> creates a situation in which it is possible for one or both of the
> modules to fail to initialize, leaving the generic one behind. This
> leads QEMU to think it can create a guest, but KVM will fail when
> calling the type-specific code:
>
> ioctl(KVM_CREATE_VM) failed: 22 Invalid argument
> qemu-kvm: failed to initialize KVM: Invalid argument
>
> Ideally this would be solved kernel-side, but it might be a while
> until we can get rid of one of the modules. So in the meantime this
> patch tries to make this less confusing for the end user by adding a
> more elucidative message:
>
> ioctl(KVM_CREATE_VM) failed: 22 Invalid argument
> PPC KVM module is not loaded. Try 'modprobe kvm_hv'.
>
> Signed-off-by: Fabiano Rosas
Applied to ppc-for-6.1, thanks.
> ---
> accel/kvm/kvm-all.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 3bd17135ce..4d9a7c7bfc 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2397,6 +2397,12 @@ static int kvm_init(MachineState *ms)
> "- for kernels supporting the vm.allocate_pgste sysctl, "
> "whether it is enabled\n");
> }
> +#elif TARGET_PPC
> +if (ret == -EINVAL) {
> +fprintf(stderr,
> +"PPC KVM module is not loaded. Try modprobe kvm_%s.\n",
> +(type == 2) ? "pr" : "hv");
> +}
> #endif
> goto err;
> }
--
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: aarch64 efi boot failures with qemu 6.0+
On 7/25/21 3:14 PM, Michael S. Tsirkin wrote:
On Sat, Jul 24, 2021 at 11:52:34AM -0700, Guenter Roeck wrote:
Hi all,
starting with qemu v6.0, some of my aarch64 efi boot tests no longer
work. Analysis shows that PCI devices with IO ports do not instantiate
in qemu v6.0 (or v6.1-rc0) when booting through efi. The problem affects
(at least) ne2k_pci, tulip, dc390, and am53c974. The problem only affects
aarch64, not x86/x86_64.
I bisected the problem to commit 0cf8882fd0 ("acpi/gpex: Inform os to
keep firmware resource map"). Since this commit, PCI device BAR
allocation has changed. Taking tulip as example, the kernel reports
the following PCI bar assignments when running qemu v5.2.
[3.921801] pci :00:01.0: [1011:0019] type 00 class 0x02
[3.922207] pci :00:01.0: reg 0x10: [io 0x-0x007f]
[3.922505] pci :00:01.0: reg 0x14: [mem 0x1000-0x107f]
[3.927111] pci :00:01.0: BAR 0: assigned [io 0x1000-0x107f]
[3.927455] pci :00:01.0: BAR 1: assigned [mem 0x1000-0x107f]
With qemu v6.0, the assignment is reported as follows.
[3.922887] pci :00:01.0: [1011:0019] type 00 class 0x02
[3.923278] pci :00:01.0: reg 0x10: [io 0x-0x007f]
[3.923451] pci :00:01.0: reg 0x14: [mem 0x1000-0x107f]
and the controller does not instantiate. The problem disapears after
reverting commit 0cf8882fd0.
Attached is a summary of test runs with various devices and qemu v5.2
as well as qemu v6.0, and the command line I use for efi boots.
Did commit 0cf8882fd0 introduce a bug, do I now need need some different
command line to instantiate PCI devices with io ports, or are such devices
simply no longer supported if the system is booted with efi support ?
Thanks,
Guenter
So that commit basically just says don't ignore what efi did.
The issue's thus likely efi.
I don't see the problem with efi boots on x86 and x86_64.
Any idea why that might be the case ?
Thanks,
Guenter
Cc the maintainer. Philippe can you comment pls?
---
Command line (tulip network interface):
CMDLINE="root=/dev/vda console=ttyAMA0"
ROOTFS="rootfs.ext2"
qemu-system-aarch64 -M virt -kernel arch/arm64/boot/Image -no-reboot \
-m 512 -cpu cortex-a57 -no-reboot \
-device tulip,netdev=net0 -netdev user,id=net0 \
-bios QEMU_EFI-aarch64.fd \
-snapshot \
-device virtio-blk-device,drive=d0 \
-drive file=${ROOTFS},if=none,id=d0,format=raw \
-nographic -serial stdio -monitor none \
--append "${CMDLINE}"
---
Boot tests with various devices known to work in qemu v5.2.
v5.2v6.0v6.0
efi non-efi efi
e1000 passpasspass
e1000-82544gc passpasspass
e1000-82545em passpasspass
e1000e passpasspass
i82550 passpasspass
i82557a passpasspass
i82557b passpasspass
i82557c passpasspass
i82558a passpasspass
i82559b passpasspass
i82559c passpasspass
i82559erpasspasspass
i82562 passpasspass
i82801 passpasspass
ne2k_pcipasspassfail<--
pcnet passpasspass
rtl8139 passpasspass
tulip passpassfail<--
usb-net passpasspass
virtio-net-device
passpasspass
virtio-net-pci passpasspass
virtio-net-pci-non-transitional
passpasspass
usb-xhcipasspasspass
usb-ehcipasspasspass
usb-ohcipasspasspass
usb-uas-xhcipasspasspass
virtio passpasspass
virtio-blk-pci passpasspass
virtio-blk-device
passpasspass
nvmepasspasspass
sdhci passpasspass
dc390 passpassfail<--
am53c974passpassfail<--
lsi53c895ai passpasspass
mptsas1068 passpasspass
lsi53c810 passpasspass
megasas passpasspass
megasas-gen2passpasspass
virtio-scsi-device
passpasspass
virtio-scsi-pci passpasspass
Re: aarch64 efi boot failures with qemu 6.0+
On Sat, Jul 24, 2021 at 11:52:34AM -0700, Guenter Roeck wrote:
> Hi all,
>
> starting with qemu v6.0, some of my aarch64 efi boot tests no longer
> work. Analysis shows that PCI devices with IO ports do not instantiate
> in qemu v6.0 (or v6.1-rc0) when booting through efi. The problem affects
> (at least) ne2k_pci, tulip, dc390, and am53c974. The problem only affects
> aarch64, not x86/x86_64.
>
> I bisected the problem to commit 0cf8882fd0 ("acpi/gpex: Inform os to
> keep firmware resource map"). Since this commit, PCI device BAR
> allocation has changed. Taking tulip as example, the kernel reports
> the following PCI bar assignments when running qemu v5.2.
>
> [3.921801] pci :00:01.0: [1011:0019] type 00 class 0x02
> [3.922207] pci :00:01.0: reg 0x10: [io 0x-0x007f]
> [3.922505] pci :00:01.0: reg 0x14: [mem 0x1000-0x107f]
> [3.927111] pci :00:01.0: BAR 0: assigned [io 0x1000-0x107f]
> [3.927455] pci :00:01.0: BAR 1: assigned [mem 0x1000-0x107f]
>
> With qemu v6.0, the assignment is reported as follows.
>
> [3.922887] pci :00:01.0: [1011:0019] type 00 class 0x02
> [3.923278] pci :00:01.0: reg 0x10: [io 0x-0x007f]
> [3.923451] pci :00:01.0: reg 0x14: [mem 0x1000-0x107f]
>
> and the controller does not instantiate. The problem disapears after
> reverting commit 0cf8882fd0.
>
> Attached is a summary of test runs with various devices and qemu v5.2
> as well as qemu v6.0, and the command line I use for efi boots.
>
> Did commit 0cf8882fd0 introduce a bug, do I now need need some different
> command line to instantiate PCI devices with io ports, or are such devices
> simply no longer supported if the system is booted with efi support ?
>
> Thanks,
> Guenter
So that commit basically just says don't ignore what efi did.
The issue's thus likely efi.
Cc the maintainer. Philippe can you comment pls?
> ---
> Command line (tulip network interface):
>
> CMDLINE="root=/dev/vda console=ttyAMA0"
> ROOTFS="rootfs.ext2"
>
> qemu-system-aarch64 -M virt -kernel arch/arm64/boot/Image -no-reboot \
> -m 512 -cpu cortex-a57 -no-reboot \
> -device tulip,netdev=net0 -netdev user,id=net0 \
> -bios QEMU_EFI-aarch64.fd \
> -snapshot \
> -device virtio-blk-device,drive=d0 \
> -drive file=${ROOTFS},if=none,id=d0,format=raw \
> -nographic -serial stdio -monitor none \
> --append "${CMDLINE}"
>
> ---
> Boot tests with various devices known to work in qemu v5.2.
>
> v5.2v6.0v6.0
> efi non-efi efi
> e1000 passpasspass
> e1000-82544gc passpasspass
> e1000-82545em passpasspass
> e1000epasspasspass
> i82550passpasspass
> i82557a passpasspass
> i82557b passpasspass
> i82557c passpasspass
> i82558a passpasspass
> i82559b passpasspass
> i82559c passpasspass
> i82559er passpasspass
> i82562passpasspass
> i82801passpasspass
> ne2k_pci passpassfail<--
> pcnet passpasspass
> rtl8139 passpasspass
> tulip passpassfail<--
> usb-net passpasspass
> virtio-net-device
> passpasspass
> virtio-net-pcipasspasspass
> virtio-net-pci-non-transitional
> passpasspass
>
> usb-xhci passpasspass
> usb-ehci passpasspass
> usb-ohci passpasspass
> usb-uas-xhci passpasspass
> virtiopasspasspass
> virtio-blk-pcipasspasspass
> virtio-blk-device
> passpasspass
> nvme passpasspass
> sdhci passpasspass
> dc390 passpassfail<--
> am53c974 passpassfail<--
> lsi53c895ai passpasspass
> mptsas1068passpasspass
> lsi53c810 passpasspass
> megasas passpasspass
> megasas-gen2 passpasspass
> virtio-scsi-device
> passpasspass
> virtio-scsi-pci passpasspass
[PULL v2 2/2] target/hexagon: Drop include of qemu.h
From: Peter Maydell The qemu.h file is a CONFIG_USER_ONLY header; it doesn't appear on the include path for softmmu builds. Currently we include it unconditionally in target/hexagon/op_helper.c. We used to need it for the put_user_*() and get_user_*() functions, but now that we have removed the uses of those from op_helper.c, the only reason it's still there is that we're implicitly relying on it pulling in some other headers. Explicitly include the headers we need for other functions, and drop the include of qemu.h. Signed-off-by: Peter Maydell Message-Id: <20210717103017.20491-1-peter.mayd...@linaro.org> Reviewed-by: Alex Bennée Reviewed-by: Richard Henderson Reviewed-by: Taylor Simpson Signed-off-by: Taylor Simpson --- target/hexagon/op_helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c index a959dba..61d5cde 100644 --- a/target/hexagon/op_helper.c +++ b/target/hexagon/op_helper.c @@ -16,7 +16,8 @@ */ #include "qemu/osdep.h" -#include "qemu.h" +#include "qemu/log.h" +#include "exec/exec-all.h" #include "exec/cpu_ldst.h" #include "exec/helper-proto.h" #include "fpu/softfloat.h" -- 2.7.4
[PULL v2 1/2] Hexagon (target/hexagon) remove put_user_*/get_user_*
Replace put_user_* with cpu_st*_data_ra
Replace get_user_* with cpu_ld*_data_ra
Suggested-by: Richard Henderson
Reviewed-by: Richard Henderson
Signed-off-by: Taylor Simpson
Message-Id: <1626384156-6248-2-git-send-email-tsimp...@quicinc.com>
---
target/hexagon/op_helper.c | 39 ++-
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 4595559..a959dba 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -17,6 +17,7 @@
#include "qemu/osdep.h"
#include "qemu.h"
+#include "exec/cpu_ldst.h"
#include "exec/helper-proto.h"
#include "fpu/softfloat.h"
#include "cpu.h"
@@ -140,22 +141,22 @@ void HELPER(debug_check_store_width)(CPUHexagonState
*env, int slot, int check)
void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
{
-switch (env->mem_log_stores[slot_num].width) {
+uintptr_t ra = GETPC();
+uint8_t width = env->mem_log_stores[slot_num].width;
+target_ulong va = env->mem_log_stores[slot_num].va;
+
+switch (width) {
case 1:
-put_user_u8(env->mem_log_stores[slot_num].data32,
-env->mem_log_stores[slot_num].va);
+cpu_stb_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
break;
case 2:
-put_user_u16(env->mem_log_stores[slot_num].data32,
- env->mem_log_stores[slot_num].va);
+cpu_stw_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
break;
case 4:
-put_user_u32(env->mem_log_stores[slot_num].data32,
- env->mem_log_stores[slot_num].va);
+cpu_stl_data_ra(env, va, env->mem_log_stores[slot_num].data32, ra);
break;
case 8:
-put_user_u64(env->mem_log_stores[slot_num].data64,
- env->mem_log_stores[slot_num].va);
+cpu_stq_data_ra(env, va, env->mem_log_stores[slot_num].data64, ra);
break;
default:
g_assert_not_reached();
@@ -393,37 +394,33 @@ static void check_noshuf(CPUHexagonState *env, uint32_t
slot)
static uint8_t mem_load1(CPUHexagonState *env, uint32_t slot,
target_ulong vaddr)
{
-uint8_t retval;
+uintptr_t ra = GETPC();
check_noshuf(env, slot);
-get_user_u8(retval, vaddr);
-return retval;
+return cpu_ldub_data_ra(env, vaddr, ra);
}
static uint16_t mem_load2(CPUHexagonState *env, uint32_t slot,
target_ulong vaddr)
{
-uint16_t retval;
+uintptr_t ra = GETPC();
check_noshuf(env, slot);
-get_user_u16(retval, vaddr);
-return retval;
+return cpu_lduw_data_ra(env, vaddr, ra);
}
static uint32_t mem_load4(CPUHexagonState *env, uint32_t slot,
target_ulong vaddr)
{
-uint32_t retval;
+uintptr_t ra = GETPC();
check_noshuf(env, slot);
-get_user_u32(retval, vaddr);
-return retval;
+return cpu_ldl_data_ra(env, vaddr, ra);
}
static uint64_t mem_load8(CPUHexagonState *env, uint32_t slot,
target_ulong vaddr)
{
-uint64_t retval;
+uintptr_t ra = GETPC();
check_noshuf(env, slot);
-get_user_u64(retval, vaddr);
-return retval;
+return cpu_ldq_data_ra(env, vaddr, ra);
}
/* Floating point */
--
2.7.4
[PULL v2 0/2] Hexagon (target/hexagon) remove put_user_*/get_user_*
The following changes since commit 7457b407edd6e8555e4b46488aab2f13959fccf8: Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-07-19' into staging (2021-07-19 11:34:08 +0100) are available in the git repository at: https://github.com/quic/qemu tags/pull-hex-20210725 for you to fetch changes up to 25fc9b79cd057e394f35d7afc18493becd515797: target/hexagon: Drop include of qemu.h (2021-07-21 15:54:02 -0500) The Hexagon target was silently failing the SIGSEGV test because the signal handler was not called. Patch 1/2 fixes the Hexagon target Patch 2/2 drops include qemu.h from target/hexagon/op_helper.c Changes in v2 Drop changes to linux-test.c due to intermittent failures on riscv Peter Maydell (1): target/hexagon: Drop include of qemu.h Taylor Simpson (1): Hexagon (target/hexagon) remove put_user_*/get_user_* target/hexagon/op_helper.c | 42 -- 1 file changed, 20 insertions(+), 22 deletions(-)
Re: [PATCH for-6.1? v2 7/9] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write
+Cédric/Benjamin
On 7/25/21 2:24 PM, Richard Henderson wrote:
> From clang-13:
> hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used \
> [-Werror,-Wunused-but-set-variable]
>
> It's pretty clear that we meant to write back 'v' after
> all that computation and not 'val'.
>
Fixes: 4f9924c4d4c ("ppc/pnv: Add models for POWER9 PHB4 PCIe Host bridge")
> Acked-by: David Gibson
> Signed-off-by: Richard Henderson
> ---
> hw/pci-host/pnv_phb4.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 54f57c660a..5c375a9f28 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t
> val)
> v &= 0xull;
> v |= 0xcfffull & val;
> }
> -*tptr = val;
> +*tptr = v;
> break;
> }
> case IODA3_TBL_MBT:
>
Re: [PATCH for-6.1?] bitops.h: revert db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation")
On 7/25/21 1:05 PM, Mark Cave-Ayland wrote:
> Commit db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation") introduced a
> bitrev8() function to reverse the bit ordering required for storing the MAC
> address in the q800 PROM.
>
> This function is not required since QEMU implements its own revbit8() function
> which does exactly the same thing. Remove the extraneous bitrev8() function
> and
> switch its only caller in hw/m68k/q800.c to use revbit8() instead.
>
> Signed-off-by: Mark Cave-Ayland
> ---
> hw/m68k/q800.c| 2 +-
> include/qemu/bitops.h | 22 --
> 2 files changed, 1 insertion(+), 23 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH for-6.1 6/6] hw/intc/armv7m_nvic: for v8.1M VECTPENDING hides S exceptions from NS
On 7/23/21 6:21 AM, Peter Maydell wrote: In Arm v8.1M the VECTPENDING field in the ICSR has new behaviour: if the register is accessed NonSecure and the highest priority pending enabled exception (that would be returned in the VECTPENDING field) targets Secure, then the VECTPENDING field must read 1 rather than the exception number of the pending exception. Implement this. Signed-off-by: Peter Maydell --- hw/intc/armv7m_nvic.c | 31 --- 1 file changed, 24 insertions(+), 7 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 5/6] hw/intc/armv7m_nvic: Correct size of ICSR.VECTPENDING
On 7/23/21 6:21 AM, Peter Maydell wrote: The VECTPENDING field in the ICSR is 9 bits wide, in bits [20:12] of the register. We were incorrectly masking it to 8 bits, so it would report the wrong value if the pending exception was greater than 256. Fix the bug. Signed-off-by: Peter Maydell --- hw/intc/armv7m_nvic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 4/6] hw/intc/armv7m_nvic: ISCR.ISRPENDING is set for non-enabled pending interrupts
On 7/23/21 6:21 AM, Peter Maydell wrote: The ISCR.ISRPENDING bit is set when an external interrupt is pending. This is true whether that external interrupt is enabled or not. This means that we can't use 's->vectpending == 0' as a shortcut to "ISRPENDING is zero", because s->vectpending indicates only the highest priority pending enabled interrupt. Remove the incorrect optimization so that if there is no pending enabled interrupt we fall through to scanning through the whole interrupt array. Signed-off-by: Peter Maydell --- hw/intc/armv7m_nvic.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 3/6] target/arm: Report M-profile alignment faults correctly to the guest
On 7/23/21 6:21 AM, Peter Maydell wrote: For M-profile, we weren't reporting alignment faults triggered by the generic TCG code correctly to the guest. These get passed into arm_v7m_cpu_do_interrupt() as an EXCP_DATA_ABORT with an A-profile style exception.fsr value of 1. We didn't check for this, and so they fell through into the default of "assume this is an MPU fault" and were reported to the guest as a data access violation MPU fault. Report these alignment faults as UsageFaults which set the UNALIGNED bit in the UFSR. Signed-off-by: Peter Maydell --- The other approach would be to have arm_cpu_do_unaligned_access() raise the EXCP_UNALIGNED which we already use for Unaligned UsageFaults which are raised by m-profile specific helper code, but I think this way is in line with the current design that generally prefers to report exception information in an A-profile format and then re-arrange that into the M-profile information in arm_v7m_cpu_do_interrupt(). --- target/arm/m_helper.c | 8 1 file changed, 8 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 2/6] target/arm: Add missing 'return's after calling v7m_exception_taken()
On 7/23/21 6:21 AM, Peter Maydell wrote: In do_v7m_exception_exit(), we perform various checks as part of performing the exception return. If one of these checks fails, the architecture requires that we take an appropriate exception on the existing stackframe. We implement this by calling v7m_exception_taken() to set up to take the new exception, and then immediately returning from do_v7m_exception_exit() without proceeding any further with the unstack-and-exception-return process. In a couple of checks that are new in v8.1M, we forgot the "return" statement, with the effect that if bad code in the guest tripped over these checks we would set up to take a UsageFault exception but then blunder on trying to also unstack and return from the original exception, with the probable result that the guest would crash. Add the missing return statements. Signed-off-by: Peter Maydell --- target/arm/m_helper.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 1/6] target/arm: Enforce that M-profile SP low 2 bits are always zero
On 7/23/21 6:21 AM, Peter Maydell wrote: For M-profile, unlike A-profile, the low 2 bits of SP are defined to be RES0H, which is to say that they must be hardwired to zero so that guest attempts to write non-zero values to them are ignored. Implement this behaviour by masking out the low bits: * for writes to r13 by the gdbstub * for writes to any of the various flavours of SP via MSR * for writes to r13 via store_reg() in generated code Note that all the direct uses of cpu_R[] in translate.c are in places where the register is definitely not r13 (usually because that has been checked for as an UNDEFINED or UNPREDICTABLE case and handled as UNDEF). All the other writes to regs[13] in C code are either: * A-profile only code * writes of values we can guarantee to be aligned, such as - writes of previous-SP-value plus or minus a 4-aligned constant - writes of the value in an SP limit register (which we already enforce to be aligned) Signed-off-by: Peter Maydell --- Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 0/2] accel/tcg: Fix hang when running in icount mode
On 7/25/21 7:44 AM, Peter Maydell wrote: This patchset fixes the intermittent hang seen when running a guest in icount mode, as reported in https://gitlab.com/qemu-project/qemu/-/issues/499 . The underlying cause of the hang is that code in cpu_loop_exec_tb() was using CF_COUNT_MASK as the maximum possible number of instructions it would try to execute from a TB when it set the icount_decr.u16.low field. This is wrong, because (a) that field can validly be set to any unsigned 16-bit integer and (b) now that CF_COUNT_MASK has been reduced to 511 in commit 78ff82bb1b67c0d7, it might be less than the number of insns in the TB. Patch one fixes cpu_loop_exec_tb() to use the actual maximum valid value for icount_decr.u16.low, which is 0x. Patch two adjusts the "should we ask for a TB with exactly this many insns in it?" condition so that instead of testing "cpu->icount_extra == 0", which should be always true if (insns_left > 0 && insns_left < tb->icount), we assert it instead. This assertion would have caught the bug fixed in patch one. Tested using the same iterating loop test described in the bug report; without the fix QEMU hangs within a handful of iterations. With the fix it managed 175 successful iterations before I got bored and hit ^C. thanks -- PMM Peter Maydell (2): accel/tcg: Don't use CF_COUNT_MASK as the max value of icount_decr.u16.low accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb() Nice one. Reviewed-by: Richard Henderson r~
[PATCH for-6.1 2/2] accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb()
In cpu_loop_exec_tb(), we decide whether to look for a TB with
exactly insns_left instructions in it using the condition
(!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount)
The check for icount_extra == 0 is unnecessary, because we just set
insns_left = MIN(0x, cpu->icount_budget);
icount_extra = icount_budget - insns_left;
and so icount_extra can only be non-zero if icount_budget > 0x
and insns_left == 0x. But in that case insns_left >= tb->icount
because 0x is much larger than TCG_MAX_INSNS, so the condition
will be false anyway.
Remove the unnecessary check, and instead assert:
* that we are only going to execute a partial TB here if the
icount budget has run out (ie icount_extra == 0)
* that the number of insns we're going to execute does fit into
the CF_COUNT_MASK
Signed-off-by: Peter Maydell
---
You could argue that we don't need the asserts, if you like.
The first one would have caught the bug fixed in the previous
commit, though.
---
accel/tcg/cpu-exec.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 6e8dc291197..5aa42fbff35 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -843,7 +843,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu,
TranslationBlock *tb,
* execute we need to ensure we find/generate a TB with exactly
* insns_left instructions in it.
*/
-if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) {
+if (insns_left > 0 && insns_left < tb->icount) {
+assert(insns_left <= CF_COUNT_MASK);
+assert(cpu->icount_extra == 0);
cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
}
#endif
--
2.20.1
Re: [PATCH for-6.1 2/2] accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb()
On Sun, 25 Jul 2021 at 18:44, Peter Maydell wrote:
>
> In cpu_loop_exec_tb(), we decide whether to look for a TB with
> exactly insns_left instructions in it using the condition
> (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount)
>
> The check for icount_extra == 0 is unnecessary, because we just set
> insns_left = MIN(0x, cpu->icount_budget);
> icount_extra = icount_budget - insns_left;
> and so icount_extra can only be non-zero if icount_budget > 0x
> and insns_left == 0x. But in that case insns_left >= tb->icount
> because 0x is much larger than TCG_MAX_INSNS, so the condition
> will be false anyway.
>
> Remove the unnecessary check, and instead assert:
> * that we are only going to execute a partial TB here if the
>icount budget has run out (ie icount_extra == 0)
> * that the number of insns we're going to execute does fit into
>the CF_COUNT_MASK
>
> Signed-off-by: Peter Maydell
> ---
> You could argue that we don't need the asserts, if you like.
> The first one would have caught the bug fixed in the previous
> commit, though.
"first" in the bulleted list, "second" in the order I put them in the code...
> ---
> accel/tcg/cpu-exec.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 6e8dc291197..5aa42fbff35 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -843,7 +843,9 @@ static inline void cpu_loop_exec_tb(CPUState *cpu,
> TranslationBlock *tb,
> * execute we need to ensure we find/generate a TB with exactly
> * insns_left instructions in it.
> */
> -if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) {
> +if (insns_left > 0 && insns_left < tb->icount) {
> +assert(insns_left <= CF_COUNT_MASK);
> +assert(cpu->icount_extra == 0);
> cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
> }
> #endif
-- PMM
[PATCH for-6.1 0/2] accel/tcg: Fix hang when running in icount mode
This patchset fixes the intermittent hang seen when running a guest in icount mode, as reported in https://gitlab.com/qemu-project/qemu/-/issues/499 . The underlying cause of the hang is that code in cpu_loop_exec_tb() was using CF_COUNT_MASK as the maximum possible number of instructions it would try to execute from a TB when it set the icount_decr.u16.low field. This is wrong, because (a) that field can validly be set to any unsigned 16-bit integer and (b) now that CF_COUNT_MASK has been reduced to 511 in commit 78ff82bb1b67c0d7, it might be less than the number of insns in the TB. Patch one fixes cpu_loop_exec_tb() to use the actual maximum valid value for icount_decr.u16.low, which is 0x. Patch two adjusts the "should we ask for a TB with exactly this many insns in it?" condition so that instead of testing "cpu->icount_extra == 0", which should be always true if (insns_left > 0 && insns_left < tb->icount), we assert it instead. This assertion would have caught the bug fixed in patch one. Tested using the same iterating loop test described in the bug report; without the fix QEMU hangs within a handful of iterations. With the fix it managed 175 successful iterations before I got bored and hit ^C. thanks -- PMM Peter Maydell (2): accel/tcg: Don't use CF_COUNT_MASK as the max value of icount_decr.u16.low accel/tcg: Remove unnecessary check on icount_extra in cpu_loop_exec_tb() accel/tcg/cpu-exec.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.20.1
[PATCH for-6.1 1/2] accel/tcg: Don't use CF_COUNT_MASK as the max value of icount_decr.u16.low
In cpu_loop_exec_tb() we were bounding the number of insns we might try to execute in a TB using CF_COUNT_MASK. This is incorrect, because we can validly put up to 0x into icount_decr.u16.low. In particular, since commit 78ff82bb1b67c0d7 reduced CF_COUNT_MASK to 511 this meant that we would incorrectly only try to execute 511 instructions in a 512-instruction TB, which could result in QEMU hanging when in icount mode. Use the actual maximum value, which is 0x. (This brings this code in to line with the similar logic in icount_prepare_for_run() in tcg-accel-ops-icount.c.) Fixes: 78ff82bb1b67c0d7 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/499 Signed-off-by: Peter Maydell --- accel/tcg/cpu-exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index fc895cf51e4..6e8dc291197 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -834,7 +834,7 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, /* Ensure global icount has gone forward */ icount_update(cpu); /* Refill decrementer and continue execution. */ -insns_left = MIN(CF_COUNT_MASK, cpu->icount_budget); +insns_left = MIN(0x, cpu->icount_budget); cpu_neg(cpu)->icount_decr.u16.low = insns_left; cpu->icount_extra = cpu->icount_budget - insns_left; -- 2.20.1
[PATCH for 6.1 v2 1/1] ui/gtk: add a keyboard fifo to the VTE consoles
Since commit 8eb13bbbac ("ui/gtk: vte: fix sending multiple
characeters") it's very easy to lock up QEMU with the GTK ui.
If you configure a guest with a serial device and the guest
doesn't listen on this device, QEMU will lock up after
entering two characters in the serial console. That's because
current code uses a busy loop for the chardev write retries
and the busy loop doesn't terminate in this case.
To fix this problem add a fifo to the VTE consoles and use the
chr_accept_input() callback function to write the remaining
characters in the queue to the chardev.
The fifo has a size of 4096 bytes, so one can copy and paste
a fairly large URL or file path.
Fixes: 8eb13bbbac ("ui/gtk: vte: fix sending multiple characeters")
Signed-off-by: Volker Rümelin
---
include/ui/gtk.h | 4
ui/gtk.c | 42 +-
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 9516670ebc..80d6bbd9b5 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -25,6 +25,9 @@
#include "ui/egl-helpers.h"
#include "ui/egl-context.h"
#endif
+#ifdef CONFIG_VTE
+#include "qemu/fifo8.h"
+#endif
#define MAX_VCS 10
@@ -62,6 +65,7 @@ typedef struct VirtualVteConsole {
GtkWidget *scrollbar;
GtkWidget *terminal;
Chardev *chr;
+Fifo8 out_fifo;
bool echo;
} VirtualVteConsole;
#endif
diff --git a/ui/gtk.c b/ui/gtk.c
index 376b4d528d..6cbcceda12 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1652,6 +1652,25 @@ static void gd_vc_adjustment_changed(GtkAdjustment
*adjustment, void *opaque)
}
}
+static void gd_vc_send_chars(VirtualConsole *vc)
+{
+uint32_t len, avail;
+
+len = qemu_chr_be_can_write(vc->vte.chr);
+avail = fifo8_num_used(>vte.out_fifo);
+if (len > avail) {
+len = avail;
+}
+while (len > 0) {
+const uint8_t *buf;
+uint32_t size;
+
+buf = fifo8_pop_buf(>vte.out_fifo, len, );
+qemu_chr_be_write(vc->vte.chr, (uint8_t *)buf, size);
+len -= size;
+}
+}
+
static int gd_vc_chr_write(Chardev *chr, const uint8_t *buf, int len)
{
VCChardev *vcd = VC_CHARDEV(chr);
@@ -1661,6 +1680,14 @@ static int gd_vc_chr_write(Chardev *chr, const uint8_t
*buf, int len)
return len;
}
+static void gd_vc_chr_accept_input(Chardev *chr)
+{
+VCChardev *vcd = VC_CHARDEV(chr);
+VirtualConsole *vc = vcd->console;
+
+gd_vc_send_chars(vc);
+}
+
static void gd_vc_chr_set_echo(Chardev *chr, bool echo)
{
VCChardev *vcd = VC_CHARDEV(chr);
@@ -1700,6 +1727,7 @@ static void char_gd_vc_class_init(ObjectClass *oc, void
*data)
cc->parse = qemu_chr_parse_vc;
cc->open = gd_vc_open;
cc->chr_write = gd_vc_chr_write;
+cc->chr_accept_input = gd_vc_chr_accept_input;
cc->chr_set_echo = gd_vc_chr_set_echo;
}
@@ -1714,6 +1742,7 @@ static gboolean gd_vc_in(VteTerminal *terminal, gchar
*text, guint size,
gpointer user_data)
{
VirtualConsole *vc = user_data;
+uint32_t free;
if (vc->vte.echo) {
VteTerminal *term = VTE_TERMINAL(vc->vte.terminal);
@@ -1733,16 +1762,10 @@ static gboolean gd_vc_in(VteTerminal *terminal, gchar
*text, guint size,
}
}
-int remaining = size;
-uint8_t* p = (uint8_t *)text;
-while (remaining > 0) {
-int can_write = qemu_chr_be_can_write(vc->vte.chr);
-int written = MIN(remaining, can_write);
-qemu_chr_be_write(vc->vte.chr, p, written);
+free = fifo8_num_free(>vte.out_fifo);
+fifo8_push_all(>vte.out_fifo, (uint8_t *)text, MIN(free, size));
+gd_vc_send_chars(vc);
-remaining -= written;
-p += written;
-}
return TRUE;
}
@@ -1759,6 +1782,7 @@ static GSList *gd_vc_vte_init(GtkDisplayState *s,
VirtualConsole *vc,
vc->s = s;
vc->vte.echo = vcd->echo;
vc->vte.chr = chr;
+fifo8_create(>vte.out_fifo, 4096);
vcd->console = vc;
snprintf(buffer, sizeof(buffer), "vc%d", idx);
--
2.26.2
[PATCH v2 0/1] ui/gtk: prevent QEMU lock up
Since commit 8eb13bbbac ("ui/gtk: vte: fix sending multiple
characeters") it's very easy to lock up QEMU with the GTK ui.
If you configure a guest with a serial device and the guest
doesn't listen on this device, QEMU will lock up after
entering two characters in the serial console.
v2:
Gerd suggested to use the chr_accept_input() callback function
instead of a write retry timer and to drop patch 2/2.
Volker Rümelin (1):
ui/gtk: add a keyboard fifo to the VTE consoles
include/ui/gtk.h | 4
ui/gtk.c | 42 +-
2 files changed, 37 insertions(+), 9 deletions(-)
--
2.26.2
Re: [PATCH for-6.2 0/2] target/sparc: Drop use of gen_io_end()
On Sat, 24 Jul 2021 at 21:48, Richard Henderson
wrote:
>
> On 7/24/21 10:27 AM, Peter Maydell wrote:
> > On Sat, 24 Jul 2021 at 14:49, Peter Maydell
> > wrote:
> >> There is a slight difficulty here with testing this: icount
> >> doesn't seem to work for sparc Linux guests in master at the
> >> moment. For instance if you get the advent calendar image from
> >>https://www.qemu-advent-calendar.org/2018/download/day11.tar.xz
> >> it will boot without icount with a command line like
> >>qemu-system-sparc -display none -vga none -machine SS-20 -serial stdio
> >> -kernel /tmp/day11/zImage.elf
> >> But if you add '-icount auto' it will get as far as
> >> "bootconsole [earlyprom0] disabled" and then apparently hang.
> >> I'm not sure what's going on here :-(
> >> (I filed this as https://gitlab.com/qemu-project/qemu/-/issues/499)
> >
> > This turns out to be a recent regression, caused by commit 78ff82bb
> > ("accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS"). It's
> > an intermittent rather than a 100% reproducible hang.
>
> Ouch. Ok, I'll have a look.
I did a bit more messing around with a repro case under rr,
and I think I now see why we end up hanging, although I'm not
100% sure what best to do to fix it:
* We have a TB with 512 insns (tb->icount == 512)
* We want to execute 511 insns (icount_decr == 511)
* the code generated by gen_tb_start() does "subtract
this TB's instruction count from icount_decr.u16.low, and
if this is negative jump to the exitlabel". 511 - 512 == -1,
so we exit the TB with status TB_EXIT_REQUESTED and without
executing any guest insns
* in cpu_loop_exit_tb() we look at insns_left, which is still 511,
so we don't take the "early return because exit_request" path
* we calculate a new insns_left = MIN(CF_COUNT_MASK, cpu->icount_budget).
icount_budget is 59267, so the new insns_left is still 511.
* we set icount_decr.u16.low to insns_left (ie same value as before)
* we set cpu->icount_extra to icount_budget - insns_left, which
is 58756
* because icount_extra is non-zero, we don't set cflags_next_tb
to force us to find an exactly 511 insn TB
* so we come out to the cpu_exec() main loop, and find again
the same 512 insn TB we started with.
* Nothing changed from the last time we tried to execute it so
we just go round and round in circles never making any progress...
We don't get this failure mode if CF_COUNT_MASK is larger than
TCG_MAX_INSNS, because the calculation of insns_left will
produce a larger number than TCG_MAX_INSNS, unless we really
are running out of icount budget (in which case icount_extra
should be 0 and we will force execution of that smaller TB).
So the primary bug here is that cpu_loop_exec_tb() needs updating
to follow the new logic of "allow insns_left = TCG_MAX_INSNS and
indicate that with 0 in the CF_COUNT_MASK field".
Q: in cpu_loop_exec_tb() in this calculation:
/*
* If the next tb has more instructions than we have left to
* execute we need to ensure we find/generate a TB with exactly
* insns_left instructions in it.
*/
if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) {
cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
}
why are we testing "!cpu->icount_extra" ? The thing the comment
says we're looking for ("this TB has more insns than we have
left to execute") would be just "insns_left > 0 && insns_left < tb->icount".
And the code generated in gen_tb_start() will exit without doing anything
if insns_left < tb->icount (as described above), so we'll get into
an infinite loop pretty much any time we decide not to force execution
of a smaller TB. It's merely that we're much more likely to do so
with CF_COUNT_MASK==511, because we are accidentally very often trying
to execute 511 insns of a 512 insn TB when we could execute all 512.
thanks
-- PMM
Re: [PATCH 12/20] Hexagon HVX (target/hexagon) helper functions
On 7/5/21 1:34 PM, Taylor Simpson wrote: +put_user_u8(env->vstore[i].data.ub[j], va + j); No put_user. r~
Re: [PATCH 11/20] Hexagon HVX (target/hexagon) instruction utility functions
On 7/5/21 1:34 PM, Taylor Simpson wrote:
Functions to support scatter/gather
...
+uint32_t count_leading_ones_2(uint16_t src)
+{
+int ret;
+for (ret = 0; src & 0x8000; src <<= 1) {
+ret++;
+}
+return ret;
+}
Not related to scatter/gather.
Also, much better implemented as
clz32(~src & 0x) - 16
r~
Re: [PATCH 10/20] Hexagon HVX (target/hexagon) C preprocessor for decode tree
On 7/5/21 1:34 PM, Taylor Simpson wrote:
+char *name = (char *)opcode_names[opcode];
+if (strncmp(name, test, strlen(test)) == 0) {
Why did you cast away const here?
r~
Re: [PATCH 06/20] Hexagon HVX (target/hexagon) macros
On 7/5/21 1:34 PM, Taylor Simpson wrote:
+static inline MMVector mmvec_vtmp_data(CPUHexagonState *env)
+{
+VRegMask vsel = env->VRegs_updated_tmp;
+MMVector ret;
+int idx = clo32(~revbit32(vsel));
+if (vsel == 0) {
+printf("[UNDEFINED] no .tmp load when implicitly required...");
+}
No random debugging printfs please.
r~
Re: [PATCH 03/20] Hexagon HVX (target/hexagon) register names
On 7/5/21 1:34 PM, Taylor Simpson wrote:
Signed-off-by: Taylor Simpson
---
target/hexagon/hex_regs.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/hexagon/hex_regs.h b/target/hexagon/hex_regs.h
index f291911..e1b3149 100644
--- a/target/hexagon/hex_regs.h
+++ b/target/hexagon/hex_regs.h
@@ -76,6 +76,7 @@ enum {
/* Use reserved control registers for qemu execution counts */
HEX_REG_QEMU_PKT_CNT = 52,
HEX_REG_QEMU_INSN_CNT = 53,
+HEX_REG_QEMU_HVX_CNT = 54,
HEX_REG_UTIMERLO = 62,
HEX_REG_UTIMERHI = 63,
};
Maybe move the hunk in patch 2 adjusting hexagon_regnames to here?
Anyway,
Reviewed-by: Richard Henderson
r~
Re: [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon Vector eXtensions (HVX) to core
On 7/5/21 1:34 PM, Taylor Simpson wrote:
HVX is a set of wide vector instructions. Machine state includes
vector registers (VRegs)
vector predicate registers (QRegs)
temporary registers for intermediate values
store buffer (masked stores and scatter/gather)
Signed-off-by: Taylor Simpson
---
target/hexagon/cpu.h| 35 -
target/hexagon/hex_arch_types.h | 5 +++
target/hexagon/insn.h | 3 ++
target/hexagon/internal.h | 3 ++
target/hexagon/mmvec/mmvec.h| 83 +
target/hexagon/cpu.c| 72 ++-
6 files changed, 198 insertions(+), 3 deletions(-)
create mode 100644 target/hexagon/mmvec/mmvec.h
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 2855dd3..0b377c3 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -26,6 +26,7 @@ typedef struct CPUHexagonState CPUHexagonState;
#include "qemu-common.h"
#include "exec/cpu-defs.h"
#include "hex_regs.h"
+#include "mmvec/mmvec.h"
#define NUM_PREGS 4
#define TOTAL_PER_THREAD_REGS 64
@@ -34,6 +35,7 @@ typedef struct CPUHexagonState CPUHexagonState;
#define STORES_MAX 2
#define REG_WRITES_MAX 32
#define PRED_WRITES_MAX 5 /* 4 insns + endloop */
+#define VSTORES_MAX 2
#define TYPE_HEXAGON_CPU "hexagon-cpu"
@@ -52,6 +54,13 @@ typedef struct {
uint64_t data64;
} MemLog;
+typedef struct {
+target_ulong va;
+int size;
+MMVector mask QEMU_ALIGNED(16);
+MMVector data QEMU_ALIGNED(16);
+} VStoreLog;
Do you really need a MMVector mask, or should this be a QRegMask?
-target_ulong gather_issued;
+bool gather_issued;
Surely unrelated to adding state.
+MMVector VRegs[NUM_VREGS] QEMU_ALIGNED(16);
+MMVector future_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
+MMVector tmp_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
Ok, this is where I'm not keen on how you handle this for integer code, and for vector
code has got to be past the realm of acceptable.
You have exactly 4 slots in your vliw packet. You cannot possibly use 32 future or tmp
slots. For integers this wastage was at least small, but for vectors these waste just shy
of 8k.
All you need to do is to be smarter about mapping e.g. 5 to 8 temp slots during
translation.
+MMQReg QRegs[NUM_QREGS] QEMU_ALIGNED(16);
+MMQReg future_QRegs[NUM_QREGS] QEMU_ALIGNED(16);
Likewise.
+/* Temporaries used within instructions */
+MMVector zero_vector QEMU_ALIGNED(16);
You must be doing something wrong to need zero in memory.
+/*
+ * The HVX register dump takes up a ton of space in the log
+ * Don't print it unless it is needed
+ */
+#define DUMP_HVX 0
+#if DUMP_HVX
+qemu_fprintf(f, "Vector Registers = {\n");
+for (int i = 0; i < NUM_VREGS; i++) {
+print_vreg(f, env, i);
+}
+for (int i = 0; i < NUM_QREGS; i++) {
+print_qreg(f, env, i);
+}
+qemu_fprintf(f, "}\n");
+#endif
Use CPU_DUMP_FPU, controlled by -d fpu.
-cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS;
+cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS + NUM_QREGS;
They're not really core regs though are they?
Surely gdb_register_coprocessor is a better representation.
r~
[PATCH for-6.1? v2 7/9] hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write
>From clang-13: hw/pci-host/pnv_phb4.c:375:18: error: variable 'v' set but not used \ [-Werror,-Wunused-but-set-variable] It's pretty clear that we meant to write back 'v' after all that computation and not 'val'. Acked-by: David Gibson Signed-off-by: Richard Henderson --- hw/pci-host/pnv_phb4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index 54f57c660a..5c375a9f28 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -392,7 +392,7 @@ static void pnv_phb4_ioda_write(PnvPHB4 *phb, uint64_t val) v &= 0xull; v |= 0xcfffull & val; } -*tptr = val; +*tptr = v; break; } case IODA3_TBL_MBT: -- 2.25.1
[PATCH for-6.1? v2 9/9] tests/unit: Remove unused variable from test_io
>From clang-13:
tests/unit/test-iov.c:161:26: error: variable 't' set but not used \
[-Werror,-Wunused-but-set-variable]
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
tests/unit/test-iov.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/unit/test-iov.c b/tests/unit/test-iov.c
index 9c415e2f1f..5371066fb6 100644
--- a/tests/unit/test-iov.c
+++ b/tests/unit/test-iov.c
@@ -158,7 +158,7 @@ static void test_io(void)
int sv[2];
int r;
-unsigned i, j, k, s, t;
+unsigned i, j, k, s;
fd_set fds;
unsigned niov;
struct iovec *iov, *siov;
@@ -182,7 +182,6 @@ static void test_io(void)
FD_ZERO();
-t = 0;
if (fork() == 0) {
/* writer */
@@ -201,7 +200,6 @@ static void test_io(void)
g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0);
if (r >= 0) {
k += r;
- t += r;
usleep(g_test_rand_int_range(0, 30));
} else if (errno == EAGAIN) {
select(sv[1]+1, NULL, , NULL, NULL);
@@ -238,7 +236,6 @@ static void test_io(void)
g_assert(memcmp(iov, siov, sizeof(*iov)*niov) == 0);
if (r > 0) {
k += r;
- t += r;
} else if (!r) {
if (s) {
break;
--
2.25.1
[PATCH for-6.1? v2 8/9] linux-user/syscall: Remove unused variable from execve
>From clang-13: linux-user/syscall.c:8503:17: error: variable 'total_size' set but not used \ [-Werror,-Wunused-but-set-variable] Acked-by: Laurent Vivier Signed-off-by: Richard Henderson --- linux-user/syscall.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 376629c689..ccd3892b2d 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -8364,7 +8364,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, abi_ulong guest_envp; abi_ulong addr; char **q; -int total_size = 0; argc = 0; guest_argp = arg2; @@ -8396,7 +8395,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, break; if (!(*q = lock_user_string(addr))) goto execve_efault; -total_size += strlen(*q) + 1; } *q = NULL; @@ -8408,7 +8406,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, break; if (!(*q = lock_user_string(addr))) goto execve_efault; -total_size += strlen(*q) + 1; } *q = NULL; -- 2.25.1
[PATCH for-6.1? v2 4/9] net/checksum: Remove unused variable in net_checksum_add_iov
>From clang-13:
../qemu/net/checksum.c:189:23: error: variable 'buf_off' set but not used \
[-Werror,-Wunused-but-set-variable]
Signed-off-by: Richard Henderson
---
net/checksum.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/checksum.c b/net/checksum.c
index 70f4eaeb3a..68245fd748 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -186,12 +186,11 @@ uint32_t
net_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt,
uint32_t iov_off, uint32_t size, uint32_t csum_offset)
{
-size_t iovec_off, buf_off;
+size_t iovec_off;
unsigned int i;
uint32_t res = 0;
iovec_off = 0;
-buf_off = 0;
for (i = 0; i < iov_cnt && size; i++) {
if (iov_off < (iovec_off + iov[i].iov_len)) {
size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size);
@@ -200,7 +199,6 @@ net_checksum_add_iov(const struct iovec *iov, const
unsigned int iov_cnt,
res += net_checksum_add_cont(len, chunk_buf, csum_offset);
csum_offset += len;
-buf_off += len;
iov_off += len;
size -= len;
}
--
2.25.1
[PATCH for-6.1? v2 5/9] hw/audio/adlib: Remove unused variable in adlib_callback
>From clang-13:
hw/audio/adlib.c:189:18: error: variable 'net' set but not used \
[-Werror,-Wunused-but-set-variable]
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
hw/audio/adlib.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 42d50d2fdc..5f979b1487 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -186,7 +186,7 @@ static int write_audio (AdlibState *s, int samples)
static void adlib_callback (void *opaque, int free)
{
AdlibState *s = opaque;
-int samples, net = 0, to_play, written;
+int samples, to_play, written;
samples = free >> SHIFT;
if (!(s->active && s->enabled) || !samples) {
@@ -219,7 +219,6 @@ static void adlib_callback (void *opaque, int free)
written = write_audio (s, samples);
if (written) {
-net += written;
samples -= written;
s->pos = (s->pos + written) % s->samples;
}
--
2.25.1
[PATCH for-6.1? v2 6/9] hw/ppc/spapr_events: Remove unused variable from check_exception
>From clang-13:
hw/ppc/spapr_events.c:937:14: error: variable 'xinfo' set but not used \
[-Werror,-Wunused-but-set-variable]
Acked-by: David Gibson
Signed-off-by: Richard Henderson
---
hw/ppc/spapr_events.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 0cfc19be19..23e2e2fff1 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -934,7 +934,6 @@ static void check_exception(PowerPCCPU *cpu,
SpaprMachineState *spapr,
uint32_t nret, target_ulong rets)
{
uint32_t mask, buf, len, event_len;
-uint64_t xinfo;
SpaprEventLogEntry *event;
struct rtas_error_log header;
int i;
@@ -944,13 +943,9 @@ static void check_exception(PowerPCCPU *cpu,
SpaprMachineState *spapr,
return;
}
-xinfo = rtas_ld(args, 1);
mask = rtas_ld(args, 2);
buf = rtas_ld(args, 4);
len = rtas_ld(args, 5);
-if (nargs == 7) {
-xinfo |= (uint64_t)rtas_ld(args, 6) << 32;
-}
event = rtas_event_log_dequeue(spapr, mask);
if (!event) {
--
2.25.1
[PATCH for-6.1? v2 3/9] util/selfmap: Discard mapping on error
>From clang-13:
util/selfmap.c:26:21: error: variable 'errors' set but not used \
[-Werror,-Wunused-but-set-variable]
Quite right of course, but there's no reason not to check errors.
First, incrementing errors is incorrect, because qemu_strtoul
returns an errno not a count -- just or them together so that
we have a non-zero value at the end.
Second, if we have an error, do not add the struct to the list,
but free it instead.
Cc: Alex Bennée
Signed-off-by: Richard Henderson
---
util/selfmap.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/util/selfmap.c b/util/selfmap.c
index 2ec99dfdda..2c14f019ce 100644
--- a/util/selfmap.c
+++ b/util/selfmap.c
@@ -23,29 +23,34 @@ GSList *read_self_maps(void)
gchar **fields = g_strsplit(lines[i], " ", 6);
if (g_strv_length(fields) > 4) {
MapInfo *e = g_new0(MapInfo, 1);
-int errors;
+int errors = 0;
const char *end;
-errors = qemu_strtoul(fields[0], , 16, >start);
-errors += qemu_strtoul(end + 1, NULL, 16, >end);
+errors |= qemu_strtoul(fields[0], , 16, >start);
+errors |= qemu_strtoul(end + 1, NULL, 16, >end);
e->is_read = fields[1][0] == 'r';
e->is_write = fields[1][1] == 'w';
e->is_exec = fields[1][2] == 'x';
e->is_priv = fields[1][3] == 'p';
-errors += qemu_strtoul(fields[2], NULL, 16, >offset);
+errors |= qemu_strtoul(fields[2], NULL, 16, >offset);
e->dev = g_strdup(fields[3]);
-errors += qemu_strtou64(fields[4], NULL, 10, >inode);
+errors |= qemu_strtou64(fields[4], NULL, 10, >inode);
-/*
- * The last field may have leading spaces which we
- * need to strip.
- */
-if (g_strv_length(fields) == 6) {
-e->path = g_strdup(g_strchug(fields[5]));
+if (!errors) {
+/*
+ * The last field may have leading spaces which we
+ * need to strip.
+ */
+if (g_strv_length(fields) == 6) {
+e->path = g_strdup(g_strchug(fields[5]));
+}
+map_info = g_slist_prepend(map_info, e);
+} else {
+g_free(e->dev);
+g_free(e);
}
-map_info = g_slist_prepend(map_info, e);
}
g_strfreev(fields);
--
2.25.1
[PATCH for-6.1? v2 2/9] accel/tcg: Remove unused variable in cpu_exec
>From clang-13:
accel/tcg/cpu-exec.c:783:15: error: variable 'cc' set but not used \
[-Werror,-Wunused-but-set-variable]
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
accel/tcg/cpu-exec.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index fc895cf51e..1c6f684cb0 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -853,7 +853,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu,
TranslationBlock *tb,
int cpu_exec(CPUState *cpu)
{
-CPUClass *cc = CPU_GET_CLASS(cpu);
int ret;
SyncClocks sc = { 0 };
@@ -887,19 +886,14 @@ int cpu_exec(CPUState *cpu)
* that we support, but is still unfixed in clang:
* https://bugs.llvm.org/show_bug.cgi?id=21183
*
- * Reload essential local variables here for those compilers.
+ * Reload an essential local variable here for those compilers.
* Newer versions of gcc would complain about this code (-Wclobbered),
* so we only perform the workaround for clang.
*/
cpu = current_cpu;
-cc = CPU_GET_CLASS(cpu);
#else
-/*
- * Non-buggy compilers preserve these locals; assert that
- * they have the correct value.
- */
+/* Non-buggy compilers preserve this; assert the correct value. */
g_assert(cpu == current_cpu);
-g_assert(cc == CPU_GET_CLASS(cpu));
#endif
#ifndef CONFIG_SOFTMMU
--
2.25.1
[PATCH for-6.1? v2 1/9] nbd/server: Mark variable unused in nbd_negotiate_meta_queries
>From clang-13:
nbd/server.c:976:22: error: variable 'bitmaps' set but not used \
[-Werror,-Wunused-but-set-variable]
which is incorrect; see //bugs.llvm.org/show_bug.cgi?id=3888.
Cc: qemu-bl...@nongnu.org
Cc: Eric Blake
Cc: Vladimir Sementsov-Ogievskiy
Signed-off-by: Richard Henderson
---
nbd/server.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/nbd/server.c b/nbd/server.c
index b60ebc3ab6..3927f7789d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -973,7 +973,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
{
int ret;
g_autofree char *export_name = NULL;
-g_autofree bool *bitmaps = NULL;
+/* Mark unused to work around https://bugs.llvm.org/show_bug.cgi?id=3888 */
+g_autofree G_GNUC_UNUSED bool *bitmaps = NULL;
NBDExportMetaContexts local_meta = {0};
uint32_t nb_queries;
size_t i;
--
2.25.1
[PATCH for-6.1? v2 0/9] Fixes for clang-13
These are all "variable set but not used" Werrors when building with clang master. Patch 1 is clearly a clang bug, not considering the side-effects of g_autofree, but the rest are legitimate. r~ Richard Henderson (9): nbd/server: Mark variable unused in nbd_negotiate_meta_queries accel/tcg: Remove unused variable in cpu_exec util/selfmap: Discard mapping on error net/checksum: Remove unused variable in net_checksum_add_iov hw/audio/adlib: Remove unused variable in adlib_callback hw/ppc/spapr_events: Remove unused variable from check_exception hw/pci-hist/pnv_phb4: Fix typo in pnv_phb4_ioda_write linux-user/syscall: Remove unused variable from execve tests/unit: Remove unused variable from test_io accel/tcg/cpu-exec.c | 10 ++ hw/audio/adlib.c | 3 +-- hw/pci-host/pnv_phb4.c | 2 +- hw/ppc/spapr_events.c | 5 - linux-user/syscall.c | 3 --- nbd/server.c | 3 ++- net/checksum.c | 4 +--- tests/unit/test-iov.c | 5 + util/selfmap.c | 29 + 9 files changed, 25 insertions(+), 39 deletions(-) -- 2.25.1
Re: [PATCH for-6.1?] bitops.h: revert db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation")
On 7/25/21 1:05 AM, Mark Cave-Ayland wrote:
Commit db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation") introduced a
bitrev8() function to reverse the bit ordering required for storing the MAC
address in the q800 PROM.
This function is not required since QEMU implements its own revbit8() function
which does exactly the same thing. Remove the extraneous bitrev8() function and
switch its only caller in hw/m68k/q800.c to use revbit8() instead.
Signed-off-by: Mark Cave-Ayland
---
hw/m68k/q800.c| 2 +-
include/qemu/bitops.h | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
---
I picked this up reading the loongarch thread where I realised that QEMU
already has a revbit8() function - I was searching for bitrev8() before
deciding that this needed to be added since this was the name of the equivalent
function in Linux.
I think this is a good candidate for 6.1 still because a) it only has 1 caller
which is easy for me to test and b) it prevents anyone else coming along and
accidentally using it later.
MCA.
Reviewed-by: Richard Henderson
r~
[PATCH for-6.1?] bitops.h: revert db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation")
Commit db1ffc32dd ("qemu/bitops.h: add bitrev8 implementation") introduced a
bitrev8() function to reverse the bit ordering required for storing the MAC
address in the q800 PROM.
This function is not required since QEMU implements its own revbit8() function
which does exactly the same thing. Remove the extraneous bitrev8() function and
switch its only caller in hw/m68k/q800.c to use revbit8() instead.
Signed-off-by: Mark Cave-Ayland
---
hw/m68k/q800.c| 2 +-
include/qemu/bitops.h | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
---
I picked this up reading the loongarch thread where I realised that QEMU
already has a revbit8() function - I was searching for bitrev8() before
deciding that this needed to be added since this was the name of the equivalent
function in Linux.
I think this is a good candidate for 6.1 still because a) it only has 1 caller
which is easy for me to test and b) it prevents anyone else coming along and
accidentally using it later.
MCA.
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 6817c8b5d1..ac0a13060b 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -334,7 +334,7 @@ static void q800_init(MachineState *machine)
prom = memory_region_get_ram_ptr(dp8393x_prom);
checksum = 0;
for (i = 0; i < 6; i++) {
-prom[i] = bitrev8(nd_table[0].macaddr.a[i]);
+prom[i] = revbit8(nd_table[0].macaddr.a[i]);
checksum ^= prom[i];
}
prom[7] = 0xff - checksum;
diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 110c56e099..03213ce952 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -618,26 +618,4 @@ static inline uint64_t half_unshuffle64(uint64_t x)
return x;
}
-/**
- * bitrev8:
- * @x: 8-bit value to be reversed
- *
- * Given an input value with bits::
- *
- * ABCDEFGH
- *
- * return the value with its bits reversed from left to right::
- *
- * HGFEDCBA
- *
- * Returns: the bit-reversed value.
- */
-static inline uint8_t bitrev8(uint8_t x)
-{
-x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa);
-x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc);
-x = (x >> 4) | (x << 4) ;
-return x;
-}
-
#endif
--
2.20.1
[PATCH] target/i386: Added consistency checks for event injection
VMRUN exits with SVM_EXIT_ERR if either:
* The event injected has a reserved type.
* When the event injected is of type 3 (exception), and the vector that
has been specified does not correspond to an exception.
This does not fix the entire exc_inj test in kvm-unit-tests.
Signed-off-by: Lara Lazier
---
target/i386/tcg/sysemu/svm_helper.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/target/i386/tcg/sysemu/svm_helper.c
b/target/i386/tcg/sysemu/svm_helper.c
index a61aa23017..70d5c2e35d 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -395,6 +395,9 @@ void helper_vmrun(CPUX86State *env, int aflag, int
next_eip_addend)
cpu_loop_exit(cs);
break;
case SVM_EVTINJ_TYPE_EXEPT:
+if (vector == EXCP02_NMI || vector >= 31) {
+cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+}
cs->exception_index = vector;
env->error_code = event_inj_err;
env->exception_is_int = 0;
@@ -410,6 +413,9 @@ void helper_vmrun(CPUX86State *env, int aflag, int
next_eip_addend)
qemu_log_mask(CPU_LOG_TB_IN_ASM, "SOFT");
cpu_loop_exit(cs);
break;
+default:
+cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
+break;
}
qemu_log_mask(CPU_LOG_TB_IN_ASM, " %#x %#x\n", cs->exception_index,
env->error_code);
--
2.25.1
Re: [PATCH for-6.2 1/2] target/sparc: Drop use of gen_io_end()
On 24/07/2021 14:49, Peter Maydell wrote:
The gen_io_end() function is obsolete (as documented in
docs/devel/tcg-icount.rst). Where an instruction is an I/O
operation, the translator frontend should call gen_io_start()
before generating the code which does the I/O, and then
end the TB immediately after this insn.
Remove the calls to gen_io_end() in the SPARC frontend,
and ensure that the insns which were calling it end the
TB if they didn't do so already.
Signed-off-by: Peter Maydell
---
target/sparc/translate.c | 25 ++---
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 11de5a49631..bb70ba17deb 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3401,7 +3401,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
tcg_temp_free_i32(r_const);
gen_store_gpr(dc, rd, cpu_dst);
if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
+/* I/O operations in icount mode must end the TB */
+dc->base.is_jmp = DISAS_EXIT;
}
}
break;
@@ -3454,7 +3455,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
tcg_temp_free_i32(r_const);
gen_store_gpr(dc, rd, cpu_dst);
if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
+/* I/O operations in icount mode must end the TB */
+dc->base.is_jmp = DISAS_EXIT;
}
}
break;
@@ -3588,7 +3590,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
tcg_temp_free_ptr(r_tickptr);
tcg_temp_free_i32(r_const);
if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
+/* I/O operations in icount mode must end the TB */
+dc->base.is_jmp = DISAS_EXIT;
}
}
break;
@@ -4582,7 +4585,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
}
gen_helper_wrpstate(cpu_env, cpu_tmp0);
if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
+/* I/O ops in icount mode must end the TB
*/
+dc->base.is_jmp = DISAS_EXIT;
}
dc->npc = DYNAMIC_PC;
break;
@@ -4598,7 +4602,8 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
}
gen_helper_wrpil(cpu_env, cpu_tmp0);
if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
+/* I/O ops in icount mode must end the TB
*/
+dc->base.is_jmp = DISAS_EXIT;
}
break;
case 9: // cwp
@@ -4697,10 +4702,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
gen_helper_tick_set_limit(r_tickptr,
cpu_hstick_cmpr);
tcg_temp_free_ptr(r_tickptr);
-if (tb_cflags(dc->base.tb) &
- CF_USE_ICOUNT) {
-gen_io_end();
-}
/* End TB to handle timer interrupt */
dc->base.is_jmp = DISAS_EXIT;
}
@@ -5327,9 +5328,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
gen_io_start();
}
gen_helper_done(cpu_env);
-if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_end();
-}
goto jmp_insn;
case 1:
if (!supervisor(dc))
@@ -5340,9 +5338,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned
int insn)
gen_io_start();
Re: [PATCH v2] acpi: x86: pcihp: add support hotplug on multifunction bridges
Hi Igor,
On Fri, Jul 23, 2021 at 12:04 PM Igor Mammedov wrote:
> Commit [1] switched PCI hotplug from native to ACPI one by default.
>
> That however breaks hotplug on following CLI that used to work:
>-nodefaults -machine q35 \
>-device
> pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1
> \
>-device
> pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2
>
> where PCI device is hotplugged to pcie-root-port-1 with error on guest
> side:
>
> ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT],
> AE_NOT_FOUND (20201113/psargs-330)
> ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error
> (AE_NOT_FOUND) (20201113/psparse-531)
> ACPI Error: Aborting method \_GPE._E01 due to previous error
> (AE_NOT_FOUND) (20201113/psparse-531)
> ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01]
> (20201113/evgpe-515)
>
> cause is that QEMU's ACPI hotplug never supported functions other then 0
> and due to bug it was generating notification entries for not described
> functions.
>
> Technically there is no reason not to describe cold-plugged bridges
> (root ports) on functions other then 0, as they similarly to bridge
> on function 0 are unpluggable.
>
> So since we need to describe multifunction devices iterate over
> fuctions as well. But describe only cold-plugged bridges[root ports]
> on functions other than 0 as well.
>
> 1)
> Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 (hw/acpi/ich9: Set ACPI
> PCI hot-plug as default on Q35)
> Signed-off-by: Igor Mammedov
> Reported-by: Laurent Vivier
> ---
> v2:
> * squash 1/2 "acpi: x86: pcihp: cleanup devfn usage in
> build_append_pci_bus_devices()"
> into the main patch
> * drop Sxx -> Sxxx change as devfn fits into 2 digits anyway
> * cleanup PCI_FUN/DEVFN and use func/devfn local variables instead
> * fix typos
> ---
> hw/i386/acpi-build.c | 44 ++--
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..a33ac8b91e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> Aml *dev, *notify_method = NULL, *method;
> QObject *bsel;
> PCIBus *sec;
> -int i;
> +int devfn;
>
> bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> NULL);
> if (bsel) {
> @@ -384,23 +384,31 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
> }
>
> -for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> +for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> DeviceClass *dc;
> PCIDeviceClass *pc;
> -PCIDevice *pdev = bus->devices[i];
> -int slot = PCI_SLOT(i);
> +PCIDevice *pdev = bus->devices[devfn];
> +int slot = PCI_SLOT(devfn);
> +int func = PCI_FUNC(devfn);
> +/* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */
> +int adr = slot << 16 | func;
> bool hotplug_enabled_dev;
> bool bridge_in_acpi;
> bool cold_plugged_bridge;
>
> if (!pdev) {
> -if (bsel) { /* add hotplug slots for non present devices */
> +/*
> + * add hotplug slots for non present devices.
> + * hotplug is supported only for non-multifunction device
> + * so generate device description only for function 0
> + */
> +if (bsel && !func) {
> if (pci_bus_is_express(bus) && slot > 0) {
> break;
> }
> -dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> +dev = aml_device("S%.02X", devfn);
> aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(slot <<
> 16)));
> +aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
> method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> aml_append(method,
> aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
> @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> continue;
> }
>
> -/* start to compose PCI slot descriptor */
> -dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
> +/*
> + * allow describing coldplugged bridges in ACPI even if they are
> not
> + * on function 0, as they are not unpluggable, for all other
> devices
> + * generate description only for function 0 per slot
> + */
> +if (func && !bridge_in_acpi) {
> +continue;
> +
Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI
Hi Sebastian,
On 11:49 Fri 09 Jul , Sebastian Huber wrote:
> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
You're referring to GICv3 but actually modifying GICv2 model. Having a
look at GICv2 reference manual, your affirmation still hold though.
> connected PE with GICR_TYPER.Processor_Number < 8. For Qemu this is the case
> since GIC_NCPU == 8.
This is GICv3 specific. For GICv2 the architectural limit is 8 CPUs.
>
> For SPI, make the interrupt pending on all CPUs and not just the processor
> targets of the interrupt.
So you're not referring to GICD_ISPENDR0 anymore right? SPIs starts at
IRQ number 32. GICD_ISPENDR0 is for IRQs 0 to 31, which are SGIs and
PPIs (This is why this reg is banked, meaning that a CPU can only
trigger a PPI of its own). Maybe make it clear in your commit message
that you are now talking about GICD_ISPENDRn with n > 0
Moreover your statement regarding SPIs seems weird to me. Setting an
SPI pending (in GICD_ISPENDRn with n > 0) should really be like having
it being triggered from the IRQ line. It makes it pending in the
distributor. The distributor then forward it as normal. Why the
GICD_ITARGETSRn configuration should be ignored in this case? At least I
can't find any reference to such a behaviour in the reference manual.
>
> This behaviour is at least present on the i.MX7D which uses an
> Cortex-A7MPCore.
Which has a GICv2, not a v3 right?
>
> Signed-off-by: Sebastian Huber
> ---
> hw/intc/arm_gic.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a994b1f024..8e377bac59 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr
> offset,
>
> for (i = 0; i < 8; i++) {
> if (value & (1 << i)) {
> +int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
Indeed, I think the current implementation for PPIs is wrong
(GIC_DIST_TARGET(irq + i) is probably 0 in this case, so a set pending
request for a PPI will get incorrectly ignored). So I agree with the irq <
GIC_INTERNAL case. But for SPIs my concerns still hold (see my comment
in the commit message).
> if (s->security_extn && !attrs.secure &&
> !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
> continue; /* Ignore Non-secure access of Group0 IRQ */
> }
>
> -GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
> +GIC_DIST_SET_PENDING(irq + i, cm);
> }
> }
> } else if (offset < 0x300) {
> @@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr
> offset,
> continue; /* Ignore Non-secure access of Group0 IRQ */
> }
>
> -/* ??? This currently clears the pending bit for all CPUs, even
> - for per-CPU interrupts. It's unclear whether this is the
> - corect behavior. */
> if (value & (1 << i)) {
> -GIC_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> +int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +GIC_DIST_CLEAR_PENDING(irq + i, cm);
I agree with this change too, but you are modifying the GICD_ICPENDRn
register behaviour without mentioning it in the commit message.
Thanks,
--
Luc
Re: [PATCH] hw/acpi: some cosmetic improvements to existing code
ping ...
On Wed, 21 Jul 2021, Ani Sinha wrote:
> All existing code using acpi_get_i386_pci_host() checks for a non-null
> return from this function call. This change brings the same check to
> acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> why we unconditionally pass a truth value to the last argument when calling
> acpi_pcihp_reset() from ich9 platform.
>
> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>
> Signed-off-by: Ani Sinha
> ---
> hw/acpi/ich9.c | 1 +
> hw/acpi/pcihp.c | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 778e27b659..58d8430eb9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
> pm->smi_en_wmask = ~0;
>
> if (pm->use_acpi_hotplug_bridge) {
> +/* on root PCIE bus, we always use native or SHPC based hotplug */
> acpi_pcihp_reset(>acpi_pci_hotplug, true);
> }
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..856c6e1b47 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
> return;
> }
>
> +if (!host) {
> +root_hp_disabled = true;
> +return;
> +}
> +
> bus = PCI_HOST_BRIDGE(host)->bus;
> if (bus) {
> /* setting the hotplug handler to NULL makes the bus
> non-hotpluggable */
> --
> 2.25.1
>
>
Re: -only-migrate and the two different uses of migration blockers
On Thu, Jul 22, 2021 at 07:00:56PM +0100, Dr. David Alan Gilbert wrote: > * David Gibson (da...@gibson.dropbear.id.au) wrote: > > On Tue, Jul 20, 2021 at 07:30:16AM +0200, Markus Armbruster wrote: > > > "Dr. David Alan Gilbert" writes: > > > > > > > * Markus Armbruster (arm...@redhat.com) wrote: > > > >> We appear to use migration blockers in two ways: > > > >> > > > >> (1) Prevent migration for an indefinite time, typically due to use of > > > >> some feature that isn't compatible with migration. > > > >> > > > >> (2) Delay migration for a short time. > > > >> > > > >> Option -only-migrate is designed for (1). It interferes with (2). > > > >> > > > >> Example for (1): device "x-pci-proxy-dev" doesn't support migration. > > > >> It > > > >> adds a migration blocker on realize, and deletes it on unrealize. With > > > >> -only-migrate, device realize fails. Works as designed. > > > >> > > > >> Example for (2): spapr_mce_req_event() makes an effort to prevent > > > >> migration degrate the reporting of FWNMIs. It adds a migration blocker > > > >> when it receives one, and deletes it when it's done handling it. This > > > >> is a best effort; if migration is already in progress by the time FWNMI > > > >> is received, we simply carry on, and that's okay. However, option > > > >> -only-migrate sabotages the best effort entirely. > > > > > > > > That's interesting; it's the first time I've heard of anyone using it as > > > > 'best effort'. I've always regarded blockers as blocking. > > > > > > Me too, until I found this one. > > > > Right, it may well have been the first usage this way, this fwnmi > > stuff isn't super old. > > > > > >> While this isn't exactly terrible, it may be a weakness in our thinking > > > >> and our infrastructure. I'm bringing it up so the people in charge are > > > >> aware :) > > > > > > > > Thanks. > > > > > > > > It almost feels like they need a way to temporarily hold off > > > > 'completion' of migratio - i.e. the phase where we stop the CPU and > > > > write the device data; mind you you'd also probably want it to stop > > > > cold-migrates/snapshots? > > > > > > Yes, a proper way to delay 'completion' for a bit would be clearer, and > > > wouldn't let -only-migrate interfere. > > > > Right. If that becomes a thing, we should use it here. Note that > > this one use case probably isn't a very strong argument for it, > > though. The only problem here is slightly less that optimal error > > reporting in a rare edge case (hardware fault occurs by chance at the > > same time as a migration). > > Can you at least put a scary comment in to say why it's so odd. > > If you wanted a choice of a different bad way to do this, since you have > savevm_htab_handlers, you might be able to make htab_save_iterate claim > there's always more to do. That would only work if the hash MMU is in use, which won't be the case with most current systems. > > and, also, I half-suspect that the whole fwnmi feature exists > > more to tick IBM RAS check boxes than because anyone will actually use > > it. > > Ah at least it's always reliable > > Dave > > > -- 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
