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

[sundeep subbaraya]

Hi Alistair,

On Fri, Jul 7, 2017 at 10:03 PM, Alistair Francis <address@hidden> wrote:

On Fri, Jul 7, 2017 at 12:08 AM, sundeep subbaraya
<address@hidden> wrote:
> Hi Alistair,
>
> On Wed, Jul 5, 2017 at 11:36 PM, Alistair Francis <address@hidden>
> wrote:
>>
>> On Sun, Jul 2, 2017 at 9:45 PM, Subbaraya Sundeep
>> <address@hidden> 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         | 200
>> > ++++++++++++++++++++++++++++++++++++++++++
>> >  include/hw/misc/msf2-sysreg.h |  82 +++++++++++++++++
>> >  3 files changed, 283 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..64ee141
>> > --- /dev/null
>> > +++ b/hw/misc/msf2-sysreg.c
>> > @@ -0,0 +1,200 @@
>> > +/*
>> > + * 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"
>>
>> Same #include comment from patch 1.
>
>
> Ok will change.
>>
>>
>> > +
>> > +#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 "\n", __func__, ## args); \
>> > +    } \
>> > +} while (0);
>> > +
>> > +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>> > +
>> > +static inline int msf2_divbits(uint32_t div)
>> > +{
>> > +    int ret = 0;
>> > +
>> > +    switch (div) {
>> > +    case 1:
>> > +        ret = 0;
>> > +        break;
>> > +    case 2:
>> > +        ret = 1;
>> > +        break;
>> > +    case 4:
>> > +        ret = 2;
>> > +        break;
>> > +    case 8:
>> > +        ret = 4;
>> > +        break;
>> > +    case 16:
>> > +        ret = 5;
>> > +        break;
>> > +    case 32:
>> > +        ret = 6;
>> > +        break;
>> > +    default:
>> > +        break;
>> > +    }
>> > +
>> > +    return ret;
>> > +}
>> > +
>> > +static void msf2_sysreg_reset(DeviceState *d)
>> > +{
>> > +    MSF2SysregState *s = MSF2_SYSREG(d);
>> > +
>> > +    DB_PRINT("RESET");
>> > +
>> > +    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
>> > +    s->regs[MSSDDR_PLL_STATUS] = 0x3;
>> > +    s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
>> > +                               msf2_divbits(s->apb1div) << 2;
>> > +}
>> > +
>> > +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
>> > +    unsigned size)
>> > +{
>> > +    MSF2SysregState *s = opaque;
>> > +    offset /= 4;
>>
>> Probably best to use a bitshift.
>
>
> Ok will change.
>>
>>
>> > +    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,
>> > +                    offset * 4, ret);
>>
>> Bitshift here as well.
>
>
> Ok will change.
>>
>>
>> > +    } 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;
>> > +    uint32_t newval = val;
>> > +    uint32_t oldval;
>> > +
>> > +    DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx64,
>> > +            offset, val);
>> > +
>> > +    offset /= 4;
>>
>> Same here
>
>
> Ok will change
>>
>>
>> > +
>> > +    switch (offset) {
>> > +    case MSSDDR_PLL_STATUS:
>> > +        break;
>> > +
>> > +    case ESRAM_CR:
>> > +        oldval = s->regs[ESRAM_CR];
>> > +        if (oldval ^ newval) {
>> > +            qemu_log_mask(LOG_GUEST_ERROR,
>> > +                       TYPE_MSF2_SYSREG": eSRAM remapping not
>> > supported\n");
>> > +            abort();
>>
>> The guest should not be able to kill QEMU, a guest error should never
>> result in an abort.
>
>
> Philippe suggested to abort because:
> If guest tries to remap since firmware do a remap, the code flow will be
> completely wrong.
> Reporting a GUEST_ERROR here is not enough since code flow continuing would
> be
> pretty hard to understand/debug.

I don't see how it will be that hard to debug as QEMU will tell you
that the guest is doing something wrong.

You can't allow the guest to abort or exit QEMU. It's a security
liability issue and specifically mentioned as not allowed:
https://github.com/qemu/qemu/blob/master/HACKING#L230

Ok. Lets hear from Philippe. Philippe?

Thanks,

Sundeep
 

Thanks,
Alistair

> We decided to abort for now.