Hi Alistair, I will remove the abort and send next iteration.
Thanks, Sundeep On Tue, Aug 1, 2017 at 11:38 AM, sundeep subbaraya <sundeep.l...@gmail.com> wrote: > Hi Philippe, > > Ping again :) > > Thanks, > Sundeep > > On Fri, Jul 21, 2017 at 2:50 PM, sundeep subbaraya <sundeep.l...@gmail.com > > wrote: > >> Hi, >> >> Ping >> >> On Thu, Jul 13, 2017 at 7:51 AM, sundeep subbaraya < >> sundeep.l...@gmail.com> wrote: >> >>> Hi Phiiippe, >>> >>> Gentle reminder. >>> >>> Thanks, >>> Sundeep >>> >>> >>> On Mon, Jul 10, 2017 at 1:55 PM, sundeep subbaraya < >>> sundeep.l...@gmail.com> wrote: >>> >>>> Hi Alistair, >>>> >>>> On Fri, Jul 7, 2017 at 10:03 PM, Alistair Francis <alistai...@gmail.com >>>> > wrote: >>>> >>>>> On Fri, Jul 7, 2017 at 12:08 AM, sundeep subbaraya >>>>> <sundeep.l...@gmail.com> wrote: >>>>> > Hi Alistair, >>>>> > >>>>> > On Wed, Jul 5, 2017 at 11:36 PM, Alistair Francis < >>>>> alistai...@gmail.com> >>>>> > wrote: >>>>> >> >>>>> >> On Sun, Jul 2, 2017 at 9:45 PM, Subbaraya Sundeep >>>>> >> <sundeep.l...@gmail.com> wrote: >>>>> >> > Added Sytem register block of Smartfusion2. >>>>> >> > This block has PLL registers which are accessed by guest. >>>>> >> > >>>>> >> > Signed-off-by: Subbaraya Sundeep <sundeep.l...@gmail.com> >>>>> >> > --- >>>>> >> > 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 <sundeep.l...@gmail.com> >>>>> >> > + * >>>>> >> > + * 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. >>>>> >>>> >>>> >>> >> >