The size of the video memory of PVH guests should be set to 0 in case
no value has been specified.

Doing not so will leave it to be -1, resulting in an additional 1 kB
of RAM being advertised in the memory map (here the output of a PVH
Mini-OS boot with 16 MB of RAM assigned):

Memory map:
000000000000-0000010003ff: RAM
0000feff8000-0000feffffff: Reserved
0000fc008000-0000fc00803f: ACPI
0000fc000000-0000fc000fff: ACPI
0000fc001000-0000fc007fff: ACPI

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_create.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index dcd09d32ba..d7a40d7550 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -427,6 +427,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         break;
     case LIBXL_DOMAIN_TYPE_PVH:
         libxl_defbool_setdefault(&b_info->u.pvh.pvshim, false);
+        if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
+            b_info->video_memkb = 0;
         if (libxl_defbool_val(b_info->u.pvh.pvshim)) {
             if (!b_info->u.pvh.pvshim_path)
                 b_info->u.pvh.pvshim_path =
-- 
2.26.2

On Fri, Dec 03, 2021 at 08:30:58AM +0100, Juergen Gross wrote:
> The size of the video memory of PVH guests should be set to 0 in case
> no value has been specified.
> 
> Doing not so will leave it to be -1, resulting in an additional 1 kB
> of RAM being advertised in the memory map (here the output of a PVH
> Mini-OS boot with 16 MB of RAM assigned):
> 
> Memory map:
> 000000000000-0000010003ff: RAM
> 0000feff8000-0000feffffff: Reserved
> 0000fc008000-0000fc00803f: ACPI
> 0000fc000000-0000fc000fff: ACPI
> 0000fc001000-0000fc007fff: ACPI
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

On Fri, Dec 03, 2021 at 08:30:58AM +0100, Juergen Gross wrote:
> The size of the video memory of PVH guests should be set to 0 in case
> no value has been specified.
> 
> Doing not so will leave it to be -1, resulting in an additional 1 kB
> of RAM being advertised in the memory map (here the output of a PVH
> Mini-OS boot with 16 MB of RAM assigned):
> 
> Memory map:
> 000000000000-0000010003ff: RAM
> 0000feff8000-0000feffffff: Reserved
> 0000fc008000-0000fc00803f: ACPI
> 0000fc000000-0000fc000fff: ACPI
> 0000fc001000-0000fc007fff: ACPI
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

On 03/12/2021 07:30, Juergen Gross wrote:
> The size of the video memory of PVH guests should be set to 0 in case
> no value has been specified.
>
> Doing not so will leave it to be -1, resulting in an additional 1 kB
> of RAM being advertised in the memory map (here the output of a PVH
> Mini-OS boot with 16 MB of RAM assigned):
>
> Memory map:
> 000000000000-0000010003ff: RAM
> 0000feff8000-0000feffffff: Reserved
> 0000fc008000-0000fc00803f: ACPI
> 0000fc000000-0000fc000fff: ACPI
> 0000fc001000-0000fc007fff: ACPI

The patch itself is fine, but some further observations based on the
memory map alone.

It is rude to provide an unsorted memory map.

The LAPIC range is required to be reserved by the ACPI spec, missing
here.  Conversely, it's unclear what the reserved region is trying to
describe.

Of the 3 ACPI ranges, one is RSDP (the first 64 bytes), one is the info
block (4k), and one is the ACPI tables themselves.

RSDP really ought to be merged into the same block as the rest of the
ACPI tables.

The info block must not be marked ACPI reclaimable RAM, because it is
referenced by AML inside the DSDT/etc.  This is a very serious issue if
the OS actually exercises its right to reclaim those regions and use
them as RAM.

~Andrew