Re: [Qemu-devel] [PATCH v4 05/14] i.MX: Add code to emulate i.MX7 SNVS IP-block

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Wed, Jan 31, 2018 at 9:10 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 01/15/2018 10:37 PM, Andrey Smirnov wrote:
>> Add code to emulate SNVS IP-block. Currently only the bits needed to
>> be able to emulate machine shutdown are implemented.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/misc/Makefile.objs       |  1 +
>>  hw/misc/imx7_snvs.c         | 83 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/misc/imx7_snvs.h | 35 +++++++++++++++++++
>>  3 files changed, 119 insertions(+)
>>  create mode 100644 hw/misc/imx7_snvs.c
>>  create mode 100644 include/hw/misc/imx7_snvs.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 4b2b705a6c..019886912c 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx6_ccm.o
>>  obj-$(CONFIG_IMX) += imx6_src.o
>>  obj-$(CONFIG_IMX) += imx7_ccm.o
>>  obj-$(CONFIG_IMX) += imx2_wdt.o
>> +obj-$(CONFIG_IMX) += imx7_snvs.o
>>  obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>>  obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>>  obj-$(CONFIG_MAINSTONE) += mst_fpga.o
>> diff --git a/hw/misc/imx7_snvs.c b/hw/misc/imx7_snvs.c
>> new file mode 100644
>> index 0000000000..670b9f4639
>> --- /dev/null
>> +++ b/hw/misc/imx7_snvs.c
>> @@ -0,0 +1,83 @@
>> +/*
>> + * IMX7 Secure Non-Volatile Storage
>> + *
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + * Bare minimum emulation code needed to support being able to shut
>> + * down linux guest gracefully.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/misc/imx7_snvs.h"
>> +#include "qemu/log.h"
>> +#include "sysemu/sysemu.h"
>> +
>> +static uint64_t imx7_snvs_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void imx7_snvs_write(void *opaque, hwaddr offset,
>> +                            uint64_t v, unsigned size)
>> +{
>> +    const uint32_t value = v;
>> +    const uint32_t mask  = SNVS_LPCR_TOP | SNVS_LPCR_DP_EN;
>> +
>> +    if (offset == SNVS_LPCR && ((value & mask) == mask)) {
>> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +    }
>> +}
>> +
>> +static const struct MemoryRegionOps imx7_snvs_ops = {
>> +    .read = imx7_snvs_read,
>
> Same here, you can remove the imx7_snvs_read() function since
> memory_region_dispatch_read() takes care of this and return 0.
>

Hmm, I don't think I agree both from reading code and trying this out.
Without .read callback the call chain ends up being the following
memory_region_dispatch_read() -> memory_region_dispatch_read1() ->
access_with_adjusted_size(..., memory_region_oldmmio_read_accessor,
...) -> memory_region_oldmmio_read_accessor() ->
mr->ops->old_mmio.read[ctz32(size)]() -> SEGFAULT

As much as I'd love to get rid of dummy .read callback, I don't see a
way to do that, unfortunately.

Thanks,
Andrey Smirnov