[Qemu-devel] [PATCH v1 0/2] riscv: htif fixes

[Qemu-devel] [PATCH v1 0/2] riscv: htif fixes

[KONRAD Frederic]

Hi all,
 
Here are some fixes for the HTIF device.

I saw some changing behavior since I use qemu upstream instead of the RISC-V
fork because the htif can't be based to 0 anymore. The first patch address
that issue.

The second patch fixes the priority of the htif subregion as I don't think we
guarantee which region get accessed when two subregions have the same
priority.

Thanks,
Fred

KONRAD Frederic (2):
  riscv: spike: allow base == 0
  riscv: htif: increase the priority of the htif subregion

 hw/riscv/riscv_htif.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v1 1/2] riscv: spike: allow base == 0

[KONRAD Frederic]

The sanity check on base doesn't allow htif to be mapped @0. Check if the
symbol exists instead so we can map it where we want.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 hw/riscv/riscv_htif.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
index 3e17f30..6e687f2 100644
--- a/hw/riscv/riscv_htif.c
+++ b/hw/riscv/riscv_htif.c
@@ -41,17 +41,20 @@
     } while (0)
 
 static uint64_t fromhost_addr, tohost_addr;
+static int address_symbol_set;
 
 void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
-    uint64_t st_size)
+                          uint64_t st_size)
 {
     if (strcmp("fromhost", st_name) == 0) {
+        address_symbol_set = 1;
         fromhost_addr = st_value;
         if (st_size != 8) {
             error_report("HTIF fromhost must be 8 bytes");
             exit(1);
         }
     } else if (strcmp("tohost", st_name) == 0) {
+        address_symbol_set = 1;
         tohost_addr = st_value;
         if (st_size != 8) {
             error_report("HTIF tohost must be 8 bytes");
@@ -248,7 +251,7 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
     qemu_chr_fe_init(&s->chr, chr, &error_abort);
     qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
         htif_be_change, s, NULL, true);
-    if (base) {
+    if (address_symbol_set) {
         memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
                             TYPE_HTIF_UART, size);
         memory_region_add_subregion(address_space, base, &s->mmio);
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 2/2] riscv: htif: increase the priority of the htif subregion

[KONRAD Frederic]

The htif device is supposed to be mapped over an other subregion. So increase
its priority to one to avoid any conflict.

Here is the output of info mtree:

Before:
(qemu) info mtree
 address-space: memory
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
     0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
     0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
     0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram

 address-space: I/O
   0000000000000000-000000000000ffff (prio 0, i/o): io

 address-space: cpu-memory-0
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
     0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
     0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
     0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram

After:
 (qemu) info mtree
 address-space: memory
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
     0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
     0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
     0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram

 address-space: I/O
   0000000000000000-000000000000ffff (prio 0, i/o): io

 address-space: cpu-memory-0
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
     0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
     0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
     0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 hw/riscv/riscv_htif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
index 6e687f2..48e5452 100644
--- a/hw/riscv/riscv_htif.c
+++ b/hw/riscv/riscv_htif.c
@@ -253,8 +253,9 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
         htif_be_change, s, NULL, true);
     if (address_symbol_set) {
         memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
-                            TYPE_HTIF_UART, size);
-        memory_region_add_subregion(address_space, base, &s->mmio);
+                              TYPE_HTIF_UART, size);
+        memory_region_add_subregion_overlap(address_space, base,
+                                            &s->mmio, 1);
     }
 
     return s;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v1 2/2] riscv: htif: increase the priority of the htif subregion

[Michael Clark]

On Wed, Apr 18, 2018 at 10:16 PM, KONRAD Frederic <
frederic.konrad@adacore.com> wrote:

> The htif device is supposed to be mapped over an other subregion. So
> increase
> its priority to one to avoid any conflict.
>
> Here is the output of info mtree:
>
> Before:
> (qemu) info mtree
>  address-space: memory
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
>      0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>      0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>      0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>
>  address-space: I/O
>    0000000000000000-000000000000ffff (prio 0, i/o): io
>
>  address-space: cpu-memory-0
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
>      0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>      0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>      0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>
> After:
>  (qemu) info mtree
>  address-space: memory
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
>      0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>      0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>      0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>
>  address-space: I/O
>    0000000000000000-000000000000ffff (prio 0, i/o): io
>
>  address-space: cpu-memory-0
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
>      0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>      0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>      0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>

Reviewed-by: Michael Clark <mjc@sifive.com>

BTW if you like I can incorporate these into the riscv tree. e.g. here,
feel free to make a PR in GitHub:

https://github.com/riscv/riscv-qemu/tree/qemu-2.13-for-upstream

(however the riscv.org tree has a lot of changes in it so you might be
better off getting these changes merged in upstream as the review backlog
is large)

It's just that i'd like to keep the trees in sync with upstream while at
the same time incorporating as many fixes as I can in the riscv tree
(before they are in upstream) so that the riscv tree is complete for users
who want to pull a branch that includes the outstanding riscv fixes. I have
scripts that can merge mutliple branches into 'riscv-all' in the riscv
github so if you make a PR against the riscv github I can merge them in
that tree, and drop them if they get accepted upstream independently. I'm
pulling from master frequently so its fine either way.

Just i'd like the riscv tree to accumulate all of the fixes that haven't
yet made it upstream.

---
>  hw/riscv/riscv_htif.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
> index 6e687f2..48e5452 100644
> --- a/hw/riscv/riscv_htif.c
> +++ b/hw/riscv/riscv_htif.c
> @@ -253,8 +253,9 @@ HTIFState *htif_mm_init(MemoryRegion *address_space,
> MemoryRegion *main_mem,
>          htif_be_change, s, NULL, true);
>      if (address_symbol_set) {
>          memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
> -                            TYPE_HTIF_UART, size);
> -        memory_region_add_subregion(address_space, base, &s->mmio);
> +                              TYPE_HTIF_UART, size);
> +        memory_region_add_subregion_overlap(address_space, base,
> +                                            &s->mmio, 1);
>      }
>
>      return s;
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH v1 2/2] riscv: htif: increase the priority of the htif subregion

[KONRAD Frederic]

On 04/20/2018 02:57 AM, Michael Clark wrote:
> On Wed, Apr 18, 2018 at 10:16 PM, KONRAD Frederic <
> frederic.konrad@adacore.com> wrote:
> 
>> The htif device is supposed to be mapped over an other subregion. So
>> increase
>> its priority to one to avoid any conflict.
>>
>> Here is the output of info mtree:
>>
>> Before:
>> (qemu) info mtree
>>   address-space: memory
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
>>       0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>>       0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>>       0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>>
>>   address-space: I/O
>>     0000000000000000-000000000000ffff (prio 0, i/o): io
>>
>>   address-space: cpu-memory-0
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000000000f (prio 0, i/o): riscv.htif.uart
>>       0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>>       0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>>       0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>>
>> After:
>>   (qemu) info mtree
>>   address-space: memory
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
>>       0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>>       0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>>       0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>>
>>   address-space: I/O
>>     0000000000000000-000000000000ffff (prio 0, i/o): io
>>
>>   address-space: cpu-memory-0
>>     0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>       0000000000000000-000000000000000f (prio 1, i/o): riscv.htif.uart
>>       0000000000000000-0000000000011fff (prio 0, ram): riscv.spike.bootrom
>>       0000000002000000-000000000200ffff (prio 0, i/o): riscv.sifive.clint
>>       0000000080000000-0000000087ffffff (prio 0, ram): riscv.spike.ram
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>>
> 
> Reviewed-by: Michael Clark <mjc@sifive.com>
> 
> BTW if you like I can incorporate these into the riscv tree. e.g. here,
> feel free to make a PR in GitHub:
> 
> https://github.com/riscv/riscv-qemu/tree/qemu-2.13-for-upstream
> 
> (however the riscv.org tree has a lot of changes in it so you might be
> better off getting these changes merged in upstream as the review backlog
> is large)
> 
> It's just that i'd like to keep the trees in sync with upstream while at
> the same time incorporating as many fixes as I can in the riscv tree
> (before they are in upstream) so that the riscv tree is complete for users
> who want to pull a branch that includes the outstanding riscv fixes. I have
> scripts that can merge mutliple branches into 'riscv-all' in the riscv
> github so if you make a PR against the riscv github I can merge them in
> that tree, and drop them if they get accepted upstream independently. I'm
> pulling from master frequently so its fine either way.
> 
> Just i'd like the riscv tree to accumulate all of the fixes that haven't
> yet made it upstream.

Hi Michael,

Ok I'll respin on the list then and try to do a PR on GitHub.

Thanks,
Fred

> 
> ---
>>   hw/riscv/riscv_htif.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
>> index 6e687f2..48e5452 100644
>> --- a/hw/riscv/riscv_htif.c
>> +++ b/hw/riscv/riscv_htif.c
>> @@ -253,8 +253,9 @@ HTIFState *htif_mm_init(MemoryRegion *address_space,
>> MemoryRegion *main_mem,
>>           htif_be_change, s, NULL, true);
>>       if (address_symbol_set) {
>>           memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
>> -                            TYPE_HTIF_UART, size);
>> -        memory_region_add_subregion(address_space, base, &s->mmio);
>> +                              TYPE_HTIF_UART, size);
>> +        memory_region_add_subregion_overlap(address_space, base,
>> +                                            &s->mmio, 1);
>>       }
>>
>>       return s;
>> --
>> 1.8.3.1
>>
>>
>