Re: [PATCH 1/1] acpi: cannot have RSDT above 4 GiB
Hi Heinrich,
On Sun, 12 Nov 2023 at 13:16, Heinrich Schuchardt
wrote:
>
>
>
> Simon Glass schrieb am So., 12. Nov. 2023, 21:03:
>>
>> Hi Heinrich,
>>
>> On Sat, 11 Nov 2023 at 07:28, Heinrich Schuchardt
>> wrote:
>> >
>> > The field RsdtAddress has only 32 bit. The RSDT table cannot be located
>> > beyond 4 GiB.
>> >
>> > Signed-off-by: Heinrich Schuchardt
>> > ---
>> > lib/acpi/base.c | 26 +++---
>> > 1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> Reviewed-by: Simon Glass
>>
>> nits / question below
>>
>> >
>> > diff --git a/lib/acpi/base.c b/lib/acpi/base.c
>> > index 2057bd2bef..128a76ad39 100644
>> > --- a/lib/acpi/base.c
>> > +++ b/lib/acpi/base.c
>> > @@ -21,10 +21,17 @@ void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct
>> > acpi_rsdt *rsdt,
>> > memcpy(rsdp->signature, RSDP_SIG, 8);
>> > memcpy(rsdp->oem_id, OEM_ID, 6);
>> >
>> > - rsdp->length = sizeof(struct acpi_rsdp);
>> > - rsdp->rsdt_address = map_to_sysmem(rsdt);
>> > + if (rsdt)
>> > + rsdp->rsdt_address = map_to_sysmem(rsdt);
>> > + else
>> > + rsdp->rsdt_address = 0;
>>
>> There is a memset() at the top so this line (and the one below) should
>> not be needed.
>>
>> > +
>> > + if (xsdt)
>> > + rsdp->xsdt_address = map_to_sysmem(xsdt);
>> > + else
>> > + rsdp->xsdt_address = 0;
>> >
>> > - rsdp->xsdt_address = map_to_sysmem(xsdt);
>> > + rsdp->length = sizeof(struct acpi_rsdp);
>> > rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
>> >
>> > /* Calculate checksums */
>> > @@ -68,11 +75,15 @@ static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
>> > static int acpi_write_base(struct acpi_ctx *ctx,
>> >const struct acpi_writer *entry)
>> > {
>> > - /* We need at least an RSDP and an RSDT Table */
>> > + /* We need at least an RSDP and an XSDT Table */
>> > ctx->rsdp = ctx->current;
>> > acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
>> > - ctx->rsdt = ctx->current;
>> > - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
>> > + if (map_to_sysmem(ctx->current) < UINT_MAX - 0x1) {
>>
>> Won't that do something different on 64-bit machines? It seems that
>> you want to check against SZ_4G ?
>
>
> For gcc int is 32bit on 64bit systems, so is UINT_MAX. Using SZ_4G could make
> this code clearer.
Oh I didn't know that...I just assumed that uint would be 64-bit on a
64-machine machine, since it is the natural reg size! Yes, that is
very very confusing. I vaguely recall some compiler setting for that
and that Linux did it a particular way.
>
> Best regards
>
> Heinrich
>
>
>>
>> > + ctx->rsdt = ctx->current;
>> > + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
>> > + } else {
>> > + ctx->rsdt = 0;
>> > + }
>> > ctx->xsdt = ctx->current;
>> > acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
>> >
>> > @@ -80,7 +91,8 @@ static int acpi_write_base(struct acpi_ctx *ctx,
>> > memset(ctx->base, '\0', ctx->current - ctx->base);
>> >
>> > acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
>> > - acpi_write_rsdt(ctx->rsdt);
>> > + if (ctx->rsdt)
>> > + acpi_write_rsdt(ctx->rsdt);
>> > acpi_write_xsdt(ctx->xsdt);
>> >
>> > return 0;
>> > --
>> > 2.40.1
>> >
>>
>> Regards,
>> Simon
Re: [PATCH 1/1] acpi: cannot have RSDT above 4 GiB
Simon Glass schrieb am So., 12. Nov. 2023, 21:03:
> Hi Heinrich,
>
> On Sat, 11 Nov 2023 at 07:28, Heinrich Schuchardt
> wrote:
> >
> > The field RsdtAddress has only 32 bit. The RSDT table cannot be located
> > beyond 4 GiB.
> >
> > Signed-off-by: Heinrich Schuchardt
> > ---
> > lib/acpi/base.c | 26 +++---
> > 1 file changed, 19 insertions(+), 7 deletions(-)
>
> Reviewed-by: Simon Glass
>
> nits / question below
>
> >
> > diff --git a/lib/acpi/base.c b/lib/acpi/base.c
> > index 2057bd2bef..128a76ad39 100644
> > --- a/lib/acpi/base.c
> > +++ b/lib/acpi/base.c
> > @@ -21,10 +21,17 @@ void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct
> acpi_rsdt *rsdt,
> > memcpy(rsdp->signature, RSDP_SIG, 8);
> > memcpy(rsdp->oem_id, OEM_ID, 6);
> >
> > - rsdp->length = sizeof(struct acpi_rsdp);
> > - rsdp->rsdt_address = map_to_sysmem(rsdt);
> > + if (rsdt)
> > + rsdp->rsdt_address = map_to_sysmem(rsdt);
> > + else
> > + rsdp->rsdt_address = 0;
>
> There is a memset() at the top so this line (and the one below) should
> not be needed.
>
> > +
> > + if (xsdt)
> > + rsdp->xsdt_address = map_to_sysmem(xsdt);
> > + else
> > + rsdp->xsdt_address = 0;
> >
> > - rsdp->xsdt_address = map_to_sysmem(xsdt);
> > + rsdp->length = sizeof(struct acpi_rsdp);
> > rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
> >
> > /* Calculate checksums */
> > @@ -68,11 +75,15 @@ static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
> > static int acpi_write_base(struct acpi_ctx *ctx,
> >const struct acpi_writer *entry)
> > {
> > - /* We need at least an RSDP and an RSDT Table */
> > + /* We need at least an RSDP and an XSDT Table */
> > ctx->rsdp = ctx->current;
> > acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
> > - ctx->rsdt = ctx->current;
> > - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> > + if (map_to_sysmem(ctx->current) < UINT_MAX - 0x1) {
>
> Won't that do something different on 64-bit machines? It seems that
> you want to check against SZ_4G ?
>
For gcc int is 32bit on 64bit systems, so is UINT_MAX. Using SZ_4G could
make this code clearer.
Best regards
Heinrich
> > + ctx->rsdt = ctx->current;
> > + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> > + } else {
> > + ctx->rsdt = 0;
> > + }
> > ctx->xsdt = ctx->current;
> > acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
> >
> > @@ -80,7 +91,8 @@ static int acpi_write_base(struct acpi_ctx *ctx,
> > memset(ctx->base, '\0', ctx->current - ctx->base);
> >
> > acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > - acpi_write_rsdt(ctx->rsdt);
> > + if (ctx->rsdt)
> > + acpi_write_rsdt(ctx->rsdt);
> > acpi_write_xsdt(ctx->xsdt);
> >
> > return 0;
> > --
> > 2.40.1
> >
>
> Regards,
> Simon
>
Re: [PATCH 1/1] acpi: cannot have RSDT above 4 GiB
Hi Heinrich,
On Sat, 11 Nov 2023 at 07:28, Heinrich Schuchardt
wrote:
>
> The field RsdtAddress has only 32 bit. The RSDT table cannot be located
> beyond 4 GiB.
>
> Signed-off-by: Heinrich Schuchardt
> ---
> lib/acpi/base.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass
nits / question below
>
> diff --git a/lib/acpi/base.c b/lib/acpi/base.c
> index 2057bd2bef..128a76ad39 100644
> --- a/lib/acpi/base.c
> +++ b/lib/acpi/base.c
> @@ -21,10 +21,17 @@ void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct
> acpi_rsdt *rsdt,
> memcpy(rsdp->signature, RSDP_SIG, 8);
> memcpy(rsdp->oem_id, OEM_ID, 6);
>
> - rsdp->length = sizeof(struct acpi_rsdp);
> - rsdp->rsdt_address = map_to_sysmem(rsdt);
> + if (rsdt)
> + rsdp->rsdt_address = map_to_sysmem(rsdt);
> + else
> + rsdp->rsdt_address = 0;
There is a memset() at the top so this line (and the one below) should
not be needed.
> +
> + if (xsdt)
> + rsdp->xsdt_address = map_to_sysmem(xsdt);
> + else
> + rsdp->xsdt_address = 0;
>
> - rsdp->xsdt_address = map_to_sysmem(xsdt);
> + rsdp->length = sizeof(struct acpi_rsdp);
> rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
>
> /* Calculate checksums */
> @@ -68,11 +75,15 @@ static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
> static int acpi_write_base(struct acpi_ctx *ctx,
>const struct acpi_writer *entry)
> {
> - /* We need at least an RSDP and an RSDT Table */
> + /* We need at least an RSDP and an XSDT Table */
> ctx->rsdp = ctx->current;
> acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
> - ctx->rsdt = ctx->current;
> - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> + if (map_to_sysmem(ctx->current) < UINT_MAX - 0x1) {
Won't that do something different on 64-bit machines? It seems that
you want to check against SZ_4G ?
> + ctx->rsdt = ctx->current;
> + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> + } else {
> + ctx->rsdt = 0;
> + }
> ctx->xsdt = ctx->current;
> acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
>
> @@ -80,7 +91,8 @@ static int acpi_write_base(struct acpi_ctx *ctx,
> memset(ctx->base, '\0', ctx->current - ctx->base);
>
> acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> - acpi_write_rsdt(ctx->rsdt);
> + if (ctx->rsdt)
> + acpi_write_rsdt(ctx->rsdt);
> acpi_write_xsdt(ctx->xsdt);
>
> return 0;
> --
> 2.40.1
>
Regards,
Simon
