[PATCH v2 0/9] RISC-V Add the OpenTitan Machine

[PATCH v2 0/9] RISC-V Add the OpenTitan Machine

[Alistair Francis]

OpenTitan is an open source silicon Root of Trust (RoT) project. This
series adds initial support for the OpenTitan machine to QEMU.

This series add the Ibex CPU to the QEMU RISC-V target. It then adds the
OpenTitan machine, the Ibex UART and the Ibex PLIC.

The UART has been tested sending and receiving data.

With this series QEMU can boot the OpenTitan ROM, Tock OS and a Tock
userspace app.

The Ibex PLIC is similar to the RISC-V PLIC (and is based on the QEMU
implementation) with some differences. The hope is that the Ibex PLIC
will converge to follow the RISC-V spec. As that happens I want to
update the QEMU Ibex PLIC and hopefully eventually replace the current
PLIC as the implementation is a little overlay complex.

For more details on OpenTitan, see here: https://docs.opentitan.org/

v2:
 - Rebase on master
 - Get uart receive working

Alistair Francis (9):
  riscv/boot: Add a missing header include
  target/riscv: Don't overwrite the reset vector
  target/riscv: Add the lowRISC Ibex CPU
  riscv: Initial commit of OpenTitan machine
  hw/char: Initial commit of Ibex UART
  hw/intc: Initial commit of lowRISC Ibex PLIC
  riscv/opentitan: Connect the PLIC device
  riscv/opentitan: Connect the UART device
  target/riscv: Use a smaller guess size for no-MMU PMP

 MAINTAINERS                         |  14 +
 default-configs/riscv32-softmmu.mak |   1 +
 default-configs/riscv64-softmmu.mak |  11 +-
 hw/char/Makefile.objs               |   1 +
 hw/char/ibex_uart.c                 | 490 ++++++++++++++++++++++++++++
 hw/intc/Makefile.objs               |   1 +
 hw/intc/ibex_plic.c                 | 261 +++++++++++++++
 hw/riscv/Kconfig                    |   9 +
 hw/riscv/Makefile.objs              |   1 +
 hw/riscv/opentitan.c                | 204 ++++++++++++
 include/hw/char/ibex_uart.h         | 110 +++++++
 include/hw/intc/ibex_plic.h         |  63 ++++
 include/hw/riscv/boot.h             |   1 +
 include/hw/riscv/opentitan.h        |  79 +++++
 target/riscv/cpu.c                  |  30 +-
 target/riscv/cpu.h                  |   1 +
 target/riscv/pmp.c                  |  19 +-
 17 files changed, 1281 insertions(+), 15 deletions(-)
 create mode 100644 hw/char/ibex_uart.c
 create mode 100644 hw/intc/ibex_plic.c
 create mode 100644 hw/riscv/opentitan.c
 create mode 100644 include/hw/char/ibex_uart.h
 create mode 100644 include/hw/intc/ibex_plic.h
 create mode 100644 include/hw/riscv/opentitan.h

-- 
2.26.2

[PATCH v2 1/9] riscv/boot: Add a missing header include

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/hw/riscv/boot.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 474a940ad5..9daa98da08 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -21,6 +21,7 @@
 #define RISCV_BOOT_H
 
 #include "exec/cpu-defs.h"
+#include "hw/loader.h"
 
 void riscv_find_and_load_firmware(MachineState *machine,
                                   const char *default_machine_firmware,
-- 
2.26.2

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

[Bin Meng]

On Fri, May 8, 2020 at 3:24 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  include/hw/riscv/boot.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 474a940ad5..9daa98da08 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -21,6 +21,7 @@
>  #define RISCV_BOOT_H
>
>  #include "exec/cpu-defs.h"
> +#include "hw/loader.h"

Why is this needed? Currently this does not break build.

>
>  void riscv_find_and_load_firmware(MachineState *machine,
>                                    const char *default_machine_firmware,

Regards,
Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

[Alistair Francis]

On Thu, May 14, 2020 at 8:34 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, May 8, 2020 at 3:24 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  include/hw/riscv/boot.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 474a940ad5..9daa98da08 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -21,6 +21,7 @@
> >  #define RISCV_BOOT_H
> >
> >  #include "exec/cpu-defs.h"
> > +#include "hw/loader.h"
>
> Why is this needed? Currently this does not break build.

Currently every c file that includes boot.h also includes loader.h
before it. Which is why the build works fine. We should be able to
include just boot.h though so this is a small fixup to allow that.

Alistair

>
> >
> >  void riscv_find_and_load_firmware(MachineState *machine,
> >                                    const char *default_machine_firmware,
>
> Regards,
> Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

[Bin Meng]

On Thu, May 14, 2020 at 11:38 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 8:34 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, May 8, 2020 at 3:24 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  include/hw/riscv/boot.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > > index 474a940ad5..9daa98da08 100644
> > > --- a/include/hw/riscv/boot.h
> > > +++ b/include/hw/riscv/boot.h
> > > @@ -21,6 +21,7 @@
> > >  #define RISCV_BOOT_H
> > >
> > >  #include "exec/cpu-defs.h"
> > > +#include "hw/loader.h"
> >
> > Why is this needed? Currently this does not break build.
>
> Currently every c file that includes boot.h also includes loader.h
> before it. Which is why the build works fine. We should be able to
> include just boot.h though so this is a small fixup to allow that.
>

I wonder if this is a required convention to make the header file
self-contained? The only thing that is offending seems to be the
symbol_fn_t typedef.

Regards,
Bin

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

[Philippe Mathieu-Daudé]

On 5/14/20 5:46 PM, Bin Meng wrote:
> On Thu, May 14, 2020 at 11:38 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Thu, May 14, 2020 at 8:34 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Fri, May 8, 2020 at 3:24 AM Alistair Francis
>>> <alistair.francis@wdc.com> wrote:
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>> ---
>>>>   include/hw/riscv/boot.h | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>>>> index 474a940ad5..9daa98da08 100644
>>>> --- a/include/hw/riscv/boot.h
>>>> +++ b/include/hw/riscv/boot.h
>>>> @@ -21,6 +21,7 @@
>>>>   #define RISCV_BOOT_H
>>>>
>>>>   #include "exec/cpu-defs.h"
>>>> +#include "hw/loader.h"
>>>
>>> Why is this needed? Currently this does not break build.
>>
>> Currently every c file that includes boot.h also includes loader.h
>> before it. Which is why the build works fine. We should be able to
>> include just boot.h though so this is a small fixup to allow that.
>>
> 
> I wonder if this is a required convention to make the header file
> self-contained? The only thing that is offending seems to be the
> symbol_fn_t typedef.

Indeed the use of the symbol_fn_t typedef justifies including its 
declaration :)

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

> 
> Regards,
> Bin
> 

Re: [PATCH v2 1/9] riscv/boot: Add a missing header include

[Bin Meng]

Hi Alistair,

On Thu, May 14, 2020 at 11:38 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 8:34 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, May 8, 2020 at 3:24 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  include/hw/riscv/boot.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > > index 474a940ad5..9daa98da08 100644
> > > --- a/include/hw/riscv/boot.h
> > > +++ b/include/hw/riscv/boot.h
> > > @@ -21,6 +21,7 @@
> > >  #define RISCV_BOOT_H
> > >
> > >  #include "exec/cpu-defs.h"
> > > +#include "hw/loader.h"
> >
> > Why is this needed? Currently this does not break build.
>
> Currently every c file that includes boot.h also includes loader.h
> before it. Which is why the build works fine. We should be able to
> include just boot.h though so this is a small fixup to allow that.
>

Please include such in the commit message to help people understand.

Reviewed-by: Bin Meng <bin.meng@windriver.com>

Regards,
Bin

[PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Alistair Francis]

If the reset vector is set in the init function don't set it again in
realise.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 059d71f2c7..8f837edf8d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
     env->features |= (1ULL << feature);
 }
 
+static int get_resetvec(CPURISCVState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    return env->resetvec;
+#endif
+    return 0;
+}
+
 static void set_resetvec(CPURISCVState *env, int resetvec)
 {
 #ifndef CONFIG_USER_ONLY
@@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_11_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
 }
 
 #if defined(TARGET_RISCV32)
@@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_09_1);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_09_1);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     set_priv_version(env, priv_version);
-    set_resetvec(env, DEFAULT_RSTVEC);
+    if (!get_resetvec(env)) {
+        set_resetvec(env, DEFAULT_RSTVEC);
+    }
 
     if (cpu->cfg.mmu) {
         set_feature(env, RISCV_FEATURE_MMU);
-- 
2.26.2

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Philippe Mathieu-Daudé]

On 5/7/20 9:13 PM, Alistair Francis wrote:
> If the reset vector is set in the init function don't set it again in
> realise.

typo "realize".

> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 059d71f2c7..8f837edf8d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
>       env->features |= (1ULL << feature);
>   }
>   
> +static int get_resetvec(CPURISCVState *env)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    return env->resetvec;
> +#endif
> +    return 0;

Don't you get an error about double return? Maybe use #else?

> +}
> +
>   static void set_resetvec(CPURISCVState *env, int resetvec)
>   {
>   #ifndef CONFIG_USER_ONLY
> @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_11_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>   }
>   
>   #if defined(TARGET_RISCV32)
> @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_09_1);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_09_1);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>       }
>   
>       set_priv_version(env, priv_version);
> -    set_resetvec(env, DEFAULT_RSTVEC);
> +    if (!get_resetvec(env)) {
> +        set_resetvec(env, DEFAULT_RSTVEC);
> +    }
>   
>       if (cpu->cfg.mmu) {
>           set_feature(env, RISCV_FEATURE_MMU);
> 

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Alistair Francis]

On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 5/7/20 9:13 PM, Alistair Francis wrote:
> > If the reset vector is set in the init function don't set it again in
> > realise.
>
> typo "realize".

It's not a typo, just correct English :)

I have changed it.

>
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   target/riscv/cpu.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 059d71f2c7..8f837edf8d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> >       env->features |= (1ULL << feature);
> >   }
> >
> > +static int get_resetvec(CPURISCVState *env)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    return env->resetvec;
> > +#endif
> > +    return 0;
>
> Don't you get an error about double return? Maybe use #else?

Apparently not, I have changed it though.

Alistair

>
> > +}
> > +
> >   static void set_resetvec(CPURISCVState *env, int resetvec)
> >   {
> >   #ifndef CONFIG_USER_ONLY
> > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >   }
> >
> >   #if defined(TARGET_RISCV32)
> > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >       }
> >
> >       set_priv_version(env, priv_version);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> > +    if (!get_resetvec(env)) {
> > +        set_resetvec(env, DEFAULT_RSTVEC);
> > +    }
> >
> >       if (cpu->cfg.mmu) {
> >           set_feature(env, RISCV_FEATURE_MMU);
> >
>

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Bin Meng]

On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > If the reset vector is set in the init function don't set it again in
> > > realise.
> >
> > typo "realize".
>
> It's not a typo, just correct English :)
>
> I have changed it.
>
> >
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >   target/riscv/cpu.c | 20 +++++++++++---------
> > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 059d71f2c7..8f837edf8d 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > >       env->features |= (1ULL << feature);
> > >   }
> > >
> > > +static int get_resetvec(CPURISCVState *env)
> > > +{
> > > +#ifndef CONFIG_USER_ONLY
> > > +    return env->resetvec;
> > > +#endif
> > > +    return 0;
> >
> > Don't you get an error about double return? Maybe use #else?
>
> Apparently not, I have changed it though.
>
> Alistair
>
> >
> > > +}
> > > +
> > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > >   {
> > >   #ifndef CONFIG_USER_ONLY
> > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >   }
> > >
> > >   #if defined(TARGET_RISCV32)
> > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > >       }
> > >
> > >       set_priv_version(env, priv_version);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > +    if (!get_resetvec(env)) {

What if we have a RISC-V CPU whose reset vector is at address 0?

> > > +        set_resetvec(env, DEFAULT_RSTVEC);
> > > +    }
> > >
> > >       if (cpu->cfg.mmu) {
> > >           set_feature(env, RISCV_FEATURE_MMU);
> > >

Regards,
Bin

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Alistair Francis]

On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com> wrote:
> > >
> > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > If the reset vector is set in the init function don't set it again in
> > > > realise.
> > >
> > > typo "realize".
> >
> > It's not a typo, just correct English :)
> >
> > I have changed it.
> >
> > >
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index 059d71f2c7..8f837edf8d 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > >       env->features |= (1ULL << feature);
> > > >   }
> > > >
> > > > +static int get_resetvec(CPURISCVState *env)
> > > > +{
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +    return env->resetvec;
> > > > +#endif
> > > > +    return 0;
> > >
> > > Don't you get an error about double return? Maybe use #else?
> >
> > Apparently not, I have changed it though.
> >
> > Alistair
> >
> > >
> > > > +}
> > > > +
> > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > >   {
> > > >   #ifndef CONFIG_USER_ONLY
> > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >   }
> > > >
> > > >   #if defined(TARGET_RISCV32)
> > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > >       }
> > > >
> > > >       set_priv_version(env, priv_version);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > +    if (!get_resetvec(env)) {
>
> What if we have a RISC-V CPU whose reset vector is at address 0?

That won't work then. I think if that happens we could swap to a
negative number.

Alistair

>
> > > > +        set_resetvec(env, DEFAULT_RSTVEC);
> > > > +    }
> > > >
> > > >       if (cpu->cfg.mmu) {
> > > >           set_feature(env, RISCV_FEATURE_MMU);
> > > >
>
> Regards,
> Bin

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Bin Meng]

On Sat, May 16, 2020 at 3:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > > <philmd@redhat.com> wrote:
> > > >
> > > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > > If the reset vector is set in the init function don't set it again in
> > > > > realise.
> > > >
> > > > typo "realize".
> > >
> > > It's not a typo, just correct English :)
> > >
> > > I have changed it.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index 059d71f2c7..8f837edf8d 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > > >       env->features |= (1ULL << feature);
> > > > >   }
> > > > >
> > > > > +static int get_resetvec(CPURISCVState *env)
> > > > > +{
> > > > > +#ifndef CONFIG_USER_ONLY
> > > > > +    return env->resetvec;
> > > > > +#endif
> > > > > +    return 0;
> > > >
> > > > Don't you get an error about double return? Maybe use #else?
> > >
> > > Apparently not, I have changed it though.
> > >
> > > Alistair
> > >
> > > >
> > > > > +}
> > > > > +
> > > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > > >   {
> > > > >   #ifndef CONFIG_USER_ONLY
> > > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >   }
> > > > >
> > > > >   #if defined(TARGET_RISCV32)
> > > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > >       }
> > > > >
> > > > >       set_priv_version(env, priv_version);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > +    if (!get_resetvec(env)) {
> >
> > What if we have a RISC-V CPU whose reset vector is at address 0?
>
> That won't work then. I think if that happens we could swap to a
> negative number.
>

env->resetvec is declared as target_ulong so negative number does not work here.

How about just remove set_resetvec() in riscv_cpu_realize()? Or
introduce a new config parameter for the reset vector, and only
override it if the config parameter is given, like other properties?

Regards,
Bin

Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

[Alistair Francis]

On Sat, May 16, 2020 at 2:03 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, May 16, 2020 at 3:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > > > <philmd@redhat.com> wrote:
> > > > >
> > > > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > > > If the reset vector is set in the init function don't set it again in
> > > > > > realise.
> > > > >
> > > > > typo "realize".
> > > >
> > > > It's not a typo, just correct English :)
> > > >
> > > > I have changed it.
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > > index 059d71f2c7..8f837edf8d 100644
> > > > > > --- a/target/riscv/cpu.c
> > > > > > +++ b/target/riscv/cpu.c
> > > > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > > > >       env->features |= (1ULL << feature);
> > > > > >   }
> > > > > >
> > > > > > +static int get_resetvec(CPURISCVState *env)
> > > > > > +{
> > > > > > +#ifndef CONFIG_USER_ONLY
> > > > > > +    return env->resetvec;
> > > > > > +#endif
> > > > > > +    return 0;
> > > > >
> > > > > Don't you get an error about double return? Maybe use #else?
> > > >
> > > > Apparently not, I have changed it though.
> > > >
> > > > Alistair
> > > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > > > >   {
> > > > > >   #ifndef CONFIG_USER_ONLY
> > > > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >   }
> > > > > >
> > > > > >   #if defined(TARGET_RISCV32)
> > > > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > > >       }
> > > > > >
> > > > > >       set_priv_version(env, priv_version);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > > +    if (!get_resetvec(env)) {
> > >
> > > What if we have a RISC-V CPU whose reset vector is at address 0?
> >
> > That won't work then. I think if that happens we could swap to a
> > negative number.
> >
>
> env->resetvec is declared as target_ulong so negative number does not work here.
>
> How about just remove set_resetvec() in riscv_cpu_realize()? Or
> introduce a new config parameter for the reset vector, and only
> override it if the config parameter is given, like other properties?

We need to set the default reset veector though. I can just set all of
the values in the init function instead.

Alistair

>
> Regards,
> Bin

[PATCH v2 3/9] target/riscv: Add the lowRISC Ibex CPU

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 10 ++++++++++
 target/riscv/cpu.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8f837edf8d..235101f685 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -160,6 +160,15 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
+static void rv32imcu_nommu_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    set_misa(env, RV32 | RVI | RVM | RVC | RVU);
+    set_priv_version(env, PRIV_VERSION_1_10_0);
+    set_resetvec(env, 0x8088);
+    set_feature(env, RISCV_FEATURE_PMP);
+}
+
 static void rv32imacu_nommu_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -620,6 +629,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
     DEFINE_CPU(TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init),
 #if defined(TARGET_RISCV32)
     DEFINE_CPU(TYPE_RISCV_CPU_BASE32,           riscv_base32_cpu_init),
+    DEFINE_CPU(TYPE_RISCV_CPU_IBEX,             rv32imcu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,       rv32imafcu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d0e7f5b9c5..8733d7467f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -35,6 +35,7 @@
 #define TYPE_RISCV_CPU_ANY              RISCV_CPU_TYPE_NAME("any")
 #define TYPE_RISCV_CPU_BASE32           RISCV_CPU_TYPE_NAME("rv32")
 #define TYPE_RISCV_CPU_BASE64           RISCV_CPU_TYPE_NAME("rv64")
+#define TYPE_RISCV_CPU_IBEX             RISCV_CPU_TYPE_NAME("lowrisc-ibex")
 #define TYPE_RISCV_CPU_SIFIVE_E31       RISCV_CPU_TYPE_NAME("sifive-e31")
 #define TYPE_RISCV_CPU_SIFIVE_E34       RISCV_CPU_TYPE_NAME("sifive-e34")
 #define TYPE_RISCV_CPU_SIFIVE_E51       RISCV_CPU_TYPE_NAME("sifive-e51")
-- 
2.26.2

Re: [PATCH v2 3/9] target/riscv: Add the lowRISC Ibex CPU

[Bin Meng]

On Fri, May 8, 2020 at 3:23 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>

Please include some commit message to have a brief introduction of this new CPU.

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 10 ++++++++++
>  target/riscv/cpu.h |  1 +
>  2 files changed, 11 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8f837edf8d..235101f685 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -160,6 +160,15 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
>      set_feature(env, RISCV_FEATURE_PMP);
>  }
>
> +static void rv32imcu_nommu_cpu_init(Object *obj)
> +{
> +    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    set_misa(env, RV32 | RVI | RVM | RVC | RVU);
> +    set_priv_version(env, PRIV_VERSION_1_10_0);
> +    set_resetvec(env, 0x8088);
> +    set_feature(env, RISCV_FEATURE_PMP);
> +}
> +
>  static void rv32imacu_nommu_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -620,6 +629,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>      DEFINE_CPU(TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init),
>  #if defined(TARGET_RISCV32)
>      DEFINE_CPU(TYPE_RISCV_CPU_BASE32,           riscv_base32_cpu_init),
> +    DEFINE_CPU(TYPE_RISCV_CPU_IBEX,             rv32imcu_nommu_cpu_init),
>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32imacu_nommu_cpu_init),
>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,       rv32imafcu_nommu_cpu_init),
>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32gcsu_priv1_10_0_cpu_init),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index d0e7f5b9c5..8733d7467f 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -35,6 +35,7 @@
>  #define TYPE_RISCV_CPU_ANY              RISCV_CPU_TYPE_NAME("any")
>  #define TYPE_RISCV_CPU_BASE32           RISCV_CPU_TYPE_NAME("rv32")
>  #define TYPE_RISCV_CPU_BASE64           RISCV_CPU_TYPE_NAME("rv64")
> +#define TYPE_RISCV_CPU_IBEX             RISCV_CPU_TYPE_NAME("lowrisc-ibex")
>  #define TYPE_RISCV_CPU_SIFIVE_E31       RISCV_CPU_TYPE_NAME("sifive-e31")
>  #define TYPE_RISCV_CPU_SIFIVE_E34       RISCV_CPU_TYPE_NAME("sifive-e34")
>  #define TYPE_RISCV_CPU_SIFIVE_E51       RISCV_CPU_TYPE_NAME("sifive-e51")
> --

Otherwise, looks good to me.
Reviewed-by: Bin Meng <bin.meng@windriver.com>

Regards,
Bin

[PATCH v2 4/9] riscv: Initial commit of OpenTitan machine

[Alistair Francis]

This adds a barebone OpenTitan machine to QEMU.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 MAINTAINERS                         |  10 ++
 default-configs/riscv32-softmmu.mak |   1 +
 default-configs/riscv64-softmmu.mak |  11 +-
 hw/riscv/Kconfig                    |   5 +
 hw/riscv/Makefile.objs              |   1 +
 hw/riscv/opentitan.c                | 169 ++++++++++++++++++++++++++++
 include/hw/riscv/opentitan.h        |  63 +++++++++++
 7 files changed, 259 insertions(+), 1 deletion(-)
 create mode 100644 hw/riscv/opentitan.c
 create mode 100644 include/hw/riscv/opentitan.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1f84e3ae2c..c3d77f0861 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1229,6 +1229,16 @@ F: pc-bios/canyonlands.dt[sb]
 F: pc-bios/u-boot-sam460ex-20100605.bin
 F: roms/u-boot-sam460ex
 
+RISC-V Machines
+-----------------
+OpenTitan
+M: Alistair Francis <Alistair.Francis@wdc.com>
+L: qemu-riscv@nongnu.org
+S: Supported
+F: hw/riscv/opentitan.c
+F: include/hw/riscv/opentitan.h
+
+
 SH4 Machines
 ------------
 R2D
diff --git a/default-configs/riscv32-softmmu.mak b/default-configs/riscv32-softmmu.mak
index 1ae077ed87..94a236c9c2 100644
--- a/default-configs/riscv32-softmmu.mak
+++ b/default-configs/riscv32-softmmu.mak
@@ -10,3 +10,4 @@ CONFIG_SPIKE=y
 CONFIG_SIFIVE_E=y
 CONFIG_SIFIVE_U=y
 CONFIG_RISCV_VIRT=y
+CONFIG_OPENTITAN=y
diff --git a/default-configs/riscv64-softmmu.mak b/default-configs/riscv64-softmmu.mak
index 235c6f473f..aaf6d735bb 100644
--- a/default-configs/riscv64-softmmu.mak
+++ b/default-configs/riscv64-softmmu.mak
@@ -1,3 +1,12 @@
 # Default configuration for riscv64-softmmu
 
-include riscv32-softmmu.mak
+# Uncomment the following lines to disable these optional devices:
+#
+#CONFIG_PCI_DEVICES=n
+
+# Boards:
+#
+CONFIG_SPIKE=y
+CONFIG_SIFIVE_E=y
+CONFIG_SIFIVE_U=y
+CONFIG_RISCV_VIRT=y
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index ff9fbe958a..94d19571f7 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -27,6 +27,11 @@ config SPIKE
     select HTIF
     select SIFIVE
 
+config OPENTITAN
+    bool
+    select HART
+    select UNIMP
+
 config RISCV_VIRT
     bool
     imply PCI_DEVICES
diff --git a/hw/riscv/Makefile.objs b/hw/riscv/Makefile.objs
index fc3c6dd7c8..57cc708f5d 100644
--- a/hw/riscv/Makefile.objs
+++ b/hw/riscv/Makefile.objs
@@ -1,6 +1,7 @@
 obj-y += boot.o
 obj-$(CONFIG_SPIKE) += riscv_htif.o
 obj-$(CONFIG_HART) += riscv_hart.o
+obj-$(CONFIG_OPENTITAN) += opentitan.o
 obj-$(CONFIG_SIFIVE_E) += sifive_e.o
 obj-$(CONFIG_SIFIVE_E) += sifive_e_prci.o
 obj-$(CONFIG_SIFIVE) += sifive_clint.o
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
new file mode 100644
index 0000000000..c00f0720ab
--- /dev/null
+++ b/hw/riscv/opentitan.c
@@ -0,0 +1,169 @@
+/*
+ * QEMU RISC-V Board Compatible with OpenTitan FPGA platform
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * Provides a board compatible with the OpenTitan FPGA platform:
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 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 "qemu/osdep.h"
+#include "hw/riscv/opentitan.h"
+#include "qapi/error.h"
+#include "hw/boards.h"
+#include "hw/misc/unimp.h"
+#include "hw/riscv/boot.h"
+#include "exec/address-spaces.h"
+
+static const struct MemmapEntry {
+    hwaddr base;
+    hwaddr size;
+} ibex_memmap[] = {
+    [IBEX_ROM] =            {  0x00008000,   0xc000 },
+    [IBEX_RAM] =            {  0x10000000,  0x10000 },
+    [IBEX_FLASH] =          {  0x20000000,  0x80000 },
+    [IBEX_UART] =           {  0x40000000,  0x10000 },
+    [IBEX_GPIO] =           {  0x40010000,  0x10000 },
+    [IBEX_SPI] =            {  0x40020000,  0x10000 },
+    [IBEX_FLASH_CTRL] =     {  0x40030000,  0x10000 },
+    [IBEX_PINMUX] =         {  0x40070000,  0x10000 },
+    [IBEX_RV_TIMER] =       {  0x40080000,  0x10000 },
+    [IBEX_PLIC] =           {  0x40090000,  0x10000 },
+    [IBEX_AES] =            {  0x40110000,  0x10000 },
+    [IBEX_HMAC] =           {  0x40120000,  0x10000 },
+    [IBEX_ALERT_HANDLER] =  {  0x40130000,  0x10000 },
+    [IBEX_USBDEV] =         {  0x40150000,  0x10000 }
+};
+
+static void riscv_opentitan_init(MachineState *machine)
+{
+    const struct MemmapEntry *memmap = ibex_memmap;
+    OpenTitanState *s = g_new0(OpenTitanState, 1);
+    MemoryRegion *sys_mem = get_system_memory();
+    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
+
+    /* Initialize SoC */
+    object_initialize_child(OBJECT(machine), "soc", &s->soc,
+                            sizeof(s->soc), TYPE_RISCV_IBEX_SOC,
+                            &error_abort, NULL);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized",
+                            &error_abort);
+
+    memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
+        memmap[IBEX_RAM].size, &error_fatal);
+    memory_region_add_subregion(sys_mem,
+        memmap[IBEX_RAM].base, main_mem);
+
+
+    if (machine->firmware) {
+        riscv_load_firmware(machine->firmware, memmap[IBEX_RAM].base, NULL);
+    }
+
+    if (machine->kernel_filename) {
+        riscv_load_kernel(machine->kernel_filename, NULL);
+    }
+}
+
+static void riscv_opentitan_machine_init(MachineClass *mc)
+{
+    mc->desc = "RISC-V Board compatible with OpenTitan";
+    mc->init = riscv_opentitan_init;
+    mc->max_cpus = 1;
+    mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
+}
+
+DEFINE_MACHINE("opentitan", riscv_opentitan_machine_init)
+
+static void riscv_lowrisc_ibex_soc_init(Object *obj)
+{
+    LowRISCIbexSoCState *s = RISCV_IBEX_SOC(obj);
+
+    object_initialize_child(obj, "cpus", &s->cpus,
+                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
+                            &error_abort, NULL);
+}
+
+static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
+{
+    const struct MemmapEntry *memmap = ibex_memmap;
+    MachineState *ms = MACHINE(qdev_get_machine());
+    LowRISCIbexSoCState *s = RISCV_IBEX_SOC(dev_soc);
+    MemoryRegion *sys_mem = get_system_memory();
+
+    object_property_set_str(OBJECT(&s->cpus), ms->cpu_type, "cpu-type",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
+                            &error_abort);
+    object_property_set_bool(OBJECT(&s->cpus), true, "realized",
+                            &error_abort);
+
+    /* Boot ROM */
+    memory_region_init_rom(&s->rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom",
+                           memmap[IBEX_ROM].size, &error_fatal);
+    memory_region_add_subregion(sys_mem,
+        memmap[IBEX_ROM].base, &s->rom);
+
+    /* Flash memory */
+    memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash",
+                           memmap[IBEX_FLASH].size, &error_fatal);
+    memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base,
+                                &s->flash_mem);
+
+    create_unimplemented_device("riscv.lowrisc.ibex.uart",
+        memmap[IBEX_UART].base, memmap[IBEX_UART].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.gpio",
+        memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.spi",
+        memmap[IBEX_SPI].base, memmap[IBEX_SPI].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl",
+        memmap[IBEX_FLASH_CTRL].base, memmap[IBEX_FLASH_CTRL].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.rv_timer",
+        memmap[IBEX_RV_TIMER].base, memmap[IBEX_RV_TIMER].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.aes",
+        memmap[IBEX_AES].base, memmap[IBEX_AES].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.hmac",
+        memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.plic",
+        memmap[IBEX_PLIC].base, memmap[IBEX_PLIC].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
+        memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.alert_handler",
+        memmap[IBEX_ALERT_HANDLER].base, memmap[IBEX_ALERT_HANDLER].size);
+    create_unimplemented_device("riscv.lowrisc.ibex.USBDEV",
+        memmap[IBEX_USBDEV].base, memmap[IBEX_USBDEV].size);
+}
+
+static void riscv_lowrisc_ibex_soc_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = riscv_lowrisc_ibex_soc_realize;
+    /* Reason: Uses serial_hds in realize function, thus can't be used twice */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo riscv_lowrisc_ibex_soc_type_info = {
+    .name = TYPE_RISCV_IBEX_SOC,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(LowRISCIbexSoCState),
+    .instance_init = riscv_lowrisc_ibex_soc_init,
+    .class_init = riscv_lowrisc_ibex_soc_class_init,
+};
+
+static void riscv_lowrisc_ibex_soc_register_types(void)
+{
+    type_register_static(&riscv_lowrisc_ibex_soc_type_info);
+}
+
+type_init(riscv_lowrisc_ibex_soc_register_types)
diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
new file mode 100644
index 0000000000..15a3d87ed0
--- /dev/null
+++ b/include/hw/riscv/opentitan.h
@@ -0,0 +1,63 @@
+/*
+ * QEMU RISC-V Board Compatible with OpenTitan FPGA platform
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_OPENTITAN_H
+#define HW_OPENTITAN_H
+
+#include "hw/riscv/riscv_hart.h"
+
+#define TYPE_RISCV_IBEX_SOC "riscv.lowrisc.ibex.soc"
+#define RISCV_IBEX_SOC(obj) \
+    OBJECT_CHECK(LowRISCIbexSoCState, (obj), TYPE_RISCV_IBEX_SOC)
+
+typedef struct LowRISCIbexSoCState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    RISCVHartArrayState cpus;
+    MemoryRegion flash_mem;
+    MemoryRegion rom;
+} LowRISCIbexSoCState;
+
+typedef struct OpenTitanState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    LowRISCIbexSoCState soc;
+} OpenTitanState;
+
+enum {
+    IBEX_ROM,
+    IBEX_RAM,
+    IBEX_FLASH,
+    IBEX_UART,
+    IBEX_GPIO,
+    IBEX_SPI,
+    IBEX_FLASH_CTRL,
+    IBEX_RV_TIMER,
+    IBEX_AES,
+    IBEX_HMAC,
+    IBEX_PLIC,
+    IBEX_PINMUX,
+    IBEX_ALERT_HANDLER,
+    IBEX_USBDEV,
+};
+
+#endif
-- 
2.26.2

Re: [PATCH v2 4/9] riscv: Initial commit of OpenTitan machine

[Bin Meng]

On Fri, May 8, 2020 at 3:26 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> This adds a barebone OpenTitan machine to QEMU.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  MAINTAINERS                         |  10 ++
>  default-configs/riscv32-softmmu.mak |   1 +
>  default-configs/riscv64-softmmu.mak |  11 +-
>  hw/riscv/Kconfig                    |   5 +
>  hw/riscv/Makefile.objs              |   1 +
>  hw/riscv/opentitan.c                | 169 ++++++++++++++++++++++++++++
>  include/hw/riscv/opentitan.h        |  63 +++++++++++
>  7 files changed, 259 insertions(+), 1 deletion(-)
>  create mode 100644 hw/riscv/opentitan.c
>  create mode 100644 include/hw/riscv/opentitan.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1f84e3ae2c..c3d77f0861 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1229,6 +1229,16 @@ F: pc-bios/canyonlands.dt[sb]
>  F: pc-bios/u-boot-sam460ex-20100605.bin
>  F: roms/u-boot-sam460ex
>
> +RISC-V Machines
> +-----------------

nits: please make --- end at an aligned place to the line before

> +OpenTitan
> +M: Alistair Francis <Alistair.Francis@wdc.com>
> +L: qemu-riscv@nongnu.org
> +S: Supported
> +F: hw/riscv/opentitan.c
> +F: include/hw/riscv/opentitan.h
> +
> +
>  SH4 Machines
>  ------------
>  R2D
> diff --git a/default-configs/riscv32-softmmu.mak b/default-configs/riscv32-softmmu.mak
> index 1ae077ed87..94a236c9c2 100644
> --- a/default-configs/riscv32-softmmu.mak
> +++ b/default-configs/riscv32-softmmu.mak
> @@ -10,3 +10,4 @@ CONFIG_SPIKE=y
>  CONFIG_SIFIVE_E=y
>  CONFIG_SIFIVE_U=y
>  CONFIG_RISCV_VIRT=y
> +CONFIG_OPENTITAN=y
> diff --git a/default-configs/riscv64-softmmu.mak b/default-configs/riscv64-softmmu.mak
> index 235c6f473f..aaf6d735bb 100644
> --- a/default-configs/riscv64-softmmu.mak
> +++ b/default-configs/riscv64-softmmu.mak
> @@ -1,3 +1,12 @@
>  # Default configuration for riscv64-softmmu
>
> -include riscv32-softmmu.mak
> +# Uncomment the following lines to disable these optional devices:
> +#
> +#CONFIG_PCI_DEVICES=n
> +
> +# Boards:
> +#
> +CONFIG_SPIKE=y
> +CONFIG_SIFIVE_E=y
> +CONFIG_SIFIVE_U=y
> +CONFIG_RISCV_VIRT=y
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index ff9fbe958a..94d19571f7 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -27,6 +27,11 @@ config SPIKE
>      select HTIF
>      select SIFIVE
>
> +config OPENTITAN
> +    bool
> +    select HART
> +    select UNIMP
> +
>  config RISCV_VIRT
>      bool
>      imply PCI_DEVICES
> diff --git a/hw/riscv/Makefile.objs b/hw/riscv/Makefile.objs
> index fc3c6dd7c8..57cc708f5d 100644
> --- a/hw/riscv/Makefile.objs
> +++ b/hw/riscv/Makefile.objs
> @@ -1,6 +1,7 @@
>  obj-y += boot.o
>  obj-$(CONFIG_SPIKE) += riscv_htif.o
>  obj-$(CONFIG_HART) += riscv_hart.o
> +obj-$(CONFIG_OPENTITAN) += opentitan.o
>  obj-$(CONFIG_SIFIVE_E) += sifive_e.o
>  obj-$(CONFIG_SIFIVE_E) += sifive_e_prci.o
>  obj-$(CONFIG_SIFIVE) += sifive_clint.o
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> new file mode 100644
> index 0000000000..c00f0720ab
> --- /dev/null
> +++ b/hw/riscv/opentitan.c
> @@ -0,0 +1,169 @@
> +/*
> + * QEMU RISC-V Board Compatible with OpenTitan FPGA platform
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * Provides a board compatible with the OpenTitan FPGA platform:
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * 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 "qemu/osdep.h"
> +#include "hw/riscv/opentitan.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/riscv/boot.h"
> +#include "exec/address-spaces.h"
> +
> +static const struct MemmapEntry {
> +    hwaddr base;
> +    hwaddr size;
> +} ibex_memmap[] = {
> +    [IBEX_ROM] =            {  0x00008000,   0xc000 },
> +    [IBEX_RAM] =            {  0x10000000,  0x10000 },
> +    [IBEX_FLASH] =          {  0x20000000,  0x80000 },
> +    [IBEX_UART] =           {  0x40000000,  0x10000 },
> +    [IBEX_GPIO] =           {  0x40010000,  0x10000 },
> +    [IBEX_SPI] =            {  0x40020000,  0x10000 },
> +    [IBEX_FLASH_CTRL] =     {  0x40030000,  0x10000 },
> +    [IBEX_PINMUX] =         {  0x40070000,  0x10000 },
> +    [IBEX_RV_TIMER] =       {  0x40080000,  0x10000 },
> +    [IBEX_PLIC] =           {  0x40090000,  0x10000 },
> +    [IBEX_AES] =            {  0x40110000,  0x10000 },
> +    [IBEX_HMAC] =           {  0x40120000,  0x10000 },
> +    [IBEX_ALERT_HANDLER] =  {  0x40130000,  0x10000 },
> +    [IBEX_USBDEV] =         {  0x40150000,  0x10000 }
> +};
> +
> +static void riscv_opentitan_init(MachineState *machine)
> +{
> +    const struct MemmapEntry *memmap = ibex_memmap;
> +    OpenTitanState *s = g_new0(OpenTitanState, 1);
> +    MemoryRegion *sys_mem = get_system_memory();
> +    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> +
> +    /* Initialize SoC */
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc,
> +                            sizeof(s->soc), TYPE_RISCV_IBEX_SOC,
> +                            &error_abort, NULL);
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> +                            &error_abort);
> +
> +    memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
> +        memmap[IBEX_RAM].size, &error_fatal);
> +    memory_region_add_subregion(sys_mem,
> +        memmap[IBEX_RAM].base, main_mem);
> +
> +
> +    if (machine->firmware) {
> +        riscv_load_firmware(machine->firmware, memmap[IBEX_RAM].base, NULL);
> +    }
> +
> +    if (machine->kernel_filename) {
> +        riscv_load_kernel(machine->kernel_filename, NULL);
> +    }
> +}
> +
> +static void riscv_opentitan_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "RISC-V Board compatible with OpenTitan";
> +    mc->init = riscv_opentitan_init;
> +    mc->max_cpus = 1;
> +    mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
> +}
> +
> +DEFINE_MACHINE("opentitan", riscv_opentitan_machine_init)
> +
> +static void riscv_lowrisc_ibex_soc_init(Object *obj)
> +{
> +    LowRISCIbexSoCState *s = RISCV_IBEX_SOC(obj);
> +
> +    object_initialize_child(obj, "cpus", &s->cpus,
> +                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
> +                            &error_abort, NULL);
> +}
> +
> +static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
> +{
> +    const struct MemmapEntry *memmap = ibex_memmap;
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    LowRISCIbexSoCState *s = RISCV_IBEX_SOC(dev_soc);
> +    MemoryRegion *sys_mem = get_system_memory();
> +
> +    object_property_set_str(OBJECT(&s->cpus), ms->cpu_type, "cpu-type",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->cpus), true, "realized",
> +                            &error_abort);
> +
> +    /* Boot ROM */
> +    memory_region_init_rom(&s->rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom",
> +                           memmap[IBEX_ROM].size, &error_fatal);
> +    memory_region_add_subregion(sys_mem,
> +        memmap[IBEX_ROM].base, &s->rom);
> +
> +    /* Flash memory */
> +    memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash",
> +                           memmap[IBEX_FLASH].size, &error_fatal);
> +    memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base,
> +                                &s->flash_mem);
> +
> +    create_unimplemented_device("riscv.lowrisc.ibex.uart",
> +        memmap[IBEX_UART].base, memmap[IBEX_UART].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.gpio",
> +        memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.spi",
> +        memmap[IBEX_SPI].base, memmap[IBEX_SPI].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl",
> +        memmap[IBEX_FLASH_CTRL].base, memmap[IBEX_FLASH_CTRL].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.rv_timer",
> +        memmap[IBEX_RV_TIMER].base, memmap[IBEX_RV_TIMER].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.aes",
> +        memmap[IBEX_AES].base, memmap[IBEX_AES].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.hmac",
> +        memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.plic",
> +        memmap[IBEX_PLIC].base, memmap[IBEX_PLIC].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
> +        memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.alert_handler",
> +        memmap[IBEX_ALERT_HANDLER].base, memmap[IBEX_ALERT_HANDLER].size);
> +    create_unimplemented_device("riscv.lowrisc.ibex.USBDEV",
> +        memmap[IBEX_USBDEV].base, memmap[IBEX_USBDEV].size);
> +}
> +
> +static void riscv_lowrisc_ibex_soc_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = riscv_lowrisc_ibex_soc_realize;
> +    /* Reason: Uses serial_hds in realize function, thus can't be used twice */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo riscv_lowrisc_ibex_soc_type_info = {
> +    .name = TYPE_RISCV_IBEX_SOC,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(LowRISCIbexSoCState),
> +    .instance_init = riscv_lowrisc_ibex_soc_init,
> +    .class_init = riscv_lowrisc_ibex_soc_class_init,
> +};
> +
> +static void riscv_lowrisc_ibex_soc_register_types(void)
> +{
> +    type_register_static(&riscv_lowrisc_ibex_soc_type_info);
> +}
> +
> +type_init(riscv_lowrisc_ibex_soc_register_types)
> diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
> new file mode 100644
> index 0000000000..15a3d87ed0
> --- /dev/null
> +++ b/include/hw/riscv/opentitan.h
> @@ -0,0 +1,63 @@
> +/*
> + * QEMU RISC-V Board Compatible with OpenTitan FPGA platform
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_OPENTITAN_H
> +#define HW_OPENTITAN_H
> +
> +#include "hw/riscv/riscv_hart.h"
> +
> +#define TYPE_RISCV_IBEX_SOC "riscv.lowrisc.ibex.soc"
> +#define RISCV_IBEX_SOC(obj) \
> +    OBJECT_CHECK(LowRISCIbexSoCState, (obj), TYPE_RISCV_IBEX_SOC)
> +
> +typedef struct LowRISCIbexSoCState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    RISCVHartArrayState cpus;
> +    MemoryRegion flash_mem;
> +    MemoryRegion rom;
> +} LowRISCIbexSoCState;
> +
> +typedef struct OpenTitanState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    LowRISCIbexSoCState soc;
> +} OpenTitanState;
> +
> +enum {
> +    IBEX_ROM,
> +    IBEX_RAM,
> +    IBEX_FLASH,
> +    IBEX_UART,
> +    IBEX_GPIO,
> +    IBEX_SPI,
> +    IBEX_FLASH_CTRL,
> +    IBEX_RV_TIMER,
> +    IBEX_AES,
> +    IBEX_HMAC,
> +    IBEX_PLIC,
> +    IBEX_PINMUX,
> +    IBEX_ALERT_HANDLER,
> +    IBEX_USBDEV,
> +};
> +
> +#endif

Looks good otherwise,
Reviewed-by: Bin Meng <bin.meng@windriver.com>

Regards,
Bin

[PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Alistair Francis]

This is the initial commit of the Ibex UART device. Serial TX is
working, while RX has been implemeneted but untested.

This is based on the documentation from:
https://docs.opentitan.org/hw/ip/uart/doc/

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 MAINTAINERS                 |   2 +
 hw/char/Makefile.objs       |   1 +
 hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
 hw/riscv/Kconfig            |   4 +
 include/hw/char/ibex_uart.h | 110 ++++++++
 5 files changed, 607 insertions(+)
 create mode 100644 hw/char/ibex_uart.c
 create mode 100644 include/hw/char/ibex_uart.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c3d77f0861..d3d47564ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,7 +1236,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
 L: qemu-riscv@nongnu.org
 S: Supported
 F: hw/riscv/opentitan.c
+F: hw/char/ibex_uart.c
 F: include/hw/riscv/opentitan.h
+F: include/hw/char/ibex_uart.h
 
 
 SH4 Machines
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 9e9a6c1aff..633996be5b 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
 common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
 common-obj-$(CONFIG_XEN) += xen_console.o
 common-obj-$(CONFIG_CADENCE) += cadence_uart.o
+common-obj-$(CONFIG_IBEX) += ibex_uart.o
 
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
 common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
new file mode 100644
index 0000000000..f6215ae23d
--- /dev/null
+++ b/hw/char/ibex_uart.c
@@ -0,0 +1,490 @@
+/*
+ * QEMU lowRISC Ibex UART device
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * For details check the documentation here:
+ *    https://docs.opentitan.org/hw/ip/uart/doc/
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/char/ibex_uart.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+
+static void ibex_uart_update_irqs(IbexUartState *s)
+{
+    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
+        qemu_set_irq(s->tx_watermark, 1);
+    } else {
+        qemu_set_irq(s->tx_watermark, 0);
+    }
+
+    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
+        qemu_set_irq(s->rx_watermark, 1);
+    } else {
+        qemu_set_irq(s->rx_watermark, 0);
+    }
+
+    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
+        qemu_set_irq(s->tx_empty, 1);
+    } else {
+        qemu_set_irq(s->tx_empty, 0);
+    }
+
+    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
+        qemu_set_irq(s->rx_overflow, 1);
+    } else {
+        qemu_set_irq(s->rx_overflow, 0);
+    }
+}
+
+static int ibex_uart_can_receive(void *opaque)
+{
+    IbexUartState *s = opaque;
+
+    if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
+        return 1;
+    }
+
+    return 0;
+}
+
+static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
+{
+    IbexUartState *s = opaque;
+    uint8_t rx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_RXILVL)
+                            >> FIFO_CTRL_RXILVL_SHIFT;
+
+    s->uart_rdata = *buf;
+
+    s->uart_status &= ~UART_STATUS_RXIDLE;
+    s->uart_status &= ~UART_STATUS_RXEMPTY;
+
+    if (size > rx_fifo_level) {
+        s->uart_intr_state |= INTR_STATE_RX_WATERMARK;
+    }
+
+    ibex_uart_update_irqs(s);
+}
+
+static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
+                               void *opaque)
+{
+    IbexUartState *s = opaque;
+    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
+                            >> FIFO_CTRL_TXILVL_SHIFT;
+    int ret;
+
+    /* instant drain the fifo when there's no back-end */
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
+        s->tx_level = 0;
+        return FALSE;
+    }
+
+    if (!s->tx_level) {
+        s->uart_status &= UART_STATUS_TXFULL;
+        s->uart_status |= UART_STATUS_TXEMPTY;
+        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
+        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
+        ibex_uart_update_irqs(s);
+        return FALSE;
+    }
+
+    ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level);
+
+    if (ret >= 0) {
+        s->tx_level -= ret;
+        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level);
+    }
+
+    if (s->tx_level) {
+        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                        ibex_uart_xmit, s);
+        if (!r) {
+            s->tx_level = 0;
+            return FALSE;
+        }
+    }
+
+    /* Clear the TX Full bit */
+    if (s->tx_level != IBEX_UART_TX_FIFO_SIZE) {
+        s->uart_status &= ~UART_STATUS_TXFULL;
+    }
+
+    /* Disable the TX_WATERMARK IRQ */
+    if (s->tx_level < tx_fifo_level) {
+        s->uart_intr_state &= ~INTR_STATE_TX_WATERMARK;
+    }
+
+    /* Set TX empty */
+    if (s->tx_level == 0) {
+        s->uart_status |= UART_STATUS_TXEMPTY;
+        s->uart_intr_state |= INTR_STATE_TX_EMPTY;
+    }
+
+    ibex_uart_update_irqs(s);
+    return FALSE;
+}
+
+static void uart_write_tx_fifo(IbexUartState *s, const uint8_t *buf,
+                               int size)
+{
+    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint8_t tx_fifo_level = (s->uart_fifo_ctrl & FIFO_CTRL_TXILVL)
+                            >> FIFO_CTRL_TXILVL_SHIFT;
+
+    if (size > IBEX_UART_TX_FIFO_SIZE - s->tx_level) {
+        size = IBEX_UART_TX_FIFO_SIZE - s->tx_level;
+        qemu_log_mask(LOG_GUEST_ERROR, "ibex_uart: TX FIFO overflow");
+    }
+
+    memcpy(s->tx_fifo + s->tx_level, buf, size);
+    s->tx_level += size;
+
+    if (s->tx_level > 0) {
+        s->uart_status &= ~UART_STATUS_TXEMPTY;
+    }
+
+    if (s->tx_level >= tx_fifo_level) {
+        s->uart_intr_state |= INTR_STATE_TX_WATERMARK;
+        ibex_uart_update_irqs(s);
+    }
+
+    if (s->tx_level == IBEX_UART_TX_FIFO_SIZE) {
+        s->uart_status |= UART_STATUS_TXFULL;
+    }
+
+    timer_mod(s->fifo_trigger_handle, current_time +
+              (s->char_tx_time * 4));
+}
+
+static void ibex_uart_reset(DeviceState *dev)
+{
+    IbexUartState *s = IBEX_UART(dev);
+
+    s->uart_intr_state = 0x00000000;
+    s->uart_intr_state = 0x00000000;
+    s->uart_intr_enable = 0x00000000;
+    s->uart_ctrl = 0x00000000;
+    s->uart_status = 0x0000003c;
+    s->uart_rdata = 0x00000000;
+    s->uart_fifo_ctrl = 0x00000000;
+    s->uart_fifo_status = 0x00000000;
+    s->uart_ovrd = 0x00000000;
+    s->uart_val = 0x00000000;
+    s->uart_timeout_ctrl = 0x00000000;
+
+    s->tx_level = 0;
+
+    s->char_tx_time = (NANOSECONDS_PER_SECOND / 230400) * 10;
+
+    ibex_uart_update_irqs(s);
+}
+
+static uint64_t ibex_uart_read(void *opaque, hwaddr addr,
+                                       unsigned int size)
+{
+    IbexUartState *s = opaque;
+    uint64_t retvalue = 0;
+
+    switch (addr) {
+    case IBEX_UART_INTR_STATE:
+        retvalue = s->uart_intr_state;
+        break;
+    case IBEX_UART_INTR_ENABLE:
+        retvalue = s->uart_intr_enable;
+        break;
+    case IBEX_UART_INTR_TEST:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: wdata is write only\n", __func__);
+        break;
+
+    case IBEX_UART_CTRL:
+        retvalue = s->uart_ctrl;
+        break;
+    case IBEX_UART_STATUS:
+        retvalue = s->uart_status;
+        break;
+
+    case IBEX_UART_RDATA:
+        retvalue = s->uart_rdata;
+        if (s->uart_ctrl & UART_CTRL_RX_ENABLE) {
+            qemu_chr_fe_accept_input(&s->chr);
+
+            s->uart_status |= UART_STATUS_RXIDLE;
+            s->uart_status |= UART_STATUS_RXEMPTY;
+        }
+        break;
+    case IBEX_UART_WDATA:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: wdata is write only\n", __func__);
+        break;
+
+    case IBEX_UART_FIFO_CTRL:
+        retvalue = s->uart_fifo_ctrl;
+        break;
+    case IBEX_UART_FIFO_STATUS:
+        retvalue = s->uart_fifo_status;
+
+        retvalue |= s->tx_level & 0x1F;
+
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: RX fifos are not supported\n", __func__);
+        break;
+
+    case IBEX_UART_OVRD:
+        retvalue = s->uart_ovrd;
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: ovrd is not supported\n", __func__);
+        break;
+    case IBEX_UART_VAL:
+        retvalue = s->uart_val;
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: val is not supported\n", __func__);
+        break;
+    case IBEX_UART_TIMEOUT_CTRL:
+        retvalue = s->uart_timeout_ctrl;
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: timeout_ctrl is not supported\n", __func__);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+        return 0;
+    }
+
+    return retvalue;
+}
+
+static void ibex_uart_write(void *opaque, hwaddr addr,
+                                  uint64_t val64, unsigned int size)
+{
+    IbexUartState *s = opaque;
+    uint32_t value = val64;
+
+    switch (addr) {
+    case IBEX_UART_INTR_STATE:
+        /* Write 1 clear */
+        s->uart_intr_state &= ~value;
+        ibex_uart_update_irqs(s);
+        break;
+    case IBEX_UART_INTR_ENABLE:
+        s->uart_intr_enable = value;
+        ibex_uart_update_irqs(s);
+        break;
+    case IBEX_UART_INTR_TEST:
+        s->uart_intr_state |= value;
+        ibex_uart_update_irqs(s);
+        break;
+
+    case IBEX_UART_CTRL:
+        s->uart_ctrl = value;
+
+        if (value & UART_CTRL_NF) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_NF is not supported\n", __func__);
+        }
+        if (value & UART_CTRL_SLPBK) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
+        }
+        if (value & UART_CTRL_LLPBK) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
+        }
+        if (value & UART_CTRL_PARITY_EN) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_PARITY_EN is not supported\n",
+                          __func__);
+        }
+        if (value & UART_CTRL_PARITY_ODD) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
+                          __func__);
+        }
+        if (value & UART_CTRL_RXBLVL) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
+        }
+        if (value & UART_CTRL_NCO) {
+            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
+            baud *= 1000;
+            baud /= 2 ^ 20;
+
+            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
+        }
+        break;
+    case IBEX_UART_STATUS:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: status is read only\n", __func__);
+        break;
+
+    case IBEX_UART_RDATA:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: rdata is read only\n", __func__);
+        break;
+    case IBEX_UART_WDATA:
+        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
+        break;
+
+    case IBEX_UART_FIFO_CTRL:
+        s->uart_fifo_ctrl = value;
+
+        if (value & FIFO_CTRL_RXRST) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: RX fifos are not supported\n", __func__);
+        }
+        if (value & FIFO_CTRL_TXRST) {
+            s->tx_level = 0;
+        }
+        break;
+    case IBEX_UART_FIFO_STATUS:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: fifo_status is read only\n", __func__);
+        break;
+
+    case IBEX_UART_OVRD:
+        s->uart_ovrd = value;
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: ovrd is not supported\n", __func__);
+        break;
+    case IBEX_UART_VAL:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: val is read only\n", __func__);
+        break;
+    case IBEX_UART_TIMEOUT_CTRL:
+        s->uart_timeout_ctrl = value;
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: timeout_ctrl is not supported\n", __func__);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+    }
+}
+
+static void fifo_trigger_update(void *opaque)
+{
+    IbexUartState *s = opaque;
+
+    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
+        ibex_uart_xmit(NULL, G_IO_OUT, s);
+    }
+}
+
+static const MemoryRegionOps ibex_uart_ops = {
+    .read = ibex_uart_read,
+    .write = ibex_uart_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static int cadence_uart_post_load(void *opaque, int version_id)
+{
+    IbexUartState *s = opaque;
+
+    ibex_uart_update_irqs(s);
+    return 0;
+}
+
+static const VMStateDescription vmstate_ibex_uart = {
+    .name = TYPE_IBEX_UART,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = cadence_uart_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(tx_fifo, IbexUartState,
+                            IBEX_UART_TX_FIFO_SIZE),
+        VMSTATE_UINT32(tx_level, IbexUartState),
+        VMSTATE_UINT64(char_tx_time, IbexUartState),
+        VMSTATE_TIMER_PTR(fifo_trigger_handle, IbexUartState),
+        VMSTATE_UINT32(uart_intr_state, IbexUartState),
+        VMSTATE_UINT32(uart_intr_enable, IbexUartState),
+        VMSTATE_UINT32(uart_ctrl, IbexUartState),
+        VMSTATE_UINT32(uart_status, IbexUartState),
+        VMSTATE_UINT32(uart_rdata, IbexUartState),
+        VMSTATE_UINT32(uart_fifo_ctrl, IbexUartState),
+        VMSTATE_UINT32(uart_fifo_status, IbexUartState),
+        VMSTATE_UINT32(uart_ovrd, IbexUartState),
+        VMSTATE_UINT32(uart_val, IbexUartState),
+        VMSTATE_UINT32(uart_timeout_ctrl, IbexUartState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property ibex_uart_properties[] = {
+    DEFINE_PROP_CHR("chardev", IbexUartState, chr),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ibex_uart_init(Object *obj)
+{
+    IbexUartState *s = IBEX_UART(obj);
+
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_watermark);
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_watermark);
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_empty);
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_overflow);
+
+    memory_region_init_io(&s->mmio, obj, &ibex_uart_ops, s,
+                          TYPE_IBEX_UART, 0x400);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+}
+
+static void ibex_uart_realize(DeviceState *dev, Error **errp)
+{
+    IbexUartState *s = IBEX_UART(dev);
+
+    s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                          fifo_trigger_update, s);
+
+    qemu_chr_fe_set_handlers(&s->chr, ibex_uart_can_receive,
+                             ibex_uart_receive, NULL, NULL,
+                             s, NULL, true);
+}
+
+static void ibex_uart_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = ibex_uart_reset;
+    dc->realize = ibex_uart_realize;
+    dc->vmsd = &vmstate_ibex_uart;
+    device_class_set_props(dc, ibex_uart_properties);
+}
+
+static const TypeInfo ibex_uart_info = {
+    .name          = TYPE_IBEX_UART,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IbexUartState),
+    .instance_init = ibex_uart_init,
+    .class_init    = ibex_uart_class_init,
+};
+
+static void ibex_uart_register_types(void)
+{
+    type_register_static(&ibex_uart_info);
+}
+
+type_init(ibex_uart_register_types)
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index 94d19571f7..28947ef3e0 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -4,6 +4,9 @@ config HTIF
 config HART
     bool
 
+config IBEX
+    bool
+
 config SIFIVE
     bool
     select MSI_NONBROKEN
@@ -29,6 +32,7 @@ config SPIKE
 
 config OPENTITAN
     bool
+    select IBEX
     select HART
     select UNIMP
 
diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
new file mode 100644
index 0000000000..2bec772615
--- /dev/null
+++ b/include/hw/char/ibex_uart.h
@@ -0,0 +1,110 @@
+/*
+ * QEMU lowRISC Ibex UART device
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * 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_IBEX_UART_H
+#define HW_IBEX_UART_H
+
+#include "hw/sysbus.h"
+#include "chardev/char-fe.h"
+#include "qemu/timer.h"
+
+#define IBEX_UART_INTR_STATE   0x00
+    #define INTR_STATE_TX_WATERMARK (1 << 0)
+    #define INTR_STATE_RX_WATERMARK (1 << 1)
+    #define INTR_STATE_TX_EMPTY     (1 << 2)
+    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
+#define IBEX_UART_INTR_ENABLE  0x04
+#define IBEX_UART_INTR_TEST    0x08
+
+#define IBEX_UART_CTRL         0x0c
+    #define UART_CTRL_TX_ENABLE     (1 << 0)
+    #define UART_CTRL_RX_ENABLE     (1 << 1)
+    #define UART_CTRL_NF            (1 << 2)
+    #define UART_CTRL_SLPBK         (1 << 4)
+    #define UART_CTRL_LLPBK         (1 << 5)
+    #define UART_CTRL_PARITY_EN     (1 << 6)
+    #define UART_CTRL_PARITY_ODD    (1 << 7)
+    #define UART_CTRL_RXBLVL        (3 << 8)
+    #define UART_CTRL_NCO           (0xFFFF << 16)
+
+#define IBEX_UART_STATUS       0x10
+    #define UART_STATUS_TXFULL  (1 << 0)
+    #define UART_STATUS_RXFULL  (1 << 1)
+    #define UART_STATUS_TXEMPTY (1 << 2)
+    #define UART_STATUS_RXIDLE  (1 << 4)
+    #define UART_STATUS_RXEMPTY (1 << 5)
+
+#define IBEX_UART_RDATA        0x14
+#define IBEX_UART_WDATA        0x18
+
+#define IBEX_UART_FIFO_CTRL    0x1c
+    #define FIFO_CTRL_RXRST          (1 << 0)
+    #define FIFO_CTRL_TXRST          (1 << 1)
+    #define FIFO_CTRL_RXILVL         (7 << 2)
+    #define FIFO_CTRL_RXILVL_SHIFT   (2)
+    #define FIFO_CTRL_TXILVL         (3 << 5)
+    #define FIFO_CTRL_TXILVL_SHIFT   (5)
+
+#define IBEX_UART_FIFO_STATUS  0x20
+#define IBEX_UART_OVRD         0x24
+#define IBEX_UART_VAL          0x28
+#define IBEX_UART_TIMEOUT_CTRL 0x2c
+
+#define IBEX_UART_TX_FIFO_SIZE 16
+
+#define TYPE_IBEX_UART "ibex-uart"
+#define IBEX_UART(obj) \
+    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
+
+typedef struct {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    /* <public> */
+    MemoryRegion mmio;
+
+    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
+    uint32_t tx_level;
+
+    QEMUTimer *fifo_trigger_handle;
+    uint64_t char_tx_time;
+
+    uint32_t uart_intr_state;
+    uint32_t uart_intr_enable;
+    uint32_t uart_ctrl;
+    uint32_t uart_status;
+    uint32_t uart_rdata;
+    uint32_t uart_fifo_ctrl;
+    uint32_t uart_fifo_status;
+    uint32_t uart_ovrd;
+    uint32_t uart_val;
+    uint32_t uart_timeout_ctrl;
+
+    CharBackend chr;
+    qemu_irq tx_watermark;
+    qemu_irq rx_watermark;
+    qemu_irq tx_empty;
+    qemu_irq rx_overflow;
+} IbexUartState;
+#endif /* HW_IBEX_UART_H */
-- 
2.26.2

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Philippe Mathieu-Daudé]

Hi Alistair,

On 5/7/20 9:13 PM, Alistair Francis wrote:
> This is the initial commit of the Ibex UART device. Serial TX is
> working, while RX has been implemeneted but untested.
> 
> This is based on the documentation from:
> https://docs.opentitan.org/hw/ip/uart/doc/
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   MAINTAINERS                 |   2 +
>   hw/char/Makefile.objs       |   1 +
>   hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
>   hw/riscv/Kconfig            |   4 +
>   include/hw/char/ibex_uart.h | 110 ++++++++
>   5 files changed, 607 insertions(+)
>   create mode 100644 hw/char/ibex_uart.c
>   create mode 100644 include/hw/char/ibex_uart.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c3d77f0861..d3d47564ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1236,7 +1236,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
>   L: qemu-riscv@nongnu.org
>   S: Supported
>   F: hw/riscv/opentitan.c
> +F: hw/char/ibex_uart.c
>   F: include/hw/riscv/opentitan.h
> +F: include/hw/char/ibex_uart.h
>   
>   
>   SH4 Machines
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 9e9a6c1aff..633996be5b 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
>   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
>   common-obj-$(CONFIG_XEN) += xen_console.o
>   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> +common-obj-$(CONFIG_IBEX) += ibex_uart.o
>   
>   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
>   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> new file mode 100644
> index 0000000000..f6215ae23d
> --- /dev/null
> +++ b/hw/char/ibex_uart.c
> @@ -0,0 +1,490 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * For details check the documentation here:
> + *    https://docs.opentitan.org/hw/ip/uart/doc/
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/ibex_uart.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static void ibex_uart_update_irqs(IbexUartState *s)
> +{
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
> +        qemu_set_irq(s->tx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->tx_watermark, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
> +        qemu_set_irq(s->rx_watermark, 1);
> +    } else {
> +        qemu_set_irq(s->rx_watermark, 0);

I wonder if having both bit separate can't lead to odd pulse behavior 
(this function should have the same result if you invert the RX/TX 
processing here). I'd be safer using a local 'raise_watermark' boolean 
variable, then call qemu_set_irq() once.

> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> +        qemu_set_irq(s->tx_empty, 1);
> +    } else {
> +        qemu_set_irq(s->tx_empty, 0);
> +    }
> +
> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
> +        qemu_set_irq(s->rx_overflow, 1);
> +    } else {
> +        qemu_set_irq(s->rx_overflow, 0);
> +    }
> +}
[...]

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Alistair Francis]

On Thu, May 14, 2020 at 11:00 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Alistair,
>
> On 5/7/20 9:13 PM, Alistair Francis wrote:
> > This is the initial commit of the Ibex UART device. Serial TX is
> > working, while RX has been implemeneted but untested.
> >
> > This is based on the documentation from:
> > https://docs.opentitan.org/hw/ip/uart/doc/
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   MAINTAINERS                 |   2 +
> >   hw/char/Makefile.objs       |   1 +
> >   hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
> >   hw/riscv/Kconfig            |   4 +
> >   include/hw/char/ibex_uart.h | 110 ++++++++
> >   5 files changed, 607 insertions(+)
> >   create mode 100644 hw/char/ibex_uart.c
> >   create mode 100644 include/hw/char/ibex_uart.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c3d77f0861..d3d47564ce 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1236,7 +1236,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
> >   L: qemu-riscv@nongnu.org
> >   S: Supported
> >   F: hw/riscv/opentitan.c
> > +F: hw/char/ibex_uart.c
> >   F: include/hw/riscv/opentitan.h
> > +F: include/hw/char/ibex_uart.h
> >
> >
> >   SH4 Machines
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index 9e9a6c1aff..633996be5b 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
> >   common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
> >   common-obj-$(CONFIG_XEN) += xen_console.o
> >   common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> > +common-obj-$(CONFIG_IBEX) += ibex_uart.o
> >
> >   common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
> >   common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> > new file mode 100644
> > index 0000000000..f6215ae23d
> > --- /dev/null
> > +++ b/hw/char/ibex_uart.c
> > @@ -0,0 +1,490 @@
> > +/*
> > + * QEMU lowRISC Ibex UART device
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * For details check the documentation here:
> > + *    https://docs.opentitan.org/hw/ip/uart/doc/
> > + *
> > + * 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.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/char/ibex_uart.h"
> > +#include "hw/irq.h"
> > +#include "hw/qdev-properties.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +
> > +static void ibex_uart_update_irqs(IbexUartState *s)
> > +{
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
> > +        qemu_set_irq(s->tx_watermark, 1);
> > +    } else {
> > +        qemu_set_irq(s->tx_watermark, 0);
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
> > +        qemu_set_irq(s->rx_watermark, 1);
> > +    } else {
> > +        qemu_set_irq(s->rx_watermark, 0);
>
> I wonder if having both bit separate can't lead to odd pulse behavior
> (this function should have the same result if you invert the RX/TX
> processing here). I'd be safer using a local 'raise_watermark' boolean
> variable, then call qemu_set_irq() once.

I'm not sure what you mean. Are you worried that TX and RX will both
go high/low at the same time?

Alistair

>
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
> > +        qemu_set_irq(s->tx_empty, 1);
> > +    } else {
> > +        qemu_set_irq(s->tx_empty, 0);
> > +    }
> > +
> > +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
> > +        qemu_set_irq(s->rx_overflow, 1);
> > +    } else {
> > +        qemu_set_irq(s->rx_overflow, 0);
> > +    }
> > +}
> [...]
>

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Philippe Mathieu-Daudé]

On 5/14/20 11:59 PM, Alistair Francis wrote:
> On Thu, May 14, 2020 at 11:00 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> Hi Alistair,
>>
>> On 5/7/20 9:13 PM, Alistair Francis wrote:
>>> This is the initial commit of the Ibex UART device. Serial TX is
>>> working, while RX has been implemeneted but untested.
>>>
>>> This is based on the documentation from:
>>> https://docs.opentitan.org/hw/ip/uart/doc/
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>>    MAINTAINERS                 |   2 +
>>>    hw/char/Makefile.objs       |   1 +
>>>    hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
>>>    hw/riscv/Kconfig            |   4 +
>>>    include/hw/char/ibex_uart.h | 110 ++++++++
>>>    5 files changed, 607 insertions(+)
>>>    create mode 100644 hw/char/ibex_uart.c
>>>    create mode 100644 include/hw/char/ibex_uart.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index c3d77f0861..d3d47564ce 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1236,7 +1236,9 @@ M: Alistair Francis <Alistair.Francis@wdc.com>
>>>    L: qemu-riscv@nongnu.org
>>>    S: Supported
>>>    F: hw/riscv/opentitan.c
>>> +F: hw/char/ibex_uart.c
>>>    F: include/hw/riscv/opentitan.h
>>> +F: include/hw/char/ibex_uart.h
>>>
>>>
>>>    SH4 Machines
>>> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
>>> index 9e9a6c1aff..633996be5b 100644
>>> --- a/hw/char/Makefile.objs
>>> +++ b/hw/char/Makefile.objs
>>> @@ -12,6 +12,7 @@ common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
>>>    common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
>>>    common-obj-$(CONFIG_XEN) += xen_console.o
>>>    common-obj-$(CONFIG_CADENCE) += cadence_uart.o
>>> +common-obj-$(CONFIG_IBEX) += ibex_uart.o
>>>
>>>    common-obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
>>>    common-obj-$(CONFIG_COLDFIRE) += mcf_uart.o
>>> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
>>> new file mode 100644
>>> index 0000000000..f6215ae23d
>>> --- /dev/null
>>> +++ b/hw/char/ibex_uart.c
>>> @@ -0,0 +1,490 @@
>>> +/*
>>> + * QEMU lowRISC Ibex UART device
>>> + *
>>> + * Copyright (c) 2020 Western Digital
>>> + *
>>> + * For details check the documentation here:
>>> + *    https://docs.opentitan.org/hw/ip/uart/doc/
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/char/ibex_uart.h"
>>> +#include "hw/irq.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "migration/vmstate.h"
>>> +#include "qemu/log.h"
>>> +#include "qemu/module.h"
>>> +
>>> +static void ibex_uart_update_irqs(IbexUartState *s)
>>> +{
>>> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_WATERMARK) {
>>> +        qemu_set_irq(s->tx_watermark, 1);
>>> +    } else {
>>> +        qemu_set_irq(s->tx_watermark, 0);
>>> +    }
>>> +
>>> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_WATERMARK) {
>>> +        qemu_set_irq(s->rx_watermark, 1);
>>> +    } else {
>>> +        qemu_set_irq(s->rx_watermark, 0);
>>
>> I wonder if having both bit separate can't lead to odd pulse behavior
>> (this function should have the same result if you invert the RX/TX
>> processing here). I'd be safer using a local 'raise_watermark' boolean
>> variable, then call qemu_set_irq() once.
> 
> I'm not sure what you mean. Are you worried that TX and RX will both
> go high/low at the same time?

Disregard my comment, it was late and I misread that rx/tx are different 
outgoing IRQs (I read this as a single watermark IRQ).

> 
> Alistair
> 
>>
>>> +    }
>>> +
>>> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_TX_EMPTY) {
>>> +        qemu_set_irq(s->tx_empty, 1);
>>> +    } else {
>>> +        qemu_set_irq(s->tx_empty, 0);
>>> +    }
>>> +
>>> +    if (s->uart_intr_state & s->uart_intr_enable & INTR_STATE_RX_OVERFLOW) {
>>> +        qemu_set_irq(s->rx_overflow, 1);
>>> +    } else {
>>> +        qemu_set_irq(s->rx_overflow, 0);
>>> +    }
>>> +}
>> [...]
>>
> 

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Philippe Mathieu-Daudé]

On 5/7/20 9:13 PM, Alistair Francis wrote:
> This is the initial commit of the Ibex UART device. Serial TX is
> working, while RX has been implemeneted but untested.
> 
> This is based on the documentation from:
> https://docs.opentitan.org/hw/ip/uart/doc/
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   MAINTAINERS                 |   2 +
>   hw/char/Makefile.objs       |   1 +
>   hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
>   hw/riscv/Kconfig            |   4 +
>   include/hw/char/ibex_uart.h | 110 ++++++++

If possible setup scripts/git.orderfile to ease review (avoid scrolling).

>   5 files changed, 607 insertions(+)
>   create mode 100644 hw/char/ibex_uart.c
>   create mode 100644 include/hw/char/ibex_uart.h
> 
[...]
> +static void ibex_uart_write(void *opaque, hwaddr addr,
> +                                  uint64_t val64, unsigned int size)
> +{
> +    IbexUartState *s = opaque;
> +    uint32_t value = val64;
> +
> +    switch (addr) {
> +    case IBEX_UART_INTR_STATE:
> +        /* Write 1 clear */
> +        s->uart_intr_state &= ~value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_ENABLE:
> +        s->uart_intr_enable = value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +    case IBEX_UART_INTR_TEST:
> +        s->uart_intr_state |= value;
> +        ibex_uart_update_irqs(s);
> +        break;
> +
> +    case IBEX_UART_CTRL:
> +        s->uart_ctrl = value;
> +
> +        if (value & UART_CTRL_NF) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_NF is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_SLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_LLPBK) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_EN) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_EN is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_PARITY_ODD) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
> +                          __func__);
> +        }
> +        if (value & UART_CTRL_RXBLVL) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
> +        }
> +        if (value & UART_CTRL_NCO) {
> +            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
> +            baud *= 1000;
> +            baud /= 2 ^ 20;
> +
> +            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> +        }
> +        break;
> +    case IBEX_UART_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_RDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: rdata is read only\n", __func__);
> +        break;
> +    case IBEX_UART_WDATA:
> +        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
> +        break;
> +
> +    case IBEX_UART_FIFO_CTRL:
> +        s->uart_fifo_ctrl = value;
> +
> +        if (value & FIFO_CTRL_RXRST) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: RX fifos are not supported\n", __func__);
> +        }
> +        if (value & FIFO_CTRL_TXRST) {
> +            s->tx_level = 0;
> +        }
> +        break;
> +    case IBEX_UART_FIFO_STATUS:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fifo_status is read only\n", __func__);
> +        break;
> +
> +    case IBEX_UART_OVRD:
> +        s->uart_ovrd = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: ovrd is not supported\n", __func__);
> +        break;
> +    case IBEX_UART_VAL:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: val is read only\n", __func__);
> +        break;
> +    case IBEX_UART_TIMEOUT_CTRL:
> +        s->uart_timeout_ctrl = value;
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: timeout_ctrl is not supported\n", __func__);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +}
> +
> +static void fifo_trigger_update(void *opaque)
> +{
> +    IbexUartState *s = opaque;
> +
> +    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
> +        ibex_uart_xmit(NULL, G_IO_OUT, s);
> +    }
> +}
> +
> +static const MemoryRegionOps ibex_uart_ops = {
> +    .read = ibex_uart_read,
> +    .write = ibex_uart_write,

As all registers are 32-bit, you want .impl min/max = 4 here.

Otherwise patch looks good.

> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
[...]
> diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> new file mode 100644
> index 0000000000..2bec772615
> --- /dev/null
> +++ b/include/hw/char/ibex_uart.h
> @@ -0,0 +1,110 @@
> +/*
> + * QEMU lowRISC Ibex UART device
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * 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_IBEX_UART_H
> +#define HW_IBEX_UART_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +#include "qemu/timer.h"
> +
> +#define IBEX_UART_INTR_STATE   0x00
> +    #define INTR_STATE_TX_WATERMARK (1 << 0)
> +    #define INTR_STATE_RX_WATERMARK (1 << 1)
> +    #define INTR_STATE_TX_EMPTY     (1 << 2)
> +    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
> +#define IBEX_UART_INTR_ENABLE  0x04
> +#define IBEX_UART_INTR_TEST    0x08
> +
> +#define IBEX_UART_CTRL         0x0c
> +    #define UART_CTRL_TX_ENABLE     (1 << 0)
> +    #define UART_CTRL_RX_ENABLE     (1 << 1)
> +    #define UART_CTRL_NF            (1 << 2)
> +    #define UART_CTRL_SLPBK         (1 << 4)
> +    #define UART_CTRL_LLPBK         (1 << 5)
> +    #define UART_CTRL_PARITY_EN     (1 << 6)
> +    #define UART_CTRL_PARITY_ODD    (1 << 7)
> +    #define UART_CTRL_RXBLVL        (3 << 8)
> +    #define UART_CTRL_NCO           (0xFFFF << 16)
> +
> +#define IBEX_UART_STATUS       0x10
> +    #define UART_STATUS_TXFULL  (1 << 0)
> +    #define UART_STATUS_RXFULL  (1 << 1)
> +    #define UART_STATUS_TXEMPTY (1 << 2)
> +    #define UART_STATUS_RXIDLE  (1 << 4)
> +    #define UART_STATUS_RXEMPTY (1 << 5)
> +
> +#define IBEX_UART_RDATA        0x14
> +#define IBEX_UART_WDATA        0x18
> +
> +#define IBEX_UART_FIFO_CTRL    0x1c
> +    #define FIFO_CTRL_RXRST          (1 << 0)
> +    #define FIFO_CTRL_TXRST          (1 << 1)
> +    #define FIFO_CTRL_RXILVL         (7 << 2)
> +    #define FIFO_CTRL_RXILVL_SHIFT   (2)
> +    #define FIFO_CTRL_TXILVL         (3 << 5)
> +    #define FIFO_CTRL_TXILVL_SHIFT   (5)
> +
> +#define IBEX_UART_FIFO_STATUS  0x20
> +#define IBEX_UART_OVRD         0x24
> +#define IBEX_UART_VAL          0x28
> +#define IBEX_UART_TIMEOUT_CTRL 0x2c
> +
> +#define IBEX_UART_TX_FIFO_SIZE 16
> +
> +#define TYPE_IBEX_UART "ibex-uart"
> +#define IBEX_UART(obj) \
> +    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
> +
> +typedef struct {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion mmio;
> +
> +    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> +    uint32_t tx_level;
> +
> +    QEMUTimer *fifo_trigger_handle;
> +    uint64_t char_tx_time;
> +
> +    uint32_t uart_intr_state;
> +    uint32_t uart_intr_enable;
> +    uint32_t uart_ctrl;
> +    uint32_t uart_status;
> +    uint32_t uart_rdata;
> +    uint32_t uart_fifo_ctrl;
> +    uint32_t uart_fifo_status;
> +    uint32_t uart_ovrd;
> +    uint32_t uart_val;
> +    uint32_t uart_timeout_ctrl;
> +
> +    CharBackend chr;
> +    qemu_irq tx_watermark;
> +    qemu_irq rx_watermark;
> +    qemu_irq tx_empty;
> +    qemu_irq rx_overflow;
> +} IbexUartState;
> +#endif /* HW_IBEX_UART_H */
> 

Re: [PATCH v2 5/9] hw/char: Initial commit of Ibex UART

[Alistair Francis]

On Fri, May 15, 2020 at 12:28 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 5/7/20 9:13 PM, Alistair Francis wrote:
> > This is the initial commit of the Ibex UART device. Serial TX is
> > working, while RX has been implemeneted but untested.
> >
> > This is based on the documentation from:
> > https://docs.opentitan.org/hw/ip/uart/doc/
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   MAINTAINERS                 |   2 +
> >   hw/char/Makefile.objs       |   1 +
> >   hw/char/ibex_uart.c         | 490 ++++++++++++++++++++++++++++++++++++
> >   hw/riscv/Kconfig            |   4 +
> >   include/hw/char/ibex_uart.h | 110 ++++++++
>
> If possible setup scripts/git.orderfile to ease review (avoid scrolling).

I have set this up.

>
> >   5 files changed, 607 insertions(+)
> >   create mode 100644 hw/char/ibex_uart.c
> >   create mode 100644 include/hw/char/ibex_uart.h
> >
> [...]
> > +static void ibex_uart_write(void *opaque, hwaddr addr,
> > +                                  uint64_t val64, unsigned int size)
> > +{
> > +    IbexUartState *s = opaque;
> > +    uint32_t value = val64;
> > +
> > +    switch (addr) {
> > +    case IBEX_UART_INTR_STATE:
> > +        /* Write 1 clear */
> > +        s->uart_intr_state &= ~value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +    case IBEX_UART_INTR_ENABLE:
> > +        s->uart_intr_enable = value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +    case IBEX_UART_INTR_TEST:
> > +        s->uart_intr_state |= value;
> > +        ibex_uart_update_irqs(s);
> > +        break;
> > +
> > +    case IBEX_UART_CTRL:
> > +        s->uart_ctrl = value;
> > +
> > +        if (value & UART_CTRL_NF) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_NF is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_SLPBK) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_SLPBK is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_LLPBK) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_LLPBK is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_PARITY_EN) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_PARITY_EN is not supported\n",
> > +                          __func__);
> > +        }
> > +        if (value & UART_CTRL_PARITY_ODD) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_PARITY_ODD is not supported\n",
> > +                          __func__);
> > +        }
> > +        if (value & UART_CTRL_RXBLVL) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: UART_CTRL_RXBLVL is not supported\n", __func__);
> > +        }
> > +        if (value & UART_CTRL_NCO) {
> > +            uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
> > +            baud *= 1000;
> > +            baud /= 2 ^ 20;
> > +
> > +            s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> > +        }
> > +        break;
> > +    case IBEX_UART_STATUS:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: status is read only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_RDATA:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: rdata is read only\n", __func__);
> > +        break;
> > +    case IBEX_UART_WDATA:
> > +        uart_write_tx_fifo(s, (uint8_t *) &value, 1);
> > +        break;
> > +
> > +    case IBEX_UART_FIFO_CTRL:
> > +        s->uart_fifo_ctrl = value;
> > +
> > +        if (value & FIFO_CTRL_RXRST) {
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "%s: RX fifos are not supported\n", __func__);
> > +        }
> > +        if (value & FIFO_CTRL_TXRST) {
> > +            s->tx_level = 0;
> > +        }
> > +        break;
> > +    case IBEX_UART_FIFO_STATUS:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: fifo_status is read only\n", __func__);
> > +        break;
> > +
> > +    case IBEX_UART_OVRD:
> > +        s->uart_ovrd = value;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: ovrd is not supported\n", __func__);
> > +        break;
> > +    case IBEX_UART_VAL:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: val is read only\n", __func__);
> > +        break;
> > +    case IBEX_UART_TIMEOUT_CTRL:
> > +        s->uart_timeout_ctrl = value;
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: timeout_ctrl is not supported\n", __func__);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> > +    }
> > +}
> > +
> > +static void fifo_trigger_update(void *opaque)
> > +{
> > +    IbexUartState *s = opaque;
> > +
> > +    if (s->uart_ctrl & UART_CTRL_TX_ENABLE) {
> > +        ibex_uart_xmit(NULL, G_IO_OUT, s);
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps ibex_uart_ops = {
> > +    .read = ibex_uart_read,
> > +    .write = ibex_uart_write,
>
> As all registers are 32-bit, you want .impl min/max = 4 here.

Added.

>
> Otherwise patch looks good.

Thanks!

Alistair

>
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> [...]
> > diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> > new file mode 100644
> > index 0000000000..2bec772615
> > --- /dev/null
> > +++ b/include/hw/char/ibex_uart.h
> > @@ -0,0 +1,110 @@
> > +/*
> > + * QEMU lowRISC Ibex UART device
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * 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_IBEX_UART_H
> > +#define HW_IBEX_UART_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "chardev/char-fe.h"
> > +#include "qemu/timer.h"
> > +
> > +#define IBEX_UART_INTR_STATE   0x00
> > +    #define INTR_STATE_TX_WATERMARK (1 << 0)
> > +    #define INTR_STATE_RX_WATERMARK (1 << 1)
> > +    #define INTR_STATE_TX_EMPTY     (1 << 2)
> > +    #define INTR_STATE_RX_OVERFLOW  (1 << 3)
> > +#define IBEX_UART_INTR_ENABLE  0x04
> > +#define IBEX_UART_INTR_TEST    0x08
> > +
> > +#define IBEX_UART_CTRL         0x0c
> > +    #define UART_CTRL_TX_ENABLE     (1 << 0)
> > +    #define UART_CTRL_RX_ENABLE     (1 << 1)
> > +    #define UART_CTRL_NF            (1 << 2)
> > +    #define UART_CTRL_SLPBK         (1 << 4)
> > +    #define UART_CTRL_LLPBK         (1 << 5)
> > +    #define UART_CTRL_PARITY_EN     (1 << 6)
> > +    #define UART_CTRL_PARITY_ODD    (1 << 7)
> > +    #define UART_CTRL_RXBLVL        (3 << 8)
> > +    #define UART_CTRL_NCO           (0xFFFF << 16)
> > +
> > +#define IBEX_UART_STATUS       0x10
> > +    #define UART_STATUS_TXFULL  (1 << 0)
> > +    #define UART_STATUS_RXFULL  (1 << 1)
> > +    #define UART_STATUS_TXEMPTY (1 << 2)
> > +    #define UART_STATUS_RXIDLE  (1 << 4)
> > +    #define UART_STATUS_RXEMPTY (1 << 5)
> > +
> > +#define IBEX_UART_RDATA        0x14
> > +#define IBEX_UART_WDATA        0x18
> > +
> > +#define IBEX_UART_FIFO_CTRL    0x1c
> > +    #define FIFO_CTRL_RXRST          (1 << 0)
> > +    #define FIFO_CTRL_TXRST          (1 << 1)
> > +    #define FIFO_CTRL_RXILVL         (7 << 2)
> > +    #define FIFO_CTRL_RXILVL_SHIFT   (2)
> > +    #define FIFO_CTRL_TXILVL         (3 << 5)
> > +    #define FIFO_CTRL_TXILVL_SHIFT   (5)
> > +
> > +#define IBEX_UART_FIFO_STATUS  0x20
> > +#define IBEX_UART_OVRD         0x24
> > +#define IBEX_UART_VAL          0x28
> > +#define IBEX_UART_TIMEOUT_CTRL 0x2c
> > +
> > +#define IBEX_UART_TX_FIFO_SIZE 16
> > +
> > +#define TYPE_IBEX_UART "ibex-uart"
> > +#define IBEX_UART(obj) \
> > +    OBJECT_CHECK(IbexUartState, (obj), TYPE_IBEX_UART)
> > +
> > +typedef struct {
> > +    /* <private> */
> > +    SysBusDevice parent_obj;
> > +
> > +    /* <public> */
> > +    MemoryRegion mmio;
> > +
> > +    uint8_t tx_fifo[IBEX_UART_TX_FIFO_SIZE];
> > +    uint32_t tx_level;
> > +
> > +    QEMUTimer *fifo_trigger_handle;
> > +    uint64_t char_tx_time;
> > +
> > +    uint32_t uart_intr_state;
> > +    uint32_t uart_intr_enable;
> > +    uint32_t uart_ctrl;
> > +    uint32_t uart_status;
> > +    uint32_t uart_rdata;
> > +    uint32_t uart_fifo_ctrl;
> > +    uint32_t uart_fifo_status;
> > +    uint32_t uart_ovrd;
> > +    uint32_t uart_val;
> > +    uint32_t uart_timeout_ctrl;
> > +
> > +    CharBackend chr;
> > +    qemu_irq tx_watermark;
> > +    qemu_irq rx_watermark;
> > +    qemu_irq tx_empty;
> > +    qemu_irq rx_overflow;
> > +} IbexUartState;
> > +#endif /* HW_IBEX_UART_H */
> >
>

[PATCH v2 6/9] hw/intc: Initial commit of lowRISC Ibex PLIC

[Alistair Francis]

The Ibex core contains a PLIC that although similar to the RISC-V spec
is not RISC-V spec compliant.

This patch implements a Ibex PLIC in a somewhat generic way.

As the current RISC-V PLIC needs tidying up, my hope is that as the Ibex
PLIC move towards spec compliance this PLIC implementation can be
updated until it can replace the current PLIC.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 MAINTAINERS                 |   2 +
 hw/intc/Makefile.objs       |   1 +
 hw/intc/ibex_plic.c         | 261 ++++++++++++++++++++++++++++++++++++
 include/hw/intc/ibex_plic.h |  63 +++++++++
 4 files changed, 327 insertions(+)
 create mode 100644 hw/intc/ibex_plic.c
 create mode 100644 include/hw/intc/ibex_plic.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d3d47564ce..f8c3cf6182 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1237,8 +1237,10 @@ L: qemu-riscv@nongnu.org
 S: Supported
 F: hw/riscv/opentitan.c
 F: hw/char/ibex_uart.c
+F: hw/intc/ibex_plic.c
 F: include/hw/riscv/opentitan.h
 F: include/hw/char/ibex_uart.h
+F: include/hw/intc/ibex_plic.h
 
 
 SH4 Machines
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index f726d87532..a61e6728fe 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -49,3 +49,4 @@ obj-$(CONFIG_ARM_GIC) += arm_gicv3_cpuif.o
 obj-$(CONFIG_MIPS_CPS) += mips_gic.o
 obj-$(CONFIG_NIOS2) += nios2_iic.o
 obj-$(CONFIG_OMPIC) += ompic.o
+obj-$(CONFIG_IBEX) += ibex_plic.o
diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c
new file mode 100644
index 0000000000..35c52d9d16
--- /dev/null
+++ b/hw/intc/ibex_plic.c
@@ -0,0 +1,261 @@
+/*
+ * QEMU RISC-V lowRISC Ibex PLIC
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * Documentation avaliable: https://docs.opentitan.org/hw/ip/rv_plic/doc/
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 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 "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/core/cpu.h"
+#include "hw/boards.h"
+#include "hw/pci/msi.h"
+#include "target/riscv/cpu_bits.h"
+#include "target/riscv/cpu.h"
+#include "hw/intc/ibex_plic.h"
+
+static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
+{
+    uint32_t end = base + (num * 0x04);
+
+    if (addr >= base && addr < end) {
+        return true;
+    }
+
+    return false;
+}
+
+static void ibex_plic_irqs_set_pending(IbexPlicState *s, int irq, bool level)
+{
+    int pending_num = irq / 32;
+
+    s->pending[pending_num] |= level << (irq % 32);
+}
+
+static bool ibex_plic_irqs_pending(IbexPlicState *s, uint32_t context)
+{
+    int i;
+
+    for (i = 0; i < s->pending_num; i++) {
+        uint32_t irq_num = ctz64(s->pending[i]) + (i * 32);
+
+        if (!(s->pending[i] & s->enable[i])) {
+            /* No pending and enabled IRQ */
+            continue;
+        }
+
+        if (s->priority[irq_num] > s->threshold) {
+            if (!s->claim) {
+                s->claim = irq_num;
+            }
+            return true;
+        }
+    }
+
+    return 0;
+}
+
+static void ibex_plic_update(IbexPlicState *s)
+{
+    CPUState *cpu;
+    int level, i;
+
+    for (i = 0; i < s->num_cpus; i++) {
+        cpu = qemu_get_cpu(i);
+
+        if (!cpu) {
+            continue;
+        }
+
+        level = ibex_plic_irqs_pending(s, 0);
+
+        riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MEIP, BOOL_TO_MASK(level));
+    }
+}
+
+static void ibex_plic_reset(DeviceState *dev)
+{
+    IbexPlicState *s = IBEX_PLIC(dev);
+
+    s->threshold = 0x00000000;
+    s->claim = 0x00000000;
+}
+
+static uint64_t ibex_plic_read(void *opaque, hwaddr addr,
+                               unsigned int size)
+{
+    IbexPlicState *s = opaque;
+    int offset;
+    uint32_t ret = 0;
+
+    if (addr_between(addr, s->pending_base, s->pending_num)) {
+        offset = (addr - s->pending_base) / 4;
+        ret = s->pending[offset];
+    } else if (addr_between(addr, s->source_base, s->source_num)) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Interrupt source mode not supported\n", __func__);
+    } else if (addr_between(addr, s->priority_base, s->priority_num)) {
+        offset = (addr - s->priority_base) / 4;
+        ret = s->priority[offset];
+    } else if (addr_between(addr, s->enable_base, s->enable_num)) {
+        offset = (addr - s->enable_base) / 4;
+        ret = s->enable[offset];
+    } else if (addr_between(addr, s->threshold_base, 1)) {
+        ret = s->threshold;
+    } else if (addr_between(addr, s->claim_base, 1)) {
+        int pending_num = s->claim / 32;
+        s->pending[pending_num] &= ~(1 << (s->claim % 32));
+
+        ret = s->claim;
+    }
+
+    return ret;
+}
+
+static void ibex_plic_write(void *opaque, hwaddr addr,
+                            uint64_t value, unsigned int size)
+{
+    IbexPlicState *s = opaque;
+
+    if (addr_between(addr, s->pending_base, s->pending_num)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Pending registers are read only\n", __func__);
+    } else if (addr_between(addr, s->source_base, s->source_num)) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Interrupt source mode not supported\n", __func__);
+    } else if (addr_between(addr, s->priority_base, s->priority_num)) {
+        uint32_t irq = ((addr - s->priority_base) >> 2) + 1;
+        s->priority[irq] = value & 7;
+    } else if (addr_between(addr, s->enable_base, s->enable_num)) {
+        uint32_t enable_reg = (addr - s->enable_base) / 4;
+
+        s->enable[enable_reg] = value;
+    } else if (addr_between(addr, s->threshold_base, 1)) {
+        s->threshold = value & 3;
+    } else if (addr_between(addr, s->claim_base, 1)) {
+        if (s->claim == value) {
+            /* Interrupt was completed */
+            s->claim = 0;
+        }
+    }
+
+    ibex_plic_update(s);
+}
+
+static const MemoryRegionOps ibex_plic_ops = {
+    .read = ibex_plic_read,
+    .write = ibex_plic_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4
+    }
+};
+
+static void ibex_plic_irq_request(void *opaque, int irq, int level)
+{
+    IbexPlicState *s = opaque;
+
+    ibex_plic_irqs_set_pending(s, irq, level > 0);
+    ibex_plic_update(s);
+}
+
+static Property ibex_plic_properties[] = {
+    DEFINE_PROP_UINT32("num-cpus", IbexPlicState, num_cpus, 1),
+    DEFINE_PROP_UINT32("num-sources", IbexPlicState, num_sources, 80),
+
+    DEFINE_PROP_UINT32("pending-base", IbexPlicState, pending_base, 0),
+    DEFINE_PROP_UINT32("pending-num", IbexPlicState, pending_num, 3),
+
+    DEFINE_PROP_UINT32("source-base", IbexPlicState, source_base, 0x0c),
+    DEFINE_PROP_UINT32("source-num", IbexPlicState, source_num, 3),
+
+    DEFINE_PROP_UINT32("priority-base", IbexPlicState, priority_base, 0x18),
+    DEFINE_PROP_UINT32("priority-num", IbexPlicState, priority_num, 80),
+
+    DEFINE_PROP_UINT32("enable-base", IbexPlicState, enable_base, 0x200),
+    DEFINE_PROP_UINT32("enable-num", IbexPlicState, enable_num, 3),
+
+    DEFINE_PROP_UINT32("threshold-base", IbexPlicState, threshold_base, 0x20c),
+
+    DEFINE_PROP_UINT32("claim-base", IbexPlicState, claim_base, 0x210),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ibex_plic_init(Object *obj)
+{
+    IbexPlicState *s = IBEX_PLIC(obj);
+
+    memory_region_init_io(&s->mmio, obj, &ibex_plic_ops, s,
+                          TYPE_IBEX_PLIC, 0x400);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+}
+
+static void ibex_plic_realize(DeviceState *dev, Error **errp)
+{
+    IbexPlicState *s = IBEX_PLIC(dev);
+    int i;
+
+    s->pending = g_new0(uint32_t, s->pending_num);
+    s->source = g_new0(uint32_t, s->source_num);
+    s->priority = g_new0(uint32_t, s->priority_num);
+    s->enable = g_new0(uint32_t, s->enable_num);
+
+    qdev_init_gpio_in(dev, ibex_plic_irq_request, s->num_sources);
+
+    /*
+     * We can't allow the supervisor to control SEIP as this would allow the
+     * supervisor to clear a pending external interrupt which will result in
+     * a lost interrupt in the case a PLIC is attached. The SEIP bit must be
+     * hardware controlled when a PLIC is attached.
+     */
+    MachineState *ms = MACHINE(qdev_get_machine());
+    unsigned int smp_cpus = ms->smp.cpus;
+    for (i = 0; i < smp_cpus; i++) {
+        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(i));
+        if (riscv_cpu_claim_interrupts(cpu, MIP_SEIP) < 0) {
+            error_report("SEIP already claimed");
+            exit(1);
+        }
+    }
+
+    msi_nonbroken = true;
+}
+
+static void ibex_plic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = ibex_plic_reset;
+    device_class_set_props(dc, ibex_plic_properties);
+    dc->realize = ibex_plic_realize;
+}
+
+static const TypeInfo ibex_plic_info = {
+    .name          = TYPE_IBEX_PLIC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IbexPlicState),
+    .instance_init = ibex_plic_init,
+    .class_init    = ibex_plic_class_init,
+};
+
+static void ibex_plic_register_types(void)
+{
+    type_register_static(&ibex_plic_info);
+}
+
+type_init(ibex_plic_register_types)
diff --git a/include/hw/intc/ibex_plic.h b/include/hw/intc/ibex_plic.h
new file mode 100644
index 0000000000..ddc7909903
--- /dev/null
+++ b/include/hw/intc/ibex_plic.h
@@ -0,0 +1,63 @@
+/*
+ * QEMU RISC-V lowRISC Ibex PLIC
+ *
+ * Copyright (c) 2020 Western Digital
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_IBEX_PLIC_H
+#define HW_IBEX_PLIC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_IBEX_PLIC "ibex-plic"
+#define IBEX_PLIC(obj) \
+    OBJECT_CHECK(IbexPlicState, (obj), TYPE_IBEX_PLIC)
+
+typedef struct IbexPlicState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion mmio;
+
+    uint32_t *pending;
+    uint32_t *source;
+    uint32_t *priority;
+    uint32_t *enable;
+    uint32_t threshold;
+    uint32_t claim;
+
+    /* config */
+    uint32_t num_cpus;
+    uint32_t num_sources;
+
+    uint32_t pending_base;
+    uint32_t pending_num;
+
+    uint32_t source_base;
+    uint32_t source_num;
+
+    uint32_t priority_base;
+    uint32_t priority_num;
+
+    uint32_t enable_base;
+    uint32_t enable_num;
+
+    uint32_t threshold_base;
+
+    uint32_t claim_base;
+} IbexPlicState;
+
+#endif /* HW_IBEX_PLIC_H */
-- 
2.26.2

Re: [PATCH v2 6/9] hw/intc: Initial commit of lowRISC Ibex PLIC

[Philippe Mathieu-Daudé]

On 5/7/20 9:13 PM, Alistair Francis wrote:
> The Ibex core contains a PLIC that although similar to the RISC-V spec
> is not RISC-V spec compliant.
> 
> This patch implements a Ibex PLIC in a somewhat generic way.
> 
> As the current RISC-V PLIC needs tidying up, my hope is that as the Ibex
> PLIC move towards spec compliance this PLIC implementation can be
> updated until it can replace the current PLIC.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   MAINTAINERS                 |   2 +
>   hw/intc/Makefile.objs       |   1 +
>   hw/intc/ibex_plic.c         | 261 ++++++++++++++++++++++++++++++++++++
>   include/hw/intc/ibex_plic.h |  63 +++++++++
>   4 files changed, 327 insertions(+)
>   create mode 100644 hw/intc/ibex_plic.c
>   create mode 100644 include/hw/intc/ibex_plic.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d3d47564ce..f8c3cf6182 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1237,8 +1237,10 @@ L: qemu-riscv@nongnu.org
>   S: Supported
>   F: hw/riscv/opentitan.c
>   F: hw/char/ibex_uart.c
> +F: hw/intc/ibex_plic.c
>   F: include/hw/riscv/opentitan.h
>   F: include/hw/char/ibex_uart.h
> +F: include/hw/intc/ibex_plic.h
>   
>   
>   SH4 Machines
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index f726d87532..a61e6728fe 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -49,3 +49,4 @@ obj-$(CONFIG_ARM_GIC) += arm_gicv3_cpuif.o
>   obj-$(CONFIG_MIPS_CPS) += mips_gic.o
>   obj-$(CONFIG_NIOS2) += nios2_iic.o
>   obj-$(CONFIG_OMPIC) += ompic.o
> +obj-$(CONFIG_IBEX) += ibex_plic.o
> diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c
> new file mode 100644
> index 0000000000..35c52d9d16
> --- /dev/null
> +++ b/hw/intc/ibex_plic.c
> @@ -0,0 +1,261 @@
> +/*
> + * QEMU RISC-V lowRISC Ibex PLIC
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * Documentation avaliable: https://docs.opentitan.org/hw/ip/rv_plic/doc/
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * 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 "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/core/cpu.h"
> +#include "hw/boards.h"
> +#include "hw/pci/msi.h"
> +#include "target/riscv/cpu_bits.h"
> +#include "target/riscv/cpu.h"
> +#include "hw/intc/ibex_plic.h"
> +
> +static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
> +{
> +    uint32_t end = base + (num * 0x04);
> +
> +    if (addr >= base && addr < end) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void ibex_plic_irqs_set_pending(IbexPlicState *s, int irq, bool level)
> +{
> +    int pending_num = irq / 32;
> +
> +    s->pending[pending_num] |= level << (irq % 32);
> +}
> +
> +static bool ibex_plic_irqs_pending(IbexPlicState *s, uint32_t context)
> +{
> +    int i;
> +
> +    for (i = 0; i < s->pending_num; i++) {
> +        uint32_t irq_num = ctz64(s->pending[i]) + (i * 32);
> +
> +        if (!(s->pending[i] & s->enable[i])) {
> +            /* No pending and enabled IRQ */
> +            continue;
> +        }
> +
> +        if (s->priority[irq_num] > s->threshold) {
> +            if (!s->claim) {
> +                s->claim = irq_num;
> +            }
> +            return true;
> +        }
> +    }
> +
> +    return 0;

return 'false'.

> +}
> +
> +static void ibex_plic_update(IbexPlicState *s)
> +{
> +    CPUState *cpu;
> +    int level, i;
> +
> +    for (i = 0; i < s->num_cpus; i++) {
> +        cpu = qemu_get_cpu(i);
> +
> +        if (!cpu) {
> +            continue;
> +        }
> +
> +        level = ibex_plic_irqs_pending(s, 0);
> +
> +        riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MEIP, BOOL_TO_MASK(level));
> +    }
> +}
> +
> +static void ibex_plic_reset(DeviceState *dev)
> +{
> +    IbexPlicState *s = IBEX_PLIC(dev);
> +
> +    s->threshold = 0x00000000;
> +    s->claim = 0x00000000;

I haven't check the datasheet reset values, for the rest:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +}
> +
> +static uint64_t ibex_plic_read(void *opaque, hwaddr addr,
> +                               unsigned int size)
> +{
> +    IbexPlicState *s = opaque;
> +    int offset;
> +    uint32_t ret = 0;
> +
> +    if (addr_between(addr, s->pending_base, s->pending_num)) {
> +        offset = (addr - s->pending_base) / 4;
> +        ret = s->pending[offset];
> +    } else if (addr_between(addr, s->source_base, s->source_num)) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: Interrupt source mode not supported\n", __func__);
> +    } else if (addr_between(addr, s->priority_base, s->priority_num)) {
> +        offset = (addr - s->priority_base) / 4;
> +        ret = s->priority[offset];
> +    } else if (addr_between(addr, s->enable_base, s->enable_num)) {
> +        offset = (addr - s->enable_base) / 4;
> +        ret = s->enable[offset];
> +    } else if (addr_between(addr, s->threshold_base, 1)) {
> +        ret = s->threshold;
> +    } else if (addr_between(addr, s->claim_base, 1)) {
> +        int pending_num = s->claim / 32;
> +        s->pending[pending_num] &= ~(1 << (s->claim % 32));
> +
> +        ret = s->claim;
> +    }
> +
> +    return ret;
> +}
> +
> +static void ibex_plic_write(void *opaque, hwaddr addr,
> +                            uint64_t value, unsigned int size)
> +{
> +    IbexPlicState *s = opaque;
> +
> +    if (addr_between(addr, s->pending_base, s->pending_num)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Pending registers are read only\n", __func__);
> +    } else if (addr_between(addr, s->source_base, s->source_num)) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: Interrupt source mode not supported\n", __func__);
> +    } else if (addr_between(addr, s->priority_base, s->priority_num)) {
> +        uint32_t irq = ((addr - s->priority_base) >> 2) + 1;
> +        s->priority[irq] = value & 7;
> +    } else if (addr_between(addr, s->enable_base, s->enable_num)) {
> +        uint32_t enable_reg = (addr - s->enable_base) / 4;
> +
> +        s->enable[enable_reg] = value;
> +    } else if (addr_between(addr, s->threshold_base, 1)) {
> +        s->threshold = value & 3;
> +    } else if (addr_between(addr, s->claim_base, 1)) {
> +        if (s->claim == value) {
> +            /* Interrupt was completed */
> +            s->claim = 0;
> +        }
> +    }
> +
> +    ibex_plic_update(s);
> +}
> +
> +static const MemoryRegionOps ibex_plic_ops = {
> +    .read = ibex_plic_read,
> +    .write = ibex_plic_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void ibex_plic_irq_request(void *opaque, int irq, int level)
> +{
> +    IbexPlicState *s = opaque;
> +
> +    ibex_plic_irqs_set_pending(s, irq, level > 0);
> +    ibex_plic_update(s);
> +}
> +
> +static Property ibex_plic_properties[] = {
> +    DEFINE_PROP_UINT32("num-cpus", IbexPlicState, num_cpus, 1),
> +    DEFINE_PROP_UINT32("num-sources", IbexPlicState, num_sources, 80),
> +
> +    DEFINE_PROP_UINT32("pending-base", IbexPlicState, pending_base, 0),
> +    DEFINE_PROP_UINT32("pending-num", IbexPlicState, pending_num, 3),
> +
> +    DEFINE_PROP_UINT32("source-base", IbexPlicState, source_base, 0x0c),
> +    DEFINE_PROP_UINT32("source-num", IbexPlicState, source_num, 3),
> +
> +    DEFINE_PROP_UINT32("priority-base", IbexPlicState, priority_base, 0x18),
> +    DEFINE_PROP_UINT32("priority-num", IbexPlicState, priority_num, 80),
> +
> +    DEFINE_PROP_UINT32("enable-base", IbexPlicState, enable_base, 0x200),
> +    DEFINE_PROP_UINT32("enable-num", IbexPlicState, enable_num, 3),
> +
> +    DEFINE_PROP_UINT32("threshold-base", IbexPlicState, threshold_base, 0x20c),
> +
> +    DEFINE_PROP_UINT32("claim-base", IbexPlicState, claim_base, 0x210),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ibex_plic_init(Object *obj)
> +{
> +    IbexPlicState *s = IBEX_PLIC(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &ibex_plic_ops, s,
> +                          TYPE_IBEX_PLIC, 0x400);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +
> +static void ibex_plic_realize(DeviceState *dev, Error **errp)
> +{
> +    IbexPlicState *s = IBEX_PLIC(dev);
> +    int i;
> +
> +    s->pending = g_new0(uint32_t, s->pending_num);
> +    s->source = g_new0(uint32_t, s->source_num);
> +    s->priority = g_new0(uint32_t, s->priority_num);
> +    s->enable = g_new0(uint32_t, s->enable_num);
> +
> +    qdev_init_gpio_in(dev, ibex_plic_irq_request, s->num_sources);
> +
> +    /*
> +     * We can't allow the supervisor to control SEIP as this would allow the
> +     * supervisor to clear a pending external interrupt which will result in
> +     * a lost interrupt in the case a PLIC is attached. The SEIP bit must be
> +     * hardware controlled when a PLIC is attached.
> +     */
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    unsigned int smp_cpus = ms->smp.cpus;
> +    for (i = 0; i < smp_cpus; i++) {
> +        RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(i));
> +        if (riscv_cpu_claim_interrupts(cpu, MIP_SEIP) < 0) {
> +            error_report("SEIP already claimed");
> +            exit(1);
> +        }
> +    }
> +
> +    msi_nonbroken = true;
> +}
> +
> +static void ibex_plic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = ibex_plic_reset;
> +    device_class_set_props(dc, ibex_plic_properties);
> +    dc->realize = ibex_plic_realize;
> +}
> +
> +static const TypeInfo ibex_plic_info = {
> +    .name          = TYPE_IBEX_PLIC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IbexPlicState),
> +    .instance_init = ibex_plic_init,
> +    .class_init    = ibex_plic_class_init,
> +};
> +
> +static void ibex_plic_register_types(void)
> +{
> +    type_register_static(&ibex_plic_info);
> +}
> +
> +type_init(ibex_plic_register_types)
> diff --git a/include/hw/intc/ibex_plic.h b/include/hw/intc/ibex_plic.h
> new file mode 100644
> index 0000000000..ddc7909903
> --- /dev/null
> +++ b/include/hw/intc/ibex_plic.h
> @@ -0,0 +1,63 @@
> +/*
> + * QEMU RISC-V lowRISC Ibex PLIC
> + *
> + * Copyright (c) 2020 Western Digital
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_IBEX_PLIC_H
> +#define HW_IBEX_PLIC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_IBEX_PLIC "ibex-plic"
> +#define IBEX_PLIC(obj) \
> +    OBJECT_CHECK(IbexPlicState, (obj), TYPE_IBEX_PLIC)
> +
> +typedef struct IbexPlicState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion mmio;
> +
> +    uint32_t *pending;
> +    uint32_t *source;
> +    uint32_t *priority;
> +    uint32_t *enable;
> +    uint32_t threshold;
> +    uint32_t claim;
> +
> +    /* config */
> +    uint32_t num_cpus;
> +    uint32_t num_sources;
> +
> +    uint32_t pending_base;
> +    uint32_t pending_num;
> +
> +    uint32_t source_base;
> +    uint32_t source_num;
> +
> +    uint32_t priority_base;
> +    uint32_t priority_num;
> +
> +    uint32_t enable_base;
> +    uint32_t enable_num;
> +
> +    uint32_t threshold_base;
> +
> +    uint32_t claim_base;
> +} IbexPlicState;
> +
> +#endif /* HW_IBEX_PLIC_H */
> 

Re: [PATCH v2 6/9] hw/intc: Initial commit of lowRISC Ibex PLIC

[Alistair Francis]

On Thu, May 14, 2020 at 11:40 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 5/7/20 9:13 PM, Alistair Francis wrote:
> > The Ibex core contains a PLIC that although similar to the RISC-V spec
> > is not RISC-V spec compliant.
> >
> > This patch implements a Ibex PLIC in a somewhat generic way.
> >
> > As the current RISC-V PLIC needs tidying up, my hope is that as the Ibex
> > PLIC move towards spec compliance this PLIC implementation can be
> > updated until it can replace the current PLIC.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   MAINTAINERS                 |   2 +
> >   hw/intc/Makefile.objs       |   1 +
> >   hw/intc/ibex_plic.c         | 261 ++++++++++++++++++++++++++++++++++++
> >   include/hw/intc/ibex_plic.h |  63 +++++++++
> >   4 files changed, 327 insertions(+)
> >   create mode 100644 hw/intc/ibex_plic.c
> >   create mode 100644 include/hw/intc/ibex_plic.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d3d47564ce..f8c3cf6182 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1237,8 +1237,10 @@ L: qemu-riscv@nongnu.org
> >   S: Supported
> >   F: hw/riscv/opentitan.c
> >   F: hw/char/ibex_uart.c
> > +F: hw/intc/ibex_plic.c
> >   F: include/hw/riscv/opentitan.h
> >   F: include/hw/char/ibex_uart.h
> > +F: include/hw/intc/ibex_plic.h
> >
> >
> >   SH4 Machines
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index f726d87532..a61e6728fe 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -49,3 +49,4 @@ obj-$(CONFIG_ARM_GIC) += arm_gicv3_cpuif.o
> >   obj-$(CONFIG_MIPS_CPS) += mips_gic.o
> >   obj-$(CONFIG_NIOS2) += nios2_iic.o
> >   obj-$(CONFIG_OMPIC) += ompic.o
> > +obj-$(CONFIG_IBEX) += ibex_plic.o
> > diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c
> > new file mode 100644
> > index 0000000000..35c52d9d16
> > --- /dev/null
> > +++ b/hw/intc/ibex_plic.c
> > @@ -0,0 +1,261 @@
> > +/*
> > + * QEMU RISC-V lowRISC Ibex PLIC
> > + *
> > + * Copyright (c) 2020 Western Digital
> > + *
> > + * Documentation avaliable: https://docs.opentitan.org/hw/ip/rv_plic/doc/
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2 or later, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * 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 "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/core/cpu.h"
> > +#include "hw/boards.h"
> > +#include "hw/pci/msi.h"
> > +#include "target/riscv/cpu_bits.h"
> > +#include "target/riscv/cpu.h"
> > +#include "hw/intc/ibex_plic.h"
> > +
> > +static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
> > +{
> > +    uint32_t end = base + (num * 0x04);
> > +
> > +    if (addr >= base && addr < end) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +static void ibex_plic_irqs_set_pending(IbexPlicState *s, int irq, bool level)
> > +{
> > +    int pending_num = irq / 32;
> > +
> > +    s->pending[pending_num] |= level << (irq % 32);
> > +}
> > +
> > +static bool ibex_plic_irqs_pending(IbexPlicState *s, uint32_t context)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < s->pending_num; i++) {
> > +        uint32_t irq_num = ctz64(s->pending[i]) + (i * 32);
> > +
> > +        if (!(s->pending[i] & s->enable[i])) {
> > +            /* No pending and enabled IRQ */
> > +            continue;
> > +        }
> > +
> > +        if (s->priority[irq_num] > s->threshold) {
> > +            if (!s->claim) {
> > +                s->claim = irq_num;
> > +            }
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return 0;
>
> return 'false'.

Fixed.

>
> > +}
> > +
> > +static void ibex_plic_update(IbexPlicState *s)
> > +{
> > +    CPUState *cpu;
> > +    int level, i;
> > +
> > +    for (i = 0; i < s->num_cpus; i++) {
> > +        cpu = qemu_get_cpu(i);
> > +
> > +        if (!cpu) {
> > +            continue;
> > +        }
> > +
> > +        level = ibex_plic_irqs_pending(s, 0);
> > +
> > +        riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MEIP, BOOL_TO_MASK(level));
> > +    }
> > +}
> > +
> > +static void ibex_plic_reset(DeviceState *dev)
> > +{
> > +    IbexPlicState *s = IBEX_PLIC(dev);
> > +
> > +    s->threshold = 0x00000000;
> > +    s->claim = 0x00000000;
>
> I haven't check the datasheet reset values, for the rest:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks for reviewing these :)

Alistair

[PATCH v2 7/9] riscv/opentitan: Connect the PLIC device

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/opentitan.c         | 19 +++++++++++++++++--
 include/hw/riscv/opentitan.h |  3 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index c00f0720ab..3926321d8c 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -25,6 +25,7 @@
 #include "hw/misc/unimp.h"
 #include "hw/riscv/boot.h"
 #include "exec/address-spaces.h"
+#include "sysemu/sysemu.h"
 
 static const struct MemmapEntry {
     hwaddr base;
@@ -92,6 +93,9 @@ static void riscv_lowrisc_ibex_soc_init(Object *obj)
     object_initialize_child(obj, "cpus", &s->cpus,
                             sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
                             &error_abort, NULL);
+
+    sysbus_init_child_obj(obj, "plic", &s->plic,
+                          sizeof(s->plic), TYPE_IBEX_PLIC);
 }
 
 static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
@@ -100,6 +104,9 @@ static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
     MachineState *ms = MACHINE(qdev_get_machine());
     LowRISCIbexSoCState *s = RISCV_IBEX_SOC(dev_soc);
     MemoryRegion *sys_mem = get_system_memory();
+    DeviceState *dev;
+    SysBusDevice *busdev;
+    Error *err = NULL;
 
     object_property_set_str(OBJECT(&s->cpus), ms->cpu_type, "cpu-type",
                             &error_abort);
@@ -120,6 +127,16 @@ static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base,
                                 &s->flash_mem);
 
+    /* PLIC */
+    dev = DEVICE(&s->plic);
+    object_property_set_bool(OBJECT(&s->plic), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(busdev, 0, memmap[IBEX_PLIC].base);
+
     create_unimplemented_device("riscv.lowrisc.ibex.uart",
         memmap[IBEX_UART].base, memmap[IBEX_UART].size);
     create_unimplemented_device("riscv.lowrisc.ibex.gpio",
@@ -134,8 +151,6 @@ static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
         memmap[IBEX_AES].base, memmap[IBEX_AES].size);
     create_unimplemented_device("riscv.lowrisc.ibex.hmac",
         memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size);
-    create_unimplemented_device("riscv.lowrisc.ibex.plic",
-        memmap[IBEX_PLIC].base, memmap[IBEX_PLIC].size);
     create_unimplemented_device("riscv.lowrisc.ibex.pinmux",
         memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size);
     create_unimplemented_device("riscv.lowrisc.ibex.alert_handler",
diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
index 15a3d87ed0..8d6a09b696 100644
--- a/include/hw/riscv/opentitan.h
+++ b/include/hw/riscv/opentitan.h
@@ -20,6 +20,7 @@
 #define HW_OPENTITAN_H
 
 #include "hw/riscv/riscv_hart.h"
+#include "hw/intc/ibex_plic.h"
 
 #define TYPE_RISCV_IBEX_SOC "riscv.lowrisc.ibex.soc"
 #define RISCV_IBEX_SOC(obj) \
@@ -31,6 +32,8 @@ typedef struct LowRISCIbexSoCState {
 
     /*< public >*/
     RISCVHartArrayState cpus;
+    IbexPlicState plic;
+
     MemoryRegion flash_mem;
     MemoryRegion rom;
 } LowRISCIbexSoCState;
-- 
2.26.2

Re: [PATCH v2 7/9] riscv/opentitan: Connect the PLIC device

[Bin Meng]

On Fri, May 8, 2020 at 3:22 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/opentitan.c         | 19 +++++++++++++++++--
>  include/hw/riscv/opentitan.h |  3 +++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

[PATCH v2 8/9] riscv/opentitan: Connect the UART device

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/opentitan.c         | 24 ++++++++++++++++++++++--
 include/hw/riscv/opentitan.h | 13 +++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 3926321d8c..a6c0b949ca 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -96,6 +96,9 @@ static void riscv_lowrisc_ibex_soc_init(Object *obj)
 
     sysbus_init_child_obj(obj, "plic", &s->plic,
                           sizeof(s->plic), TYPE_IBEX_PLIC);
+
+    sysbus_init_child_obj(obj, "uart", &s->uart,
+                          sizeof(s->uart), TYPE_IBEX_UART);
 }
 
 static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
@@ -137,8 +140,25 @@ static void riscv_lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, memmap[IBEX_PLIC].base);
 
-    create_unimplemented_device("riscv.lowrisc.ibex.uart",
-        memmap[IBEX_UART].base, memmap[IBEX_UART].size);
+    /* UART */
+    dev = DEVICE(&(s->uart));
+    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(busdev, 0, memmap[IBEX_UART].base);
+    sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(DEVICE(&s->plic),
+                       IBEX_UART_TX_WATERMARK_IRQ));
+    sysbus_connect_irq(busdev, 1, qdev_get_gpio_in(DEVICE(&s->plic),
+                       IBEX_UART_RX_WATERMARK_IRQ));
+    sysbus_connect_irq(busdev, 2, qdev_get_gpio_in(DEVICE(&s->plic),
+                       IBEX_UART_TX_EMPTY_IRQ));
+    sysbus_connect_irq(busdev, 3, qdev_get_gpio_in(DEVICE(&s->plic),
+                       IBEX_UART_RX_OVERFLOW_IRQ));
+
     create_unimplemented_device("riscv.lowrisc.ibex.gpio",
         memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size);
     create_unimplemented_device("riscv.lowrisc.ibex.spi",
diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
index 8d6a09b696..825a3610bc 100644
--- a/include/hw/riscv/opentitan.h
+++ b/include/hw/riscv/opentitan.h
@@ -21,6 +21,7 @@
 
 #include "hw/riscv/riscv_hart.h"
 #include "hw/intc/ibex_plic.h"
+#include "hw/char/ibex_uart.h"
 
 #define TYPE_RISCV_IBEX_SOC "riscv.lowrisc.ibex.soc"
 #define RISCV_IBEX_SOC(obj) \
@@ -33,6 +34,7 @@ typedef struct LowRISCIbexSoCState {
     /*< public >*/
     RISCVHartArrayState cpus;
     IbexPlicState plic;
+    IbexUartState uart;
 
     MemoryRegion flash_mem;
     MemoryRegion rom;
@@ -63,4 +65,15 @@ enum {
     IBEX_USBDEV,
 };
 
+enum {
+    IBEX_UART_RX_PARITY_ERR_IRQ = 0x28,
+    IBEX_UART_RX_TIMEOUT_IRQ = 0x27,
+    IBEX_UART_RX_BREAK_ERR_IRQ = 0x26,
+    IBEX_UART_RX_FRAME_ERR_IRQ = 0x25,
+    IBEX_UART_RX_OVERFLOW_IRQ = 0x24,
+    IBEX_UART_TX_EMPTY_IRQ = 0x23,
+    IBEX_UART_RX_WATERMARK_IRQ = 0x22,
+    IBEX_UART_TX_WATERMARK_IRQ = 0x21
+};
+
 #endif
-- 
2.26.2

Re: [PATCH v2 8/9] riscv/opentitan: Connect the UART device

[Bin Meng]

On Fri, May 8, 2020 at 3:24 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/opentitan.c         | 24 ++++++++++++++++++++++--
>  include/hw/riscv/opentitan.h | 13 +++++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>

[PATCH v2 9/9] target/riscv: Use a smaller guess size for no-MMU PMP

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/pmp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 0e6b640fbd..5aba4d13ea 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -233,12 +233,21 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
         return true;
     }
 
-    /*
-     * if size is unknown (0), assume that all bytes
-     * from addr to the end of the page will be accessed.
-     */
     if (size == 0) {
-        pmp_size = -(addr | TARGET_PAGE_MASK);
+        if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
+            /*
+             * if size is unknown (0), assume that all bytes
+             * from addr to the end of the page will be accessed.
+             */
+            pmp_size = -(addr | TARGET_PAGE_MASK);
+        } else {
+            /*
+             * If size is unknown (0) and we don't have an MMU,
+             * just guess the size as the xlen as we don't want to
+             * access an entire page worth.
+             */
+            pmp_size = sizeof(target_ulong);
+        }
     } else {
         pmp_size = size;
     }
-- 
2.26.2

Re: [PATCH v2 9/9] target/riscv: Use a smaller guess size for no-MMU PMP

[Bin Meng]

On Fri, May 8, 2020 at 3:29 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/pmp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 0e6b640fbd..5aba4d13ea 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -233,12 +233,21 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>          return true;
>      }
>
> -    /*
> -     * if size is unknown (0), assume that all bytes
> -     * from addr to the end of the page will be accessed.
> -     */
>      if (size == 0) {
> -        pmp_size = -(addr | TARGET_PAGE_MASK);
> +        if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> +            /*
> +             * if size is unknown (0), assume that all bytes
> +             * from addr to the end of the page will be accessed.
> +             */
> +            pmp_size = -(addr | TARGET_PAGE_MASK);
> +        } else {
> +            /*
> +             * If size is unknown (0) and we don't have an MMU,
> +             * just guess the size as the xlen as we don't want to
> +             * access an entire page worth.
> +             */

It looks the comment does not match the code logic. This else branch
is the MMU branch.

> +            pmp_size = sizeof(target_ulong);
> +        }
>      } else {
>          pmp_size = size;
>      }
> --

Regards,
Bin

Re: [PATCH v2 0/9] RISC-V Add the OpenTitan Machine

[Alistair Francis]

On Thu, May 7, 2020 at 12:21 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> OpenTitan is an open source silicon Root of Trust (RoT) project. This
> series adds initial support for the OpenTitan machine to QEMU.
>
> This series add the Ibex CPU to the QEMU RISC-V target. It then adds the
> OpenTitan machine, the Ibex UART and the Ibex PLIC.
>
> The UART has been tested sending and receiving data.
>
> With this series QEMU can boot the OpenTitan ROM, Tock OS and a Tock
> userspace app.
>
> The Ibex PLIC is similar to the RISC-V PLIC (and is based on the QEMU
> implementation) with some differences. The hope is that the Ibex PLIC
> will converge to follow the RISC-V spec. As that happens I want to
> update the QEMU Ibex PLIC and hopefully eventually replace the current
> PLIC as the implementation is a little overlay complex.
>
> For more details on OpenTitan, see here: https://docs.opentitan.org/

Ping!

+ Some people who might be able to review this series (or at least ack).

I'll give it another week and if I don't hear anything I'll merge it.

Alistair

>
> v2:
>  - Rebase on master
>  - Get uart receive working
>
> Alistair Francis (9):
>   riscv/boot: Add a missing header include
>   target/riscv: Don't overwrite the reset vector
>   target/riscv: Add the lowRISC Ibex CPU
>   riscv: Initial commit of OpenTitan machine
>   hw/char: Initial commit of Ibex UART
>   hw/intc: Initial commit of lowRISC Ibex PLIC
>   riscv/opentitan: Connect the PLIC device
>   riscv/opentitan: Connect the UART device
>   target/riscv: Use a smaller guess size for no-MMU PMP
>
>  MAINTAINERS                         |  14 +
>  default-configs/riscv32-softmmu.mak |   1 +
>  default-configs/riscv64-softmmu.mak |  11 +-
>  hw/char/Makefile.objs               |   1 +
>  hw/char/ibex_uart.c                 | 490 ++++++++++++++++++++++++++++
>  hw/intc/Makefile.objs               |   1 +
>  hw/intc/ibex_plic.c                 | 261 +++++++++++++++
>  hw/riscv/Kconfig                    |   9 +
>  hw/riscv/Makefile.objs              |   1 +
>  hw/riscv/opentitan.c                | 204 ++++++++++++
>  include/hw/char/ibex_uart.h         | 110 +++++++
>  include/hw/intc/ibex_plic.h         |  63 ++++
>  include/hw/riscv/boot.h             |   1 +
>  include/hw/riscv/opentitan.h        |  79 +++++
>  target/riscv/cpu.c                  |  30 +-
>  target/riscv/cpu.h                  |   1 +
>  target/riscv/pmp.c                  |  19 +-
>  17 files changed, 1281 insertions(+), 15 deletions(-)
>  create mode 100644 hw/char/ibex_uart.c
>  create mode 100644 hw/intc/ibex_plic.c
>  create mode 100644 hw/riscv/opentitan.c
>  create mode 100644 include/hw/char/ibex_uart.h
>  create mode 100644 include/hw/intc/ibex_plic.h
>  create mode 100644 include/hw/riscv/opentitan.h
>
> --
> 2.26.2
>