Re: [Qemu-devel] [Qemu-devel PATCH 2/5] msf2: Microsemi Smartfusion2 System Register block.

[Philippe Mathieu-Daudé]

On 05/12/2017 12:48 AM, sundeep subbaraya wrote:
> Hi Phiilippe,
>
> On Wed, May 10, 2017 at 4:04 PM, Philippe Mathieu-Daudé <address@hidden>
> wrote:
>
> > Hi Subbaraya,
> >
> >
> > On 05/09/2017 01:44 PM, Subbaraya Sundeep wrote:
> >
> > > Added Sytem register block of Smartfusion2.
> > > This block has PLL registers which are accessed by guest.
> > >
> > > Signed-off-by: Subbaraya Sundeep <address@hidden>
> > > ---
> > >  hw/misc/Makefile.objs         |   1 +
> > >  hw/misc/msf2-sysreg.c         | 131 ++++++++++++++++++++++++++++++
> > > ++++++++++++
> > >  include/hw/misc/msf2-sysreg.h |  80 ++++++++++++++++++++++++++
> > >  3 files changed, 212 insertions(+)
> > >  create mode 100644 hw/misc/msf2-sysreg.c
> > >  create mode 100644 include/hw/misc/msf2-sysreg.h
> > >
> > > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > > index c8b4893..0f52354 100644
> > > --- a/hw/misc/Makefile.objs
> > > +++ b/hw/misc/Makefile.objs
> > > @@ -56,3 +56,4 @@ obj-$(CONFIG_EDU) += edu.o
> > >  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
> > >  obj-$(CONFIG_AUX) += auxbus.o
> > >  obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
> > > +obj-$(CONFIG_MSF2) += msf2-sysreg.o
> > > diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
> > > new file mode 100644
> > > index 0000000..53e9cba
> > > --- /dev/null
> > > +++ b/hw/misc/msf2-sysreg.c
> > > @@ -0,0 +1,131 @@
> > > +/*
> > > + * System Register block model of Microsemi SmartFusion2.
> > > + *
> > > + * Copyright (c) 2017 Subbaraya Sundeep <address@hidden>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * as published by the Free Software Foundation; either version
> > > + * 2 of the License, or (at your option) any later version.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > along
> > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include "hw/misc/msf2-sysreg.h"
> > > +
> > > +#ifndef MSF2_SYSREG_ERR_DEBUG
> > > +#define MSF2_SYSREG_ERR_DEBUG  0
> > > +#endif
> > > +
> > > +#define DB_PRINT_L(lvl, fmt, args...) do { \
> > > +    if (MSF2_SYSREG_ERR_DEBUG >= lvl) { \
> > > +        qemu_log("%s: " fmt, __func__, ## args); \
> > > +    } \
> > > +} while (0);
> > > +
> > > +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> > > +
> > > +static void msf2_sysreg_reset(DeviceState *d)
> > > +{
> > > +    MSF2SysregState *s = MSF2_SYSREG(d);
> > > +
> > > +    DB_PRINT("RESET\n");
> > > +
> > > +    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x02420041;
> > > +    s->regs[MSSDDR_FACC1_CR] = 0x0A482124;
> > > +    s->regs[MSSDDR_PLL_STATUS] = 0x3;
> > > +}
> > > +
> > > +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
> > > +    unsigned size)
> > > +{
> > > +    MSF2SysregState *s = opaque;
> > > +    offset /= 4;
> > > +    uint32_t ret = 0;
> > > +
> > > +    if (offset < ARRAY_SIZE(s->regs)) {
> > > +        ret = s->regs[offset];
> > > +        DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx32 "\n",
> > > +                    offset * 4, ret);
> > > +    } else {
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                    "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
> > > +                    offset * 4);
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +static void msf2_sysreg_write(void *opaque, hwaddr offset,
> > > +                          uint64_t val, unsigned size)
> > > +{
> > > +    MSF2SysregState *s = (MSF2SysregState *)opaque;
> > > +    offset /= 4;
> > > +
> > > +    DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx64 "\n",
> > > +            offset * 4, val);
> > > +
> > > +    switch (offset) {
> > > +    case MSSDDR_PLL_STATUS:
> > > +        break;
> > > +
> > > +    default:
> > > +        if (offset < ARRAY_SIZE(s->regs)) {
> > > +            s->regs[offset] = val;
> >
> > I think this is pretty unsafe for the guest to continue if those registers
> > are accessed in your current implementation.
> >
> > I'd at least somehow abort for few of them (RESET, REMAP*), what do you
> > think?
>
>
> Ok. We can abort for REMAP. All the peripherals need to be released from
> reset by writing to Sysreg.
> Can we handle that case where if guest writes to Sysreg to reset SPI(say)
> then we can reset SPI
> model from here? I am very new here and I did not come across this case in
> the models I have referred.
> Please let me know if that is possible. If not possible I would simply log
> messages like released
> from reset/put in reset.

You are lucky if you didn't have to use the remap register :)
This can be implemented later if needed, don't get lost there now!

On Cold Reset the SF2 starts with the Cache Matrix mapping the CM3 code 
region (anything below 0x2000.0000) to the eNVM (which is physically 
mapped at 0x6000.0000). Now if you write to the REMAP reg you can 
instead map the eSRAM or the DDR space to the CM3 code, and the eNVM 
will only be accessible at 0x6000.0000. For the cpu the execution 
depends of the code stored at the mapped region.

If there is enough space to run your code in the eNVM, don't worry about 
modeling a remap. But now anyone can try your model, and if their 
firmware do a remap, the code flow will be completely wrong.
I think just reporting a GUEST_ERROR here is not enough since code flow 
continuing would be pretty hard to understand/debug. That's why I 
suggest to report an error and abort for now.

> Thanks,
> Sundeep
>
> > +        } else {
> > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > +                        "%s: Bad offset 0x%08" HWADDR_PRIx "\n",
> > > __func__,
> > > +                        offset * 4);
> > > +        }
> > > +        break;
> > > +    }
> > > +}
> > > +
> > > +static const MemoryRegionOps sysreg_ops = {
> > > +    .read = msf2_sysreg_read,
> > > +    .write = msf2_sysreg_write,
> > > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > > +};
> > > +
> > > +static void msf2_sysreg_init(Object *obj)
> > > +{
> > > +    MSF2SysregState *s = MSF2_SYSREG(obj);
> > > +
> > > +    memory_region_init_io(&s->iomem, obj, &sysreg_ops, s,
> > > TYPE_MSF2_SYSREG,
> > > +                          MSF2_SYSREG_MMIO_SIZE);
> > > +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_msf2_sysreg = {
> > > +    .name = TYPE_MSF2_SYSREG,
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT32_ARRAY(regs, MSF2SysregState,
> > > MSF2_SYSREG_NUM_REGS),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static void msf2_sysreg_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +
> > > +    dc->vmsd = &vmstate_msf2_sysreg;
> > > +    dc->reset = msf2_sysreg_reset;
> > > +}
> > > +
> > > +static const TypeInfo msf2_sysreg_info = {
> > > +    .name  = TYPE_MSF2_SYSREG,
> > > +    .parent = TYPE_SYS_BUS_DEVICE,
> > > +    .class_init = msf2_sysreg_class_init,
> > > +    .instance_size  = sizeof(MSF2SysregState),
> > > +    .instance_init = msf2_sysreg_init,
> > > +};
> > > +
> > > +static void msf2_sysreg_register_types(void)
> > > +{
> > > +    type_register_static(&msf2_sysreg_info);
> > > +}
> > > +
> > > +type_init(msf2_sysreg_register_types)
> > > diff --git a/include/hw/misc/msf2-sysreg.h b/include/hw/misc/msf2-sysreg.
> > > h
> > > new file mode 100644
> > > index 0000000..a485ed6
> > > --- /dev/null
> > > +++ b/include/hw/misc/msf2-sysreg.h
> > > @@ -0,0 +1,80 @@
> > > +/*
> > > + * Microsemi SmartFusion2 SYSREG
> > > + *
> > > + * Copyright (c) 2017 Subbaraya Sundeep <address@hidden>
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining
> > > a copy
> > > + * of this software and associated documentation files (the "Software"),
> > > to deal
> > > + * in the Software without restriction, including without limitation the
> > > rights
> > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > > sell
> > > + * copies of the Software, and to permit persons to whom the Software is
> > > + * furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice shall be
> > > included in
> > > + * all copies or substantial portions of the Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> > > SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > > OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > > ARISING FROM,
> > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > > DEALINGS IN
> > > + * THE SOFTWARE.
> > > + */
> > > +
> > > +#ifndef HW_MSF2_SYSREG_H
> > > +#define HW_MSF2_SYSREG_H
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "hw/sysbus.h"
> > > +#include "hw/hw.h"
> > > +#include "sysemu/sysemu.h"
> > > +#include "qemu/log.h"
> > > +
> > > +enum {
> > > +    ESRAM_CR        = 0x00 / 4,
> > > +    ESRAM_MAX_LAT,
> > > +    DDR_CR,
> > > +    ENVM_CR,
> > > +    ENVM_REMAP_BASE_CR,
> > > +    ENVM_REMAP_FAB_CR,
> > > +    CC_CR,
> > > +    CC_REGION_CR,
> > > +    CC_LOCK_BASE_ADDR_CR,
> > > +    CC_FLUSH_INDX_CR,
> > > +    DDRB_BUF_TIMER_CR,
> > > +    DDRB_NB_ADDR_CR,
> > > +    DDRB_NB_SIZE_CR,
> > > +    DDRB_CR,
> > > +
> > > +    SOFT_RESET_CR  = 0x48 / 4,
> > > +    M3_CR,
> > > +
> > > +    GPIO_SYSRESET_SEL_CR = 0x58 / 4,
> > > +
> > > +    MDDR_CR = 0x60 / 4,
> > > +
> > > +    MSSDDR_PLL_STATUS_LOW_CR = 0x90 / 4,
> > > +    MSSDDR_PLL_STATUS_HIGH_CR,
> > > +    MSSDDR_FACC1_CR,
> > > +    MSSDDR_FACC2_CR,
> > > +
> > > +    MSSDDR_PLL_STATUS = 0x150 / 4,
> > > +
> > > +};
> > > +
> > > +#define MSF2_SYSREG_MMIO_SIZE     0x300
> > > +#define MSF2_SYSREG_NUM_REGS      (MSF2_SYSREG_MMIO_SIZE / 4)
> > > +
> > > +#define TYPE_MSF2_SYSREG          "msf2-sysreg"
> > > +#define MSF2_SYSREG(obj)  OBJECT_CHECK(MSF2SysregState, (obj),
> > > TYPE_MSF2_SYSREG)
> > > +
> > > +typedef struct MSF2SysregState {
> > > +    SysBusDevice parent_obj;
> > > +
> > > +    MemoryRegion iomem;
> > > +
> > > +    uint32_t regs[MSF2_SYSREG_NUM_REGS];
> > > +} MSF2SysregState;
> > > +
> > > +#endif /* HW_MSF2_SYSREG_H */