Re: [Qemu-devel] [PATCH v4 04/20] hw/acpi/aml-build: Add aml_memory32_fixed() term

[Alex Bennée]

Shannon Zhao <address@hidden> writes:

> From: Shannon Zhao <address@hidden>
>
> Add aml_memory32_fixed() for describing device mmio region in resource 
> template.
> These can be used to generating DSDT table for ACPI on ARM.
>
> Signed-off-by: Shannon Zhao <address@hidden>
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
>  hw/acpi/aml-build.c         | 22 ++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  1 +
>  2 files changed, 23 insertions(+)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8d01959..fefe7c7 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -505,6 +505,28 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, 
> Aml *arg3, Aml *arg4)
>      return var;
>  }
>  
> +/*
> + * ACPI 1.0: 6.4.3.4 Memory32Fixed (Memory Resource Descriptor Macro)
> + */
> +Aml *aml_memory32_fixed(uint64_t addr, uint64_t size, uint8_t rw_flag)
> +{
> +    Aml *var = aml_alloc();

This is more aimed at the ACPI maintainers but I wonder if there should
be an aml_alloc_sized that pre-allocates the GArray? Otherwise we spend
a lot of time realloc'ing while building these entries up. Or even a
varidac build_append_bytes?

> +    build_append_byte(var->buf, 0x86); /* Memory32Fixed Resource Descriptor 
> */
> +    build_append_byte(var->buf, 9); /* Length, bits[7:0] value = 9 */
> +    build_append_byte(var->buf, 0); /* Length, bits[15:8] value = 0 */
> +    build_append_byte(var->buf, rw_flag); /* Write status, 1 rw 0 ro */
> +    build_append_byte(var->buf, addr & 0xff); /* Range base address 
> bits[7:0] */
> +    build_append_byte(var->buf, (addr >> 8) & 0xff); /* Range base address 
> bits[15:8] */
> +    build_append_byte(var->buf, (addr >> 16) & 0xff); /* Range base address 
> bits[23:16] */
> +    build_append_byte(var->buf, (addr >> 24) & 0xff); /* Range base
> address bits[31:24] */

I'm should point out we have handy utility functions for bit fiddling:

    build_append_byte(var->buf, extract64(addr, 8, 8)); /* Range base address 
bits[15:8] */

> +
> +    build_append_byte(var->buf, size & 0xff); /* Range length bits[7:0] */
> +    build_append_byte(var->buf, (size >> 8) & 0xff); /* Range length 
> bits[15:8] */
> +    build_append_byte(var->buf, (size >> 16) & 0xff); /* Range length 
> bits[23:16] */
> +    build_append_byte(var->buf, (size >> 24) & 0xff); /* Range length
> bits[31:24] */

Hmm we seem to have two 64 bit inputs which we only use 32 bits worth
of. Maybe the prototype should be fixed to avoid accidents of accidentally
passing in 64 bit values.


> +    return var;
> +}
> +
>  /* ACPI 1.0b: 6.4.2.5 I/O Port Descriptor */
>  Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t max_base,
>              uint8_t aln, uint8_t len)
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1705001..baa0652 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -162,6 +162,7 @@ Aml *aml_call1(const char *method, Aml *arg1);
>  Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
>  Aml *aml_call3(const char *method, Aml *arg1, Aml *arg2, Aml *arg3);
>  Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml 
> *arg4);
> +Aml *aml_memory32_fixed(uint64_t addr, uint64_t size, uint8_t rw_flag);
>  Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t max_base,
>              uint8_t aln, uint8_t len);
>  Aml *aml_operation_region(const char *name, AmlRegionSpace rs,

-- 
Alex Bennée