target-arm queue for rc1 -- these are all bug fixes.

The following changes since commit b9404bf592e7ba74180e1a54ed7a266ec6ee67f2:

Merge remote-tracking branch 'remotes/dgilbert/tags/pull-hmp-20190715' into staging (2019-07-15 12:22:07 +0100)

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

for you to fetch changes up to 51c9122e92b776a3f16af0b9282f1dc5012e2a19:

target/arm: NS BusFault on vector table fetch escalates to NS HardFault (2019-07-15 14:17:04 +0100)

* report ARMv8-A FP support for AArch32 -cpu max

* hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

* hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

* hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO

* hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO

* hw/arm/virt: Fix non-secure flash mode

* pl031: Correctly migrate state when using -rtc clock=host

* fix regression that meant arm926 and arm1026 lost VFP

double-precision support

* v8M: NS BusFault on vector table fetch escalates to NS HardFault

Alex Bennée (1):

target/arm: report ARMv8-A FP support for AArch32 -cpu max

David Engraf (1):

hw/arm/virt: Fix non-secure flash mode

Peter Maydell (3):

pl031: Correctly migrate state when using -rtc clock=host

target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026

target/arm: NS BusFault on vector table fetch escalates to NS HardFault

Philippe Mathieu-Daudé (5):

hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO

hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO

include/hw/timer/pl031.h | 2 ++

hw/arm/virt.c | 2 +-

hw/core/machine.c | 1 +

hw/display/xlnx_dp.c | 15 +++++---

hw/ssi/mss-spi.c | 8 ++++-

hw/ssi/xilinx_spips.c | 43 +++++++++++++++-------

hw/timer/pl031.c | 92 +++++++++++++++++++++++++++++++++++++++++++++---

target/arm/cpu.c | 16 +++++++++

target/arm/m_helper.c | 21 ++++++++---

9 files changed, 174 insertions(+), 26 deletions(-)

The ARMv5 architecture didn't specify detailed per-feature ID

registers. Now that we're using the MVFR0 register fields to

gate the existence of VFP instructions, we need to set up

the correct values in the cpu->isar structure so that we still

provide an FPU to the guest.

This fixes a regression in the arm926 and arm1026 CPUs, which

are the only ones that both have VFP and are ARMv5 or earlier.

This regression was introduced by the VFP refactoring, and more

specifically by commits 1120827fa182f0e76 and 266bd25c485597c,

which accidentally disabled VFP short-vector support and

double-precision support on these CPUs.

Fixes: 1120827fa182f0e

Fixes: 266bd25c485597c

Fixes: https://bugs.launchpad.net/qemu/+bug/1836192

Reported-by: Christophe Lyon <christophe.lyon@linaro.org>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Tested-by: Christophe Lyon <christophe.lyon@linaro.org>

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

---

target/arm/cpu.c | 12 ++++++++++++

1 file changed, 12 insertions(+)

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

--- a/target/arm/cpu.c

+++ b/target/arm/cpu.c

@@ -XXX,XX +XXX,XX @@ static void arm926_initfn(Object *obj)

* set the field to indicate Jazelle support within QEMU.

*/

cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1);

+ /*

+ * Similarly, we need to set MVFR0 fields to enable double precision

+ * and short vector support even though ARMv5 doesn't have this register.

+ */

+ cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);

+ cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);

}

static void arm946_initfn(Object *obj)

@@ -XXX,XX +XXX,XX @@ static void arm1026_initfn(Object *obj)

* set the field to indicate Jazelle support within QEMU.

*/

cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1);

+ /*

+ * Similarly, we need to set MVFR0 fields to enable double precision

+ * and short vector support even though ARMv5 doesn't have this register.

+ */

+ cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1);

+ cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1);

{

/* The 1026 had an IFAR at c6,c0,0,1 rather than the ARMv6 c6,c0,0,2 */

2.20.1

In the M-profile architecture, when we do a vector table fetch and it

fails, we need to report a HardFault. Whether this is a Secure HF or

a NonSecure HF depends on several things. If AIRCR.BFHFNMINS is 0

then HF is always Secure, because there is no NonSecure HardFault.

Otherwise, the answer depends on whether the 'underlying exception'

(MemManage, BusFault, SecureFault) targets Secure or NonSecure. (In

the pseudocode, this is handled in the Vector() function: the final

exc.isSecure is calculated by looking at the exc.isSecure from the

exception returned from the memory access, not the isSecure input

argument.)

We weren't doing this correctly, because we were looking at

the target security domain of the exception we were trying to

load the vector table entry for. This produces errors of two kinds:

* a load from the NS vector table which hits the "NS access

to S memory" SecureFault should end up as a Secure HardFault,

but we were raising an NS HardFault

* a load from the S vector table which causes a BusFault

should raise an NS HardFault if BFHFNMINS == 1 (because

in that case all BusFaults are NonSecure), but we were raising

a Secure HardFault

Correct the logic.

We also fix a comment error where we claimed that we might

be escalating MemManage to HardFault, and forgot about SecureFault.

(Vector loads can never hit MPU access faults, because they're

always aligned and always use the default address map.)

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

target/arm/m_helper.c | 21 +++++++++++++++++----

1 file changed, 17 insertions(+), 4 deletions(-)

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

--- a/target/arm/m_helper.c

+++ b/target/arm/m_helper.c

@@ -XXX,XX +XXX,XX @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,

if (sattrs.ns) {

attrs.secure = false;

} else if (!targets_secure) {

- /* NS access to S memory */

+ /*

+ * NS access to S memory: the underlying exception which we escalate

+ * to HardFault is SecureFault, which always targets Secure.

+ */

+ exc_secure = true;

goto load_fail;

}

}

@@ -XXX,XX +XXX,XX @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,

vector_entry = address_space_ldl(arm_addressspace(cs, attrs), addr,

attrs, &result);

if (result != MEMTX_OK) {

+ /*

+ * Underlying exception is BusFault: its target security state

+ * depends on BFHFNMINS.

+ */

+ exc_secure = !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);

goto load_fail;

}

*pvec = vector_entry;

@@ -XXX,XX +XXX,XX @@ load_fail:

/*

* All vector table fetch fails are reported as HardFault, with

* HFSR.VECTTBL and .FORCED set. (FORCED is set because

- * technically the underlying exception is a MemManage or BusFault

+ * technically the underlying exception is a SecureFault or BusFault

* that is escalated to HardFault.) This is a terminal exception,

* so we will either take the HardFault immediately or else enter

* lockup (the latter case is handled in armv7m_nvic_set_pending_derived()).

+ * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are

+ * secure); otherwise it targets the same security state as the

+ * underlying exception.

*/

- exc_secure = targets_secure ||

- !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);

+ if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) {

+ exc_secure = true;

+ }

env->v7m.hfsr |= R_V7M_HFSR_VECTTBL_MASK | R_V7M_HFSR_FORCED_MASK;

armv7m_nvic_set_pending_derived(env->nvic, ARMV7M_EXCP_HARD, exc_secure);

return false;

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

In the next commit we will implement the write_with_attrs()

handler. To avoid using different APIs, convert the read()

handler first.

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

hw/ssi/xilinx_spips.c | 23 +++++++++++------------

1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c

--- a/hw/ssi/xilinx_spips.c

+++ b/hw/ssi/xilinx_spips.c

@@ -XXX,XX +XXX,XX @@ static void lqspi_load_cache(void *opaque, hwaddr addr)

}

-static uint64_t

-lqspi_read(void *opaque, hwaddr addr, unsigned int size)

+static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,

+ unsigned size, MemTxAttrs attrs)

{

- XilinxQSPIPS *q = opaque;

- uint32_t ret;

+ XilinxQSPIPS *q = XILINX_QSPIPS(opaque);

if (addr >= q->lqspi_cached_addr &&

addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {

uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];

- ret = cpu_to_le32(*(uint32_t *)retp);

- DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,

- (unsigned)ret);

- return ret;

- } else {

- lqspi_load_cache(opaque, addr);

- return lqspi_read(opaque, addr, size);

+ *value = cpu_to_le32(*(uint32_t *)retp);

+ DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",

+ addr, *value);

+ return MEMTX_OK;

}

+

+ lqspi_load_cache(opaque, addr);

+ return lqspi_read(opaque, addr, value, size, attrs);

static const MemoryRegionOps lqspi_ops = {

- .read = lqspi_read,

+ .read_with_attrs = lqspi_read,

.endianness = DEVICE_NATIVE_ENDIAN,

.valid = {

.min_access_size = 1,

2.20.1

From: David Engraf <david.engraf@sysgo.com>

Using the whole 128 MiB flash in non-secure mode is not working because

virt_flash_fdt() expects the same address for secure_sysmem and sysmem.

This is not correctly handled by caller because it forwards NULL for

secure_sysmem in non-secure flash mode.

Fixed by using sysmem when secure_sysmem is NULL.

Signed-off-by: David Engraf <david.engraf@sysgo.com>

Message-id: 20190712075002.14326-1-david.engraf@sysgo.com

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

hw/arm/virt.c | 2 +-

diff --git a/hw/arm/virt.c b/hw/arm/virt.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/arm/virt.c

+++ b/hw/arm/virt.c

@@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine)

&machine->device_memory->mr);

}

- virt_flash_fdt(vms, sysmem, secure_sysmem);

+ virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem);

create_gic(vms, pic);

2.20.1

The PL031 RTC tracks the difference between the guest RTC

and the host RTC using a tick_offset field. For migration,

however, we currently always migrate the offset between

the guest and the vm_clock, even if the RTC clock is not

the same as the vm_clock; this was an attempt to retain

migration backwards compatibility.

Unfortunately this results in the RTC behaving oddly across

a VM state save and restore -- since the VM clock stands still

across save-then-restore, regardless of how much real world

time has elapsed, the guest RTC ends up out of sync with the

host RTC in the restored VM.

Fix this by migrating the raw tick_offset. To retain migration

compatibility as far as possible, we have a new property

migrate-tick-offset; by default this is 'true' and we will

migrate the true tick offset in a new subsection; if the

incoming data has no subsection we fall back to the old

vm_clock-based offset information, so old->new migration

compatibility is preserved. For complete new->old migration

compatibility, the property is set to 'false' for 4.0 and

earlier machine types (this will only affect 'virt-4.0'

and below, as none of the other pl031-using machines are

versioned).

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

include/hw/timer/pl031.h | 2 +

hw/core/machine.c | 1 +

hw/timer/pl031.c | 92 ++++++++++++++++++++++++++++++++++++++--

3 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/include/hw/timer/pl031.h b/include/hw/timer/pl031.h

index XXXXXXX..XXXXXXX 100644

--- a/include/hw/timer/pl031.h

+++ b/include/hw/timer/pl031.h

@@ -XXX,XX +XXX,XX @@ typedef struct PL031State {

*/

uint32_t tick_offset_vmstate;

uint32_t tick_offset;

+ bool tick_offset_migrated;

+ bool migrate_tick_offset;

uint32_t mr;

uint32_t lr;

diff --git a/hw/core/machine.c b/hw/core/machine.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/core/machine.c

+++ b/hw/core/machine.c

@@ -XXX,XX +XXX,XX @@ GlobalProperty hw_compat_4_0[] = {

{ "virtio-gpu-pci", "edid", "false" },

{ "virtio-device", "use-started", "false" },

{ "virtio-balloon-device", "qemu-4-0-config-size", "true" },

+ { "pl031", "migrate-tick-offset", "false" },

};

const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);

diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/timer/pl031.c

+++ b/hw/timer/pl031.c

@@ -XXX,XX +XXX,XX @@ static int pl031_pre_save(void *opaque)

{

PL031State *s = opaque;

- /* tick_offset is base_time - rtc_clock base time. Instead, we want to

- * store the base time relative to the QEMU_CLOCK_VIRTUAL for backwards-compatibility. */

+ /*

+ * The PL031 device model code uses the tick_offset field, which is

+ * the offset between what the guest RTC should read and what the

+ * QEMU rtc_clock reads:

+ * guest_rtc = rtc_clock + tick_offset

+ * and so

+ * tick_offset = guest_rtc - rtc_clock

+ *

+ * We want to migrate this offset, which sounds straightforward.

+ * Unfortunately older versions of QEMU migrated a conversion of this

+ * offset into an offset from the vm_clock. (This was in turn an

+ * attempt to be compatible with even older QEMU versions, but it

+ * has incorrect behaviour if the rtc_clock is not the same as the

+ * vm_clock.) So we put the actual tick_offset into a migration

+ * subsection, and the backwards-compatible time-relative-to-vm_clock

+ * in the main migration state.

+ *

+ * Calculate base time relative to QEMU_CLOCK_VIRTUAL:

+ */

int64_t delta = qemu_clock_get_ns(rtc_clock) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND;

return 0;

}

+static int pl031_pre_load(void *opaque)

+{

+ PL031State *s = opaque;

+

+ s->tick_offset_migrated = false;

+ return 0;

+}

+

static int pl031_post_load(void *opaque, int version_id)

{

PL031State *s = opaque;

- int64_t delta = qemu_clock_get_ns(rtc_clock) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

- s->tick_offset = s->tick_offset_vmstate - delta / NANOSECONDS_PER_SECOND;

+ /*

+ * If we got the tick_offset subsection, then we can just use

+ * the value in that. Otherwise the source is an older QEMU and

+ * has given us the offset from the vm_clock; convert it back to

+ * an offset from the rtc_clock. This will cause time to incorrectly

+ * go backwards compared to the host RTC, but this is unavoidable.

+ */

+

+ if (!s->tick_offset_migrated) {

+ int64_t delta = qemu_clock_get_ns(rtc_clock) -

+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

+ s->tick_offset = s->tick_offset_vmstate -

+ delta / NANOSECONDS_PER_SECOND;

+ }

pl031_set_alarm(s);

return 0;

}

+static int pl031_tick_offset_post_load(void *opaque, int version_id)

+{

+ PL031State *s = opaque;

+

+ s->tick_offset_migrated = true;

+ return 0;

+}

+

+static bool pl031_tick_offset_needed(void *opaque)

+{

+ PL031State *s = opaque;

+

+ return s->migrate_tick_offset;

+}

+

+static const VMStateDescription vmstate_pl031_tick_offset = {

+ .name = "pl031/tick-offset",

+ .version_id = 1,

+ .minimum_version_id = 1,

+ .needed = pl031_tick_offset_needed,

+ .post_load = pl031_tick_offset_post_load,

+ .fields = (VMStateField[]) {

+ VMSTATE_UINT32(tick_offset, PL031State),

+ VMSTATE_END_OF_LIST()

+ }

+};

+

static const VMStateDescription vmstate_pl031 = {

.name = "pl031",

.version_id = 1,

.minimum_version_id = 1,

.pre_save = pl031_pre_save,

+ .pre_load = pl031_pre_load,

.post_load = pl031_post_load,

.fields = (VMStateField[]) {

VMSTATE_UINT32(tick_offset_vmstate, PL031State),

@@ -XXX,XX +XXX,XX @@ static const VMStateDescription vmstate_pl031 = {

VMSTATE_UINT32(im, PL031State),

VMSTATE_UINT32(is, PL031State),

VMSTATE_END_OF_LIST()

+ },

+ .subsections = (const VMStateDescription*[]) {

+ &vmstate_pl031_tick_offset,

+ NULL

}

};

+static Property pl031_properties[] = {

+ /*

+ * True to correctly migrate the tick offset of the RTC. False to

+ * obtain backward migration compatibility with older QEMU versions,

+ * at the expense of the guest RTC going backwards compared with the

+ * host RTC when the VM is saved/restored if using -rtc host.

+ * (Even if set to 'true' older QEMU can migrate forward to newer QEMU;

+ * 'false' also permits newer QEMU to migrate to older QEMU.)

+ */

+ DEFINE_PROP_BOOL("migrate-tick-offset",

+ PL031State, migrate_tick_offset, true),

+ DEFINE_PROP_END_OF_LIST()

+};

+

static void pl031_class_init(ObjectClass *klass, void *data)

{

DeviceClass *dc = DEVICE_CLASS(klass);

dc->vmsd = &vmstate_pl031;

+ dc->props = pl031_properties;

}

static const TypeInfo pl031_info = {

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

In the previous commit we fixed a crash when the guest read a

register that pop from an empty FIFO.

By auditing the repository, we found another similar use with

an easy way to reproduce:

$ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S

QEMU 4.0.50 monitor - type 'help' for more information

(qemu) xp/b 0xfd4a0134

Aborted (core dumped)

(gdb) bt

#0 0x00007f6936dea57f in raise () at /lib64/libc.so.6

#1 0x00007f6936dd4895 in abort () at /lib64/libc.so.6

#2 0x0000561ad32975ec in xlnx_dp_aux_pop_rx_fifo (s=0x7f692babee70) at hw/display/xlnx_dp.c:431

#3 0x0000561ad3297dc0 in xlnx_dp_read (opaque=0x7f692babee70, offset=77, size=4) at hw/display/xlnx_dp.c:667

#4 0x0000561ad321b896 in memory_region_read_accessor (mr=0x7f692babf620, addr=308, value=0x7ffe05c1db88, size=4, shift=0, mask=4294967295, attrs=...) at memory.c:439

#5 0x0000561ad321bd70 in access_with_adjusted_size (addr=308, value=0x7ffe05c1db88, size=1, access_size_min=4, access_size_max=4, access_fn=0x561ad321b858 <memory_region_read_accessor>, mr=0x7f692babf620, attrs=...) at memory.c:569

#6 0x0000561ad321e9d5 in memory_region_dispatch_read1 (mr=0x7f692babf620, addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1420

#7 0x0000561ad321ea9d in memory_region_dispatch_read (mr=0x7f692babf620, addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1447

#8 0x0000561ad31bd742 in flatview_read_continue (fv=0x561ad69c04f0, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1, addr1=308, l=1, mr=0x7f692babf620) at exec.c:3385

#9 0x0000561ad31bd895 in flatview_read (fv=0x561ad69c04f0, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1) at exec.c:3423

#10 0x0000561ad31bd90b in address_space_read_full (as=0x561ad5bb3020, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1) at exec.c:3436

#11 0x0000561ad33b1c42 in address_space_read (len=1, buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", attrs=..., addr=4249485620, as=0x561ad5bb3020) at include/exec/memory.h:2131

#12 0x0000561ad33b1c42 in memory_dump (mon=0x561ad59c4530, count=1, format=120, wsize=1, addr=4249485620, is_physical=1) at monitor/misc.c:723

#13 0x0000561ad33b1fc1 in hmp_physical_memory_dump (mon=0x561ad59c4530, qdict=0x561ad6c6fd00) at monitor/misc.c:795

#14 0x0000561ad37b4a9f in handle_hmp_command (mon=0x561ad59c4530, cmdline=0x561ad59d0f22 "/b 0x00000000fd4a0134") at monitor/hmp.c:1082

Fix by checking the FIFO is not empty before popping from it.

The datasheet is not clear about the reset value of this register,

we choose to return '0'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Message-id: 20190709113715.7761-4-philmd@redhat.com

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

hw/display/xlnx_dp.c | 15 +++++++++++----

1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/display/xlnx_dp.c

+++ b/hw/display/xlnx_dp.c

@@ -XXX,XX +XXX,XX @@ static uint8_t xlnx_dp_aux_pop_rx_fifo(XlnxDPState *s)

uint8_t ret;

if (fifo8_is_empty(&s->rx_fifo)) {

- DPRINTF("rx_fifo underflow..\n");

- abort();

+ qemu_log_mask(LOG_GUEST_ERROR,

+ "%s: Reading empty RX_FIFO\n",

+ __func__);

+ /*

+ * The datasheet is not clear about the reset value, it seems

+ * to be unspecified. We choose to return '0'.

+ */

+ ret = 0;

+ } else {

+ ret = fifo8_pop(&s->rx_fifo);

+ DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret);

}

- ret = fifo8_pop(&s->rx_fifo);

- DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret);

return ret;

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Reading the RX_DATA register when the RX_FIFO is empty triggers

an abort. This can be easily reproduced:

$ qemu-system-arm -M emcraft-sf2 -monitor stdio -S

QEMU 4.0.50 monitor - type 'help' for more information

(qemu) x 0x40001010

Aborted (core dumped)

(gdb) bt

#1 0x00007f035874f895 in abort () at /lib64/libc.so.6

#2 0x00005628686591ff in fifo8_pop (fifo=0x56286a9a4c68) at util/fifo8.c:66

#3 0x00005628683e0b8e in fifo32_pop (fifo=0x56286a9a4c68) at include/qemu/fifo32.h:137

#4 0x00005628683e0efb in spi_read (opaque=0x56286a9a4850, addr=4, size=4) at hw/ssi/mss-spi.c:168

#5 0x0000562867f96801 in memory_region_read_accessor (mr=0x56286a9a4b60, addr=16, value=0x7ffeecb0c5c8, size=4, shift=0, mask=4294967295, attrs=...) at memory.c:439

#6 0x0000562867f96cdb in access_with_adjusted_size (addr=16, value=0x7ffeecb0c5c8, size=4, access_size_min=1, access_size_max=4, access_fn=0x562867f967c3 <memory_region_read_accessor>, mr=0x56286a9a4b60, attrs=...) at memory.c:569

#7 0x0000562867f99940 in memory_region_dispatch_read1 (mr=0x56286a9a4b60, addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1420

#8 0x0000562867f99a08 in memory_region_dispatch_read (mr=0x56286a9a4b60, addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1447

#9 0x0000562867f38721 in flatview_read_continue (fv=0x56286aec6360, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, addr1=16, l=4, mr=0x56286a9a4b60) at exec.c:3385

#10 0x0000562867f38874 in flatview_read (fv=0x56286aec6360, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at exec.c:3423

#11 0x0000562867f388ea in address_space_read_full (as=0x56286aa3e890, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at exec.c:3436

#12 0x0000562867f389c5 in address_space_rw (as=0x56286aa3e890, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, is_write=false) at exec.c:3466

#13 0x0000562867f3bdd7 in cpu_memory_rw_debug (cpu=0x56286aa19d00, addr=1073745936, buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, is_write=0) at exec.c:3976

#14 0x000056286811ed51 in memory_dump (mon=0x56286a8c32d0, count=1, format=120, wsize=4, addr=1073745936, is_physical=0) at monitor/misc.c:730

#15 0x000056286811eff1 in hmp_memory_dump (mon=0x56286a8c32d0, qdict=0x56286b15c400) at monitor/misc.c:785

#16 0x00005628684740ee in handle_hmp_command (mon=0x56286a8c32d0, cmdline=0x56286a8caeb2 "0x40001010") at monitor/hmp.c:1082

From the datasheet "Actel SmartFusion Microcontroller Subsystem

User's Guide" Rev.1, Table 13-3 "SPI Register Summary", this

register has a reset value of 0.

Check the FIFO is not empty before accessing it, else log an

error message.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Message-id: 20190709113715.7761-3-philmd@redhat.com

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

hw/ssi/mss-spi.c | 8 +++++++-

1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/ssi/mss-spi.c

+++ b/hw/ssi/mss-spi.c

@@ -XXX,XX +XXX,XX @@ spi_read(void *opaque, hwaddr addr, unsigned int size)

case R_SPI_RX:

s->regs[R_SPI_STATUS] &= ~S_RXFIFOFUL;

s->regs[R_SPI_STATUS] &= ~S_RXCHOVRF;

- ret = fifo32_pop(&s->rx_fifo);

+ if (fifo32_is_empty(&s->rx_fifo)) {

+ qemu_log_mask(LOG_GUEST_ERROR,

+ "%s: Reading empty RX_FIFO\n",

+ __func__);

+ } else {

+ ret = fifo32_pop(&s->rx_fifo);

+ }

if (fifo32_is_empty(&s->rx_fifo)) {

s->regs[R_SPI_STATUS] |= S_RXFIFOEMP;

}

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Both lqspi_read() and lqspi_load_cache() expect a 32-bit

aligned address.

>From UG1085 datasheet [*] chapter on 'Quad-SPI Controller':

Transfer Size Limitations

Because of the 32-bit wide TX, RX, and generic FIFO, all

APB/AXI transfers must be an integer multiple of 4-bytes.

Shorter transfers are not possible.

Set MemoryRegionOps.impl values to force 32-bit accesses,

this way we are sure we do not access the lqspi_buf[] array

out of bound.

[*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---

hw/ssi/xilinx_spips.c | 4 ++++

1 file changed, 4 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/ssi/xilinx_spips.c

+++ b/hw/ssi/xilinx_spips.c

@@ -XXX,XX +XXX,XX @@ static const MemoryRegionOps lqspi_ops = {

.read_with_attrs = lqspi_read,

.write_with_attrs = lqspi_write,

.endianness = DEVICE_NATIVE_ENDIAN,

+ .impl = {

+ .min_access_size = 4,

+ .max_access_size = 4,

+ },

.valid = {

.min_access_size = 1,

.max_access_size = 4

2.20.1

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Lei Sun found while auditing the code that a CPU write would

trigger a NULL pointer dereference.

>From UG1085 datasheet [*] AXI writes in this region are ignored

and generates an AXI Slave Error (SLVERR).

Fix by implementing the write_with_attrs() handler.

Return MEMTX_ERROR when the region is accessed (this error maps

to an AXI slave error).

[*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

Reported-by: Lei Sun <slei.casper@gmail.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

hw/ssi/xilinx_spips.c | 16 ++++++++++++++++

1 file changed, 16 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c

--- a/hw/ssi/xilinx_spips.c

+++ b/hw/ssi/xilinx_spips.c

@@ -XXX,XX +XXX,XX @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,

return lqspi_read(opaque, addr, value, size, attrs);

}

+static MemTxResult lqspi_write(void *opaque, hwaddr offset, uint64_t value,

+ unsigned size, MemTxAttrs attrs)

+{

+ /*

+ * From UG1085, Chapter 24 (Quad-SPI controllers):

+ * - Writes are ignored

+ * - AXI writes generate an external AXI slave error (SLVERR)

+ */

+ qemu_log_mask(LOG_GUEST_ERROR, "%s Unexpected %u-bit access to 0x%" PRIx64

+ " (value: 0x%" PRIx64 "\n",

+ __func__, size << 3, offset, value);

+

+ return MEMTX_ERROR;

+}

+

static const MemoryRegionOps lqspi_ops = {

.read_with_attrs = lqspi_read,

+ .write_with_attrs = lqspi_write,

.endianness = DEVICE_NATIVE_ENDIAN,

.valid = {

.min_access_size = 1,

2.20.1