Re: [PATCH 3/3] tools/libxl: Only allocate 64 bytes for RSDP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2021 18:34, Kevin Stefanov wrote:
> RSDP's size is 64 bytes and later in the function, its buffer is
> hardcoded to be 64 bytes long. Don't bother to allocate a whole page.
> 
> Signed-off-by: Kevin Stefanov <kevin.stefanov@xxxxxxxxxx>

Purely technically and within the constraints of the present code:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
However, ...

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -183,7 +183,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>          goto out;
>      }
>  
> -    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> +    config.rsdp = (unsigned long)libxl__malloc(gc, 64);

... this is the 4th literal 64 in the function (including one in a
comment), all of which are meant to represent the same abstract thing.
And none of which look to be really correct:
sizeof(struct acpi_20_rsdp) == 36 according to my counting. While I
don't mind using 64 (for the time being), I think this should then be
via a #define, which would be accompanied by a respective comment. Or
else via sizeof(struct acpi_20_rsdp).

But of course hard-coding the size isn't really forward compatible
anyway. It should rather be libacpi to specify what size the blob is.
And then it might be safer to stick to allocating a full page here, as
the actual size won't be known up front. Or the allocated size would
need to become an input to acpi_build_tables(), so that it wouldn't
risk overrunning the space.

Jan