Message ID | 20211203073058.10980-1-jgross@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tools/libs/light: set video_mem for PVH guests | expand |
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,
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
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.
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 =
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(+)