[PATCH] tools/libs/light: set video_mem for PVH guests

[PATCH] tools/libs/light: set video_mem for PVH guests

[Juergen Gross]

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

Re: [PATCH] tools/libs/light: set video_mem for PVH guests

[Roger Pau Monné]

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.

Re: [PATCH] tools/libs/light: set video_mem for PVH guests

[Anthony PERARD]

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

Re: [PATCH] tools/libs/light: set video_mem for PVH guests

[Andrew Cooper]

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

Re: [PATCH] tools/libs/light: set video_mem for PVH guests

[Roger Pau Monné]

On Wed, Dec 08, 2021 at 02:00:48PM +0000, Andrew Cooper wrote:
> 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.

IIRC those are the special pages used by Xen console, store, identity
page tables, ioreq...

> 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.

There's no DSDT on PVH, but I'm unsure whether it's also marked as
reclaimable RAM on HVM which does have a DSDT. Last two blocks should
likely be ACPI NVS I assume.

Thanks, Roger.