A last small test of bug fixes before rc1.

The following changes since commit ed8ad9728a9c0eec34db9dff61dfa2f1dd625637:

Merge tag 'pull-tpm-2023-07-14-1' of https://github.com/stefanberger/qemu-tpm into staging (2023-07-15 14:54:04 +0100)

https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20230717

for you to fetch changes up to c2c1c4a35c7c2b1a4140b0942b9797c857e476a4:

hw/nvram: Avoid unnecessary Xilinx eFuse backstore write (2023-07-17 11:05:52 +0100)

* hw/arm/sbsa-ref: set 'slots' property of xhci

* linux-user: Remove pointless NULL check in clock_adjtime handling

* ptw: Fix S1_ptw_translate() debug path

* ptw: Account for FEAT_RME when applying {N}SW, SA bits

* accel/tcg: Zero-pad PC in TCG CPU exec trace lines

* hw/nvram: Avoid unnecessary Xilinx eFuse backstore write

linux-user: Remove pointless NULL check in clock_adjtime handling

target/arm/ptw.c: Add comments to S1Translate struct fields

target/arm: Fix S1_ptw_translate() debug path

target/arm/ptw.c: Account for FEAT_RME when applying {N}SW, SA bits

accel/tcg: Zero-pad PC in TCG CPU exec trace lines

Tong Ho (1):

hw/nvram: Avoid unnecessary Xilinx eFuse backstore write

In the code for TARGET_NR_clock_adjtime, we set the pointer phtx to

the address of the local variable htx. This means it can never be

NULL, but later in the code we check it for NULL anyway. Coverity

complains about this (CID 1507683) because the NULL check comes after

a call to clock_adjtime() that assumes it is non-NULL.

Since phtx is always &htx, and is used only in three places, it's not

really necessary. Remove it, bringing the code structure in to line

with that for TARGET_NR_clock_adjtime64, which already uses a simple

'&htx' when it wants a pointer to 'htx'.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Message-id: 20230623144410.1837261-1-peter.maydell@linaro.org

linux-user/syscall.c | 12 +++++-------

1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c

--- a/linux-user/syscall.c

+++ b/linux-user/syscall.c

@@ -XXX,XX +XXX,XX @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,

#if defined(TARGET_NR_clock_adjtime) && defined(CONFIG_CLOCK_ADJTIME)

case TARGET_NR_clock_adjtime:

{

- struct timex htx, *phtx = &htx;

+ struct timex htx;

- if (target_to_host_timex(phtx, arg2) != 0) {

+ if (target_to_host_timex(&htx, arg2) != 0) {

return -TARGET_EFAULT;

}

- ret = get_errno(clock_adjtime(arg1, phtx));

- if (!is_error(ret) && phtx) {

- if (host_to_target_timex(arg2, phtx) != 0) {

- return -TARGET_EFAULT;

- }

+ ret = get_errno(clock_adjtime(arg1, &htx));

+ if (!is_error(ret) && host_to_target_timex(arg2, &htx)) {

+ return -TARGET_EFAULT;

}

return ret;

2.34.1

Add comments to the in_* fields in the S1Translate struct

that explain what they're doing.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Message-id: 20230710152130.3928330-2-peter.maydell@linaro.org

target/arm/ptw.c | 40 ++++++++++++++++++++++++++++++++++++++++

1 file changed, 40 insertions(+)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c

--- a/target/arm/ptw.c

+++ b/target/arm/ptw.c

@@ -XXX,XX +XXX,XX @@

#endif

typedef struct S1Translate {

+ /*

+ * in_mmu_idx : specifies which TTBR, TCR, etc to use for the walk.

+ * Together with in_space, specifies the architectural translation regime.

+ */

ARMMMUIdx in_mmu_idx;

+ /*

+ * in_ptw_idx: specifies which mmuidx to use for the actual

+ * page table descriptor load operations. This will be one of the

+ * ARMMMUIdx_Stage2* or one of the ARMMMUIdx_Phys_* indexes.

+ * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit,

+ * this field is updated accordingly.

+ */

ARMMMUIdx in_ptw_idx;

+ /*

+ * in_space: the security space for this walk. This plus

+ * the in_mmu_idx specify the architectural translation regime.

+ * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit,

+ * this field is updated accordingly.

+ *

+ * Note that the security space for the in_ptw_idx may be different

+ * from that for the in_mmu_idx. We do not need to explicitly track

+ * the in_ptw_idx security space because:

+ * - if the in_ptw_idx is an ARMMMUIdx_Phys_* then the mmuidx

+ * itself specifies the security space

+ * - if the in_ptw_idx is an ARMMMUIdx_Stage2* then the security

+ * space used for ptw reads is the same as that of the security

+ * space of the stage 1 translation for all cases except where

+ * stage 1 is Secure; in that case the only possibilities for

+ * the ptw read are Secure and NonSecure, and the in_ptw_idx

+ * value being Stage2 vs Stage2_S distinguishes those.

+ */

ARMSecuritySpace in_space;

+ /*

+ * in_secure: whether the translation regime is a Secure one.

+ * This is always equal to arm_space_is_secure(in_space).

+ * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit,

+ * this field is updated accordingly.

+ */

bool in_secure;

+ /*

+ * in_debug: is this a QEMU debug access (gdbstub, etc)? Debug

+ * accesses will not update the guest page table access flags

+ * and will not change the state of the softmmu TLBs.

+ */

bool in_debug;

/*

* If this is stage 2 of a stage 1+2 page table walk, then this must

2.34.1

In get_phys_addr_twostage() the code that applies the effects of

VSTCR.{SA,SW} and VTCR.{NSA,NSW} only updates result->f.attrs.secure.

Now we also have f.attrs.space for FEAT_RME, we need to keep the two

in sync.

These bits only have an effect for Secure space translations, not

for Root, so use the input in_space field to determine whether to

apply them rather than the input is_secure. This doesn't actually

make a difference because Root translations are never two-stage,

but it's a little clearer.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Message-id: 20230710152130.3928330-4-peter.maydell@linaro.org

target/arm/ptw.c | 13 ++++++++-----

1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c

--- a/target/arm/ptw.c

+++ b/target/arm/ptw.c

@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,

hwaddr ipa;

int s1_prot, s1_lgpgsz;

bool is_secure = ptw->in_secure;

+ ARMSecuritySpace in_space = ptw->in_space;

bool ret, ipa_secure;

ARMCacheAttrs cacheattrs1;

ARMSecuritySpace ipa_space;

@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,

* Check if IPA translates to secure or non-secure PA space.

* Note that VSTCR overrides VTCR and {N}SW overrides {N}SA.

*/

- result->f.attrs.secure =

- (is_secure

- && !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))

- && (ipa_secure

- || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))));

+ if (in_space == ARMSS_Secure) {

+ result->f.attrs.secure =

+ !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))

+ && (ipa_secure

+ || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW)));

+ result->f.attrs.space = arm_secure_to_space(result->f.attrs.secure);

+ }

return false;

2.34.1

In commit f0a08b0913befbd we changed the type of the PC from

target_ulong to vaddr. In doing so we inadvertently dropped the

zero-padding on the PC in trace lines (the second item inside the []

in these lines). They used to look like this on AArch64, for

instance:

Trace 0: 0x7f2260000100 [00000000/0000000040000000/00000061/ff200000]

and now they look like this:

Trace 0: 0x7f4f50000100 [00000000/40000000/00000061/ff200000]

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Anton Johansson <anjo@rev.ng>

Message-id: 20230711165434.4123674-1-peter.maydell@linaro.org

accel/tcg/cpu-exec.c | 4 ++--

accel/tcg/translate-all.c | 2 +-

2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c

--- a/accel/tcg/cpu-exec.c

+++ b/accel/tcg/cpu-exec.c

@@ -XXX,XX +XXX,XX @@ static void log_cpu_exec(vaddr pc, CPUState *cpu,

if (qemu_log_in_addr_range(pc)) {

qemu_log_mask(CPU_LOG_EXEC,

"Trace %d: %p [%08" PRIx64

- "/%" VADDR_PRIx "/%08x/%08x] %s\n",

+ "/%016" VADDR_PRIx "/%08x/%08x] %s\n",

cpu->cpu_index, tb->tc.ptr, tb->cs_base, pc,

tb->flags, tb->cflags, lookup_symbol(pc));

@@ -XXX,XX +XXX,XX @@ cpu_tb_exec(CPUState *cpu, TranslationBlock *itb, int *tb_exit)

if (qemu_loglevel_mask(CPU_LOG_EXEC)) {

vaddr pc = log_pc(cpu, last_tb);

if (qemu_log_in_addr_range(pc)) {

- qemu_log("Stopped execution of TB chain before %p [%"

+ qemu_log("Stopped execution of TB chain before %p [%016"

VADDR_PRIx "] %s\n",

last_tb->tc.ptr, pc, lookup_symbol(pc));

}

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/translate-all.c

+++ b/accel/tcg/translate-all.c

@@ -XXX,XX +XXX,XX @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)

if (qemu_loglevel_mask(CPU_LOG_EXEC)) {

vaddr pc = log_pc(cpu, tb);

if (qemu_log_in_addr_range(pc)) {

- qemu_log("cpu_io_recompile: rewound execution of TB to %"

+ qemu_log("cpu_io_recompile: rewound execution of TB to %016"

VADDR_PRIx "\n", pc);

}

2.34.1

From: Tong Ho <tong.ho@amd.com>

hw/nvram/xlnx-efuse.c | 11 +++++++++--

1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/xlnx-efuse.c b/hw/nvram/xlnx-efuse.c

--- a/hw/nvram/xlnx-efuse.c

+++ b/hw/nvram/xlnx-efuse.c

@@ -XXX,XX +XXX,XX @@ static bool efuse_ro_bits_find(XlnxEFuse *s, uint32_t k)

bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)

{

+ uint32_t set, *row;

+

if (efuse_ro_bits_find(s, bit)) {

g_autofree char *path = object_get_canonical_path(OBJECT(s));

@@ -XXX,XX +XXX,XX @@ bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)

return false;

- s->fuse32[bit / 32] |= 1 << (bit % 32);

- efuse_bdrv_sync(s, bit);

+ /* Avoid back-end write unless there is a real update */

+ row = &s->fuse32[bit / 32];

+ set = 1 << (bit % 32);

+ if (!(set & *row)) {

+ *row |= set;

+ efuse_bdrv_sync(s, bit);

+ }

return true;

}

2.34.1