[Qemu-devel] [PATCH 0/2] aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

[Qemu-devel] [PATCH 0/2] aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

[Philippe Mathieu-Daudé]

The diffstat prooves it :)

Philippe Mathieu-Daudé (2):
  hw/arm/aspeed: directly map the serial device to the system address space
  hw/arm/aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

 include/hw/arm/aspeed_soc.h |  1 -
 hw/arm/aspeed_soc.c         | 34 ++++------------------------------
 2 files changed, 4 insertions(+), 31 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index c83b7e207b..a786750e14 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     /* UART - attach an 8250 to the IO space as our UART5 */
     if (serial_hds[0]) {
         qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
-        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
+        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
                        uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
     }
 
-- 
2.16.1

Re: [Qemu-devel] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Peter Maydell]

On 8 February 2018 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/aspeed_soc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index c83b7e207b..a786750e14 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>      /* UART - attach an 8250 to the IO space as our UART5 */
>      if (serial_hds[0]) {
>          qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
> -        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
> +        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
>                         uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
>      }

Is this a bug fix? It certainly changes behaviour, which
suggests that a fuller commit message would be useful.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Philippe Mathieu-Daudé]

On 02/08/2018 02:30 PM, Peter Maydell wrote:
> On 8 February 2018 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/aspeed_soc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index c83b7e207b..a786750e14 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>      /* UART - attach an 8250 to the IO space as our UART5 */
>>      if (serial_hds[0]) {
>>          qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
>> -        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
>> +        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
>>                         uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
>>      }
> 
> Is this a bug fix? It certainly changes behaviour, which
> suggests that a fuller commit message would be useful.

This patch is buggy indeed, using system_memory the serial address
should be ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE.

Thanks!

Phil.

Re: [Qemu-devel] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

On Thu, 2018-02-08 at 14:40 -0300, Philippe Mathieu-Daudé wrote:
> On 02/08/2018 02:30 PM, Peter Maydell wrote:
> > On 8 February 2018 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > >  hw/arm/aspeed_soc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > > index c83b7e207b..a786750e14 100644
> > > --- a/hw/arm/aspeed_soc.c
> > > +++ b/hw/arm/aspeed_soc.c
> > > @@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> > >      /* UART - attach an 8250 to the IO space as our UART5 */
> > >      if (serial_hds[0]) {
> > >          qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
> > > -        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
> > > +        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
> > >                         uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
> > >      }
> > 
> > Is this a bug fix? It certainly changes behaviour, which
> > suggests that a fuller commit message would be useful.
> 
> This patch is buggy indeed, using system_memory the serial address
> should be ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE.
> 

If you'd like to test, you can grab images from here:

https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/

For example (without the patch):

    $ wget     https://openpower.xyz/job/openbmc-build/1240/distro=ubuntu,target=romulus/artifact/deploy/images/romulus/image-bmc
...
$ qemu-system-arm -M romulus-bmc -m 512 -drive file=image-bmc,if=mtd,format=raw -nographic


U-Boot 2016.07 (Jan 25 2018 - 16:31:27 +0000)   

       Watchdog enabled                         
DRAM:  496 MiB                                  
Flash: 32 MiB                                   
*** Warning - bad CRC, using default environment

In:    serial                                   
Out:   serial                                   
Err:   serial                                   
Net:   aspeednic#0                              
Error: aspeednic#0 address not set.             

Hit any key to stop autoboot:  0                
## Loading kernel from FIT Image at 20080000 ...
...

And yeah, this patch breaks output.

Thanks for the cleanup though.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

On 02/08/2018 09:40 PM, Andrew Jeffery wrote:
> On Thu, 2018-02-08 at 14:40 -0300, Philippe Mathieu-Daudé wrote:
>> On 02/08/2018 02:30 PM, Peter Maydell wrote:
>>> On 8 February 2018 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/arm/aspeed_soc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>>>> index c83b7e207b..a786750e14 100644
>>>> --- a/hw/arm/aspeed_soc.c
>>>> +++ b/hw/arm/aspeed_soc.c
>>>> @@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>>>      /* UART - attach an 8250 to the IO space as our UART5 */
>>>>      if (serial_hds[0]) {
>>>>          qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
>>>> -        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
>>>> +        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
>>>>                         uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
>>>>      }
>>>
>>> Is this a bug fix? It certainly changes behaviour, which
>>> suggests that a fuller commit message would be useful.
>>
>> This patch is buggy indeed, using system_memory the serial address
>> should be ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE.
>>
> 
> If you'd like to test, you can grab images from here:
> 
> https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/
> 
> For example (without the patch):
> 
>     $ wget     https://openpower.xyz/job/openbmc-build/1240/distro=ubuntu,target=romulus/artifact/deploy/images/romulus/image-bmc

Thanks for this useful link!

> ...
> $ qemu-system-arm -M romulus-bmc -m 512 -drive file=image-bmc,if=mtd,format=raw -nographic

good candidate for an Avocado / pyexpect test...

> 
> 
> U-Boot 2016.07 (Jan 25 2018 - 16:31:27 +0000)   
> 
>        Watchdog enabled                         
> DRAM:  496 MiB                                  
> Flash: 32 MiB                                   
> *** Warning - bad CRC, using default environment
> 
> In:    serial                                   
> Out:   serial                                   
> Err:   serial                                   
> Net:   aspeednic#0                              
> Error: aspeednic#0 address not set.             
> 
> Hit any key to stop autoboot:  0                
> ## Loading kernel from FIT Image at 20080000 ...
> ...
> 
> And yeah, this patch breaks output.

Sorry I didn't care enough testing this series :|

> 
> Thanks for the cleanup though.
> 
> Andrew
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/2] hw/arm/aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/aspeed_soc.h |  1 -
 hw/arm/aspeed_soc.c         | 32 +++-----------------------------
 2 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index f26914a2b9..11ec0179db 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -31,7 +31,6 @@ typedef struct AspeedSoCState {
 
     /*< public >*/
     ARMCPU cpu;
-    MemoryRegion iomem;
     MemoryRegion sram;
     AspeedVICState vic;
     AspeedTimerCtrlState timerctrl;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index a786750e14..3d21128d47 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -15,6 +15,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "exec/address-spaces.h"
+#include "hw/misc/unimp.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/char/serial.h"
 #include "qemu/log.h"
@@ -99,31 +100,6 @@ static const AspeedSoCInfo aspeed_socs[] = {
     },
 };
 
-/*
- * IO handlers: simply catch any reads/writes to IO addresses that aren't
- * handled by a device mapping.
- */
-
-static uint64_t aspeed_soc_io_read(void *p, hwaddr offset, unsigned size)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
-                  __func__, offset, size);
-    return 0;
-}
-
-static void aspeed_soc_io_write(void *opaque, hwaddr offset, uint64_t value,
-                unsigned size)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
-                  __func__, offset, value, size);
-}
-
-static const MemoryRegionOps aspeed_soc_io_ops = {
-    .read = aspeed_soc_io_read,
-    .write = aspeed_soc_io_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static void aspeed_soc_init(Object *obj)
 {
     AspeedSoCState *s = ASPEED_SOC(obj);
@@ -199,10 +175,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     Error *err = NULL, *local_err = NULL;
 
     /* IO space */
-    memory_region_init_io(&s->iomem, NULL, &aspeed_soc_io_ops, NULL,
-            "aspeed_soc.io", ASPEED_SOC_IOMEM_SIZE);
-    memory_region_add_subregion_overlap(get_system_memory(),
-                                        ASPEED_SOC_IOMEM_BASE, &s->iomem, -1);
+    create_unimplemented_device("aspeed_soc.io",
+                                ASPEED_SOC_IOMEM_BASE, ASPEED_SOC_IOMEM_SIZE);
 
     /* CPU */
     object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
-- 
2.16.1

Re: [Qemu-devel] [PATCH 2/2] hw/arm/aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

[Cédric Le Goater]

On 02/08/2018 06:22 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

This looks like a good idea. (Have you actually tried it ? 
just asking)

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  include/hw/arm/aspeed_soc.h |  1 -
>  hw/arm/aspeed_soc.c         | 32 +++-----------------------------
>  2 files changed, 3 insertions(+), 30 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index f26914a2b9..11ec0179db 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -31,7 +31,6 @@ typedef struct AspeedSoCState {
>  
>      /*< public >*/
>      ARMCPU cpu;
> -    MemoryRegion iomem;
>      MemoryRegion sram;
>      AspeedVICState vic;
>      AspeedTimerCtrlState timerctrl;
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index a786750e14..3d21128d47 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -15,6 +15,7 @@
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "exec/address-spaces.h"
> +#include "hw/misc/unimp.h"
>  #include "hw/arm/aspeed_soc.h"
>  #include "hw/char/serial.h"
>  #include "qemu/log.h"
> @@ -99,31 +100,6 @@ static const AspeedSoCInfo aspeed_socs[] = {
>      },
>  };
>  
> -/*
> - * IO handlers: simply catch any reads/writes to IO addresses that aren't
> - * handled by a device mapping.
> - */
> -
> -static uint64_t aspeed_soc_io_read(void *p, hwaddr offset, unsigned size)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
> -                  __func__, offset, size);
> -    return 0;
> -}
> -
> -static void aspeed_soc_io_write(void *opaque, hwaddr offset, uint64_t value,
> -                unsigned size)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> -                  __func__, offset, value, size);
> -}
> -
> -static const MemoryRegionOps aspeed_soc_io_ops = {
> -    .read = aspeed_soc_io_read,
> -    .write = aspeed_soc_io_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
>  static void aspeed_soc_init(Object *obj)
>  {
>      AspeedSoCState *s = ASPEED_SOC(obj);
> @@ -199,10 +175,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>      Error *err = NULL, *local_err = NULL;
>  
>      /* IO space */
> -    memory_region_init_io(&s->iomem, NULL, &aspeed_soc_io_ops, NULL,
> -            "aspeed_soc.io", ASPEED_SOC_IOMEM_SIZE);
> -    memory_region_add_subregion_overlap(get_system_memory(),
> -                                        ASPEED_SOC_IOMEM_BASE, &s->iomem, -1);
> +    create_unimplemented_device("aspeed_soc.io",
> +                                ASPEED_SOC_IOMEM_BASE, ASPEED_SOC_IOMEM_SIZE);
>  
>      /* CPU */
>      object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/aspeed: simplify using the 'unimplemented device' for aspeed_soc.io

[Philippe Mathieu-Daudé]

On 02/08/2018 03:14 PM, Cédric Le Goater wrote:
> On 02/08/2018 06:22 PM, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This looks like a good idea. (Have you actually tried it ? 
> just asking)

v1 obviously not :S v2 yes!

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks :)

> 
> Thanks,
> 
> C. 
> 
>> ---
>>  include/hw/arm/aspeed_soc.h |  1 -
>>  hw/arm/aspeed_soc.c         | 32 +++-----------------------------
>>  2 files changed, 3 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>> index f26914a2b9..11ec0179db 100644
>> --- a/include/hw/arm/aspeed_soc.h
>> +++ b/include/hw/arm/aspeed_soc.h
>> @@ -31,7 +31,6 @@ typedef struct AspeedSoCState {
>>  
>>      /*< public >*/
>>      ARMCPU cpu;
>> -    MemoryRegion iomem;
>>      MemoryRegion sram;
>>      AspeedVICState vic;
>>      AspeedTimerCtrlState timerctrl;
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index a786750e14..3d21128d47 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -15,6 +15,7 @@
>>  #include "qemu-common.h"
>>  #include "cpu.h"
>>  #include "exec/address-spaces.h"
>> +#include "hw/misc/unimp.h"
>>  #include "hw/arm/aspeed_soc.h"
>>  #include "hw/char/serial.h"
>>  #include "qemu/log.h"
>> @@ -99,31 +100,6 @@ static const AspeedSoCInfo aspeed_socs[] = {
>>      },
>>  };
>>  
>> -/*
>> - * IO handlers: simply catch any reads/writes to IO addresses that aren't
>> - * handled by a device mapping.
>> - */
>> -
>> -static uint64_t aspeed_soc_io_read(void *p, hwaddr offset, unsigned size)
>> -{
>> -    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
>> -                  __func__, offset, size);
>> -    return 0;
>> -}
>> -
>> -static void aspeed_soc_io_write(void *opaque, hwaddr offset, uint64_t value,
>> -                unsigned size)
>> -{
>> -    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
>> -                  __func__, offset, value, size);
>> -}
>> -
>> -static const MemoryRegionOps aspeed_soc_io_ops = {
>> -    .read = aspeed_soc_io_read,
>> -    .write = aspeed_soc_io_write,
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> -};
>> -
>>  static void aspeed_soc_init(Object *obj)
>>  {
>>      AspeedSoCState *s = ASPEED_SOC(obj);
>> @@ -199,10 +175,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>      Error *err = NULL, *local_err = NULL;
>>  
>>      /* IO space */
>> -    memory_region_init_io(&s->iomem, NULL, &aspeed_soc_io_ops, NULL,
>> -            "aspeed_soc.io", ASPEED_SOC_IOMEM_SIZE);
>> -    memory_region_add_subregion_overlap(get_system_memory(),
>> -                                        ASPEED_SOC_IOMEM_BASE, &s->iomem, -1);
>> +    create_unimplemented_device("aspeed_soc.io",
>> +                                ASPEED_SOC_IOMEM_BASE, ASPEED_SOC_IOMEM_SIZE);
>>  
>>      /* CPU */
>>      object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
>>
> 
>