diff mbox series

[v2,10/11] hw/avr: Prepare for TARGET_PAGE_SIZE > 256

Message ID 20250325224403.4011975-11-richard.henderson@linaro.org (mailing list archive)
State New
Headers show
Series target/avr: Increase page size | expand

Commit Message

Richard Henderson March 25, 2025, 10:44 p.m. UTC 
If i/o does not cover the entire first page, allocate a portion
of ram as an i/o device, so that the entire first page is i/o.

While memory_region_init_ram_device_ptr is happy to allocate
the RAMBlock, it does not register the ram for migration.
Do this by hand.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/avr/atmega.h |  1 +
 hw/avr/atmega.c | 39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 33 insertions(+), 7 deletions(-)

Comments

Pierrick Bouvier March 26, 2025, 3:18 p.m. UTC | #1 
On 3/25/25 15:44, Richard Henderson wrote:
> If i/o does not cover the entire first page, allocate a portion
> of ram as an i/o device, so that the entire first page is i/o.
> 
> While memory_region_init_ram_device_ptr is happy to allocate
> the RAMBlock, it does not register the ram for migration.
> Do this by hand.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/avr/atmega.h |  1 +
>   hw/avr/atmega.c | 39 ++++++++++++++++++++++++++++++++-------
>   2 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> index a99ee15c7e..9ac4678231 100644
> --- a/hw/avr/atmega.h
> +++ b/hw/avr/atmega.h
> @@ -41,6 +41,7 @@ struct AtmegaMcuState {
>       MemoryRegion flash;
>       MemoryRegion eeprom;
>       MemoryRegion sram;
> +    MemoryRegion sram_io;
>       DeviceState *io;
>       AVRMaskState pwr[POWER_MAX];
>       AVRUsartState usart[USART_MAX];
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index f6844bf118..11fab184de 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -19,6 +19,7 @@
>   #include "hw/sysbus.h"
>   #include "qom/object.h"
>   #include "hw/misc/unimp.h"
> +#include "migration/vmstate.h"
>   #include "atmega.h"
>   
>   enum AtmegaPeripheral {
> @@ -224,8 +225,6 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>       char *devname;
>       size_t i;
>   
> -    assert(mc->io_size <= 0x200);
> -
>       if (!s->xtal_freq_hz) {
>           error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
>           return;
> @@ -240,11 +239,37 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>       qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>       cpudev = DEVICE(&s->cpu);
>   
> -    /* SRAM */
> -    memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
> -                           &error_abort);
> -    memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA + mc->io_size, &s->sram);
> +    /*
> +     * SRAM
> +     *
> +     * Softmmu is not able mix i/o and ram on the same page.
> +     * Therefore in all cases, the first page exclusively contains i/o.
> +     *
> +     * If the MCU's i/o region matches the page size, then we can simply
> +     * allocate all ram starting at the second page.  Otherwise, we must
> +     * allocate some ram as i/o to complete the first page.
> +     */
> +    assert(mc->io_size == 0x100 || mc->io_size == 0x200);
> +    if (mc->io_size >= TARGET_PAGE_SIZE) {
> +        memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
> +                               &error_abort);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + mc->io_size, &s->sram);
> +    } else {
> +        int sram_io_size = TARGET_PAGE_SIZE - mc->io_size;
> +        void *sram_io_mem = g_malloc0(sram_io_size);
> +
> +        memory_region_init_ram_device_ptr(&s->sram_io, OBJECT(dev), "sram-as-io",
> +                                          sram_io_size, sram_io_mem);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + mc->io_size, &s->sram_io);
> +        vmstate_register_ram(&s->sram_io, dev);
> +
> +        memory_region_init_ram(&s->sram, OBJECT(dev), "sram",
> +                               mc->sram_size - sram_io_size, &error_abort);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + TARGET_PAGE_SIZE, &s->sram);
> +    }
>   
>       /* Flash */
>       memory_region_init_rom(&s->flash, OBJECT(dev),

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Philippe Mathieu-Daudé March 27, 2025, 6:24 p.m. UTC | #2 
On 25/3/25 23:44, Richard Henderson wrote:
> If i/o does not cover the entire first page, allocate a portion
> of ram as an i/o device, so that the entire first page is i/o.
> 
> While memory_region_init_ram_device_ptr is happy to allocate
> the RAMBlock, it does not register the ram for migration.
> Do this by hand.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/avr/atmega.h |  1 +
>   hw/avr/atmega.c | 39 ++++++++++++++++++++++++++++++++-------
>   2 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> index a99ee15c7e..9ac4678231 100644
> --- a/hw/avr/atmega.h
> +++ b/hw/avr/atmega.h
> @@ -41,6 +41,7 @@ struct AtmegaMcuState {
>       MemoryRegion flash;
>       MemoryRegion eeprom;
>       MemoryRegion sram;
> +    MemoryRegion sram_io;
>       DeviceState *io;
>       AVRMaskState pwr[POWER_MAX];
>       AVRUsartState usart[USART_MAX];

> @@ -240,11 +239,37 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>       qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>       cpudev = DEVICE(&s->cpu);
>   
> -    /* SRAM */
> -    memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
> -                           &error_abort);
> -    memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA + mc->io_size, &s->sram);
> +    /*
> +     * SRAM
> +     *
> +     * Softmmu is not able mix i/o and ram on the same page.
> +     * Therefore in all cases, the first page exclusively contains i/o.
> +     *
> +     * If the MCU's i/o region matches the page size, then we can simply
> +     * allocate all ram starting at the second page.  Otherwise, we must
> +     * allocate some ram as i/o to complete the first page.
> +     */
> +    assert(mc->io_size == 0x100 || mc->io_size == 0x200);
> +    if (mc->io_size >= TARGET_PAGE_SIZE) {
> +        memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
> +                               &error_abort);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + mc->io_size, &s->sram);
> +    } else {
> +        int sram_io_size = TARGET_PAGE_SIZE - mc->io_size;
> +        void *sram_io_mem = g_malloc0(sram_io_size);

Please declare sram_io_mem in AtmegaMcuState, after sram_io.

> +
> +        memory_region_init_ram_device_ptr(&s->sram_io, OBJECT(dev), "sram-as-io",
> +                                          sram_io_size, sram_io_mem);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + mc->io_size, &s->sram_io);
> +        vmstate_register_ram(&s->sram_io, dev);
> +
> +        memory_region_init_ram(&s->sram, OBJECT(dev), "sram",
> +                               mc->sram_size - sram_io_size, &error_abort);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    OFFSET_DATA + TARGET_PAGE_SIZE, &s->sram);
> +    }
Richard Henderson March 27, 2025, 6:46 p.m. UTC | #3 
On 3/27/25 13:24, Philippe Mathieu-Daudé wrote:
> On 25/3/25 23:44, Richard Henderson wrote:
>> If i/o does not cover the entire first page, allocate a portion
>> of ram as an i/o device, so that the entire first page is i/o.
>>
>> While memory_region_init_ram_device_ptr is happy to allocate
>> the RAMBlock, it does not register the ram for migration.
>> Do this by hand.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   hw/avr/atmega.h |  1 +
>>   hw/avr/atmega.c | 39 ++++++++++++++++++++++++++++++++-------
>>   2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
>> index a99ee15c7e..9ac4678231 100644
>> --- a/hw/avr/atmega.h
>> +++ b/hw/avr/atmega.h
>> @@ -41,6 +41,7 @@ struct AtmegaMcuState {
>>       MemoryRegion flash;
>>       MemoryRegion eeprom;
>>       MemoryRegion sram;
>> +    MemoryRegion sram_io;
>>       DeviceState *io;
>>       AVRMaskState pwr[POWER_MAX];
>>       AVRUsartState usart[USART_MAX];
> 
>> @@ -240,11 +239,37 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>>       qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>>       cpudev = DEVICE(&s->cpu);
>> -    /* SRAM */
>> -    memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
>> -                           &error_abort);
>> -    memory_region_add_subregion(get_system_memory(),
>> -                                OFFSET_DATA + mc->io_size, &s->sram);
>> +    /*
>> +     * SRAM
>> +     *
>> +     * Softmmu is not able mix i/o and ram on the same page.
>> +     * Therefore in all cases, the first page exclusively contains i/o.
>> +     *
>> +     * If the MCU's i/o region matches the page size, then we can simply
>> +     * allocate all ram starting at the second page.  Otherwise, we must
>> +     * allocate some ram as i/o to complete the first page.
>> +     */
>> +    assert(mc->io_size == 0x100 || mc->io_size == 0x200);
>> +    if (mc->io_size >= TARGET_PAGE_SIZE) {
>> +        memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
>> +                               &error_abort);
>> +        memory_region_add_subregion(get_system_memory(),
>> +                                    OFFSET_DATA + mc->io_size, &s->sram);
>> +    } else {
>> +        int sram_io_size = TARGET_PAGE_SIZE - mc->io_size;
>> +        void *sram_io_mem = g_malloc0(sram_io_size);
> 
> Please declare sram_io_mem in AtmegaMcuState, after sram_io.

Why?

>> +
>> +        memory_region_init_ram_device_ptr(&s->sram_io, OBJECT(dev), "sram-as-io",
>> +                                          sram_io_size, sram_io_mem);

After this, it's accessible as

     s->sram_io->ram_block->host

It wouldn't be accessed in any other way, surely?


r~
diff mbox series

Patch

diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
index a99ee15c7e..9ac4678231 100644
--- a/hw/avr/atmega.h
+++ b/hw/avr/atmega.h
@@ -41,6 +41,7 @@  struct AtmegaMcuState {
     MemoryRegion flash;
     MemoryRegion eeprom;
     MemoryRegion sram;
+    MemoryRegion sram_io;
     DeviceState *io;
     AVRMaskState pwr[POWER_MAX];
     AVRUsartState usart[USART_MAX];
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index f6844bf118..11fab184de 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -19,6 +19,7 @@ 
 #include "hw/sysbus.h"
 #include "qom/object.h"
 #include "hw/misc/unimp.h"
+#include "migration/vmstate.h"
 #include "atmega.h"
 
 enum AtmegaPeripheral {
@@ -224,8 +225,6 @@  static void atmega_realize(DeviceState *dev, Error **errp)
     char *devname;
     size_t i;
 
-    assert(mc->io_size <= 0x200);
-
     if (!s->xtal_freq_hz) {
         error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
         return;
@@ -240,11 +239,37 @@  static void atmega_realize(DeviceState *dev, Error **errp)
     qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
     cpudev = DEVICE(&s->cpu);
 
-    /* SRAM */
-    memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
-                           &error_abort);
-    memory_region_add_subregion(get_system_memory(),
-                                OFFSET_DATA + mc->io_size, &s->sram);
+    /*
+     * SRAM
+     *
+     * Softmmu is not able mix i/o and ram on the same page.
+     * Therefore in all cases, the first page exclusively contains i/o.
+     *
+     * If the MCU's i/o region matches the page size, then we can simply
+     * allocate all ram starting at the second page.  Otherwise, we must
+     * allocate some ram as i/o to complete the first page.
+     */
+    assert(mc->io_size == 0x100 || mc->io_size == 0x200);
+    if (mc->io_size >= TARGET_PAGE_SIZE) {
+        memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
+                               &error_abort);
+        memory_region_add_subregion(get_system_memory(),
+                                    OFFSET_DATA + mc->io_size, &s->sram);
+    } else {
+        int sram_io_size = TARGET_PAGE_SIZE - mc->io_size;
+        void *sram_io_mem = g_malloc0(sram_io_size);
+
+        memory_region_init_ram_device_ptr(&s->sram_io, OBJECT(dev), "sram-as-io",
+                                          sram_io_size, sram_io_mem);
+        memory_region_add_subregion(get_system_memory(),
+                                    OFFSET_DATA + mc->io_size, &s->sram_io);
+        vmstate_register_ram(&s->sram_io, dev);
+
+        memory_region_init_ram(&s->sram, OBJECT(dev), "sram",
+                               mc->sram_size - sram_io_size, &error_abort);
+        memory_region_add_subregion(get_system_memory(),
+                                    OFFSET_DATA + TARGET_PAGE_SIZE, &s->sram);
+    }
 
     /* Flash */
     memory_region_init_rom(&s->flash, OBJECT(dev),