Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region
@ 2020-10-16  0:39 Igor Druzhinin
  2020-10-16  6:34 ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Igor Druzhinin @ 2020-10-16  0:39 UTC (permalink / raw)
  To: xen-devel; +Cc: jbeulich, andrew.cooper3, roger.pau, wl, iwj, Igor Druzhinin

ACPI specification contains statements describing memory marked with regular
"ACPI data" type as reclaimable by the guest. Although the guest shouldn't
really do it if it wants kexec or similar functionality to work, there
could still be ambiguities in treating these regions as potentially regular
RAM.

One such example is SeaBIOS which currently reports "ACPI data" regions as
RAM to the guest in its e801 call. Which it might have the right to do as any
user of this is expected to be ACPI unaware. But a QEMU bootloader later seems
to ignore that fact and is instead using e801 to find a place for initrd which
causes the tables to be erased. While arguably QEMU bootloader or SeaBIOS need
to be fixed / improved here, that is just one example of the potential problems
from using a reclaimable memory type.

Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
described by the spec as non-reclaimable (so cannot ever be treated like RAM).

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v2:
- Put the exact reasoning into a comment
- Improved commit message
---
 tools/firmware/hvmloader/e820.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 38bcf18..c490a0b 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -202,16 +202,21 @@ int build_e820_table(struct e820entry *e820,
     nr++;
 
     /*
-     * Mark populated reserved memory that contains ACPI tables as ACPI data.
+     * Mark populated reserved memory that contains ACPI tables as ACPI NVS.
      * That should help the guest to treat it correctly later: e.g. pass to
-     * the next kernel on kexec or reclaim if necessary.
+     * the next kernel on kexec.
+     *
+     * Using NVS type instead of a regular one helps to prevent potential
+     * space reuse by an ACPI unaware / buggy bootloader, option ROM, etc.
+     * before an ACPI OS takes control. This is possible due to the fact that
+     * ACPI NVS memory is explicitly described as non-reclaimable in ACPI spec.
      */
 
     if ( acpi_enabled )
     {
         e820[nr].addr = RESERVED_MEMBASE;
         e820[nr].size = acpi_mem_end - RESERVED_MEMBASE;
-        e820[nr].type = E820_ACPI;
+        e820[nr].type = E820_NVS;
         nr++;
     }
 
-- 
2.7.4



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region
  2020-10-16  0:39 [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region Igor Druzhinin
@ 2020-10-16  6:34 ` Jan Beulich
  2020-10-16 13:34   ` Sander Eikelenboom
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2020-10-16  6:34 UTC (permalink / raw)
  To: Igor Druzhinin; +Cc: xen-devel, andrew.cooper3, roger.pau, wl, iwj

On 16.10.2020 02:39, Igor Druzhinin wrote:
> ACPI specification contains statements describing memory marked with regular
> "ACPI data" type as reclaimable by the guest. Although the guest shouldn't
> really do it if it wants kexec or similar functionality to work, there
> could still be ambiguities in treating these regions as potentially regular
> RAM.
> 
> One such example is SeaBIOS which currently reports "ACPI data" regions as
> RAM to the guest in its e801 call. Which it might have the right to do as any
> user of this is expected to be ACPI unaware. But a QEMU bootloader later seems
> to ignore that fact and is instead using e801 to find a place for initrd which
> causes the tables to be erased. While arguably QEMU bootloader or SeaBIOS need
> to be fixed / improved here, that is just one example of the potential problems
> from using a reclaimable memory type.
> 
> Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
> described by the spec as non-reclaimable (so cannot ever be treated like RAM).
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region
  2020-10-16  6:34 ` Jan Beulich
@ 2020-10-16 13:34   ` Sander Eikelenboom
  2020-10-16 13:48     ` Igor Druzhinin
  0 siblings, 1 reply; 5+ messages in thread
From: Sander Eikelenboom @ 2020-10-16 13:34 UTC (permalink / raw)
  To: Jan Beulich, Igor Druzhinin; +Cc: xen-devel, andrew.cooper3, roger.pau, wl, iwj

On 16/10/2020 08:34, Jan Beulich wrote:
> On 16.10.2020 02:39, Igor Druzhinin wrote:
>> ACPI specification contains statements describing memory marked with regular
>> "ACPI data" type as reclaimable by the guest. Although the guest shouldn't
>> really do it if it wants kexec or similar functionality to work, there
>> could still be ambiguities in treating these regions as potentially regular
>> RAM.
>>
>> One such example is SeaBIOS which currently reports "ACPI data" regions as
>> RAM to the guest in its e801 call. Which it might have the right to do as any
>> user of this is expected to be ACPI unaware. But a QEMU bootloader later seems
>> to ignore that fact and is instead using e801 to find a place for initrd which
>> causes the tables to be erased. While arguably QEMU bootloader or SeaBIOS need
>> to be fixed / improved here, that is just one example of the potential problems
>> from using a reclaimable memory type.
>>
>> Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
>> described by the spec as non-reclaimable (so cannot ever be treated like RAM).
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> 

I don't see any stable and or fixes tags, but I assume this will go to
the stable trees (which have (a backport of)
8efa46516c5f4cf185c8df179812c185d3c27eb6 in their staging branches) ?

(and as reporter it would have been nice to have been CC'ed on the patch)

--
Sander


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region
  2020-10-16 13:34   ` Sander Eikelenboom
@ 2020-10-16 13:48     ` Igor Druzhinin
  2020-10-16 15:13       ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Igor Druzhinin @ 2020-10-16 13:48 UTC (permalink / raw)
  To: Sander Eikelenboom, Jan Beulich
  Cc: xen-devel, andrew.cooper3, roger.pau, wl, iwj

On 16/10/2020 14:34, Sander Eikelenboom wrote:
> On 16/10/2020 08:34, Jan Beulich wrote:
>> On 16.10.2020 02:39, Igor Druzhinin wrote:
>>> ACPI specification contains statements describing memory marked with regular
>>> "ACPI data" type as reclaimable by the guest. Although the guest shouldn't
>>> really do it if it wants kexec or similar functionality to work, there
>>> could still be ambiguities in treating these regions as potentially regular
>>> RAM.
>>>
>>> One such example is SeaBIOS which currently reports "ACPI data" regions as
>>> RAM to the guest in its e801 call. Which it might have the right to do as any
>>> user of this is expected to be ACPI unaware. But a QEMU bootloader later seems
>>> to ignore that fact and is instead using e801 to find a place for initrd which
>>> causes the tables to be erased. While arguably QEMU bootloader or SeaBIOS need
>>> to be fixed / improved here, that is just one example of the potential problems
>>> from using a reclaimable memory type.
>>>
>>> Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
>>> described by the spec as non-reclaimable (so cannot ever be treated like RAM).
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>
> 
> I don't see any stable and or fixes tags, but I assume this will go to
> the stable trees (which have (a backport of)
> 8efa46516c5f4cf185c8df179812c185d3c27eb6 in their staging branches) ?

Yes, this should go to the stable branches as well. I don't usually see Fixes:
tag being used on xen-devel (not sure if it's intentional or simply not
customary). It's also questionable whether it's a fix or a workaround for an
issue with compatibility.

Igor


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region
  2020-10-16 13:48     ` Igor Druzhinin
@ 2020-10-16 15:13       ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2020-10-16 15:13 UTC (permalink / raw)
  To: Igor Druzhinin, Sander Eikelenboom
  Cc: xen-devel, andrew.cooper3, roger.pau, wl, iwj

On 16.10.2020 15:48, Igor Druzhinin wrote:
> On 16/10/2020 14:34, Sander Eikelenboom wrote:
>> On 16/10/2020 08:34, Jan Beulich wrote:
>>> On 16.10.2020 02:39, Igor Druzhinin wrote:
>>>> ACPI specification contains statements describing memory marked with regular
>>>> "ACPI data" type as reclaimable by the guest. Although the guest shouldn't
>>>> really do it if it wants kexec or similar functionality to work, there
>>>> could still be ambiguities in treating these regions as potentially regular
>>>> RAM.
>>>>
>>>> One such example is SeaBIOS which currently reports "ACPI data" regions as
>>>> RAM to the guest in its e801 call. Which it might have the right to do as any
>>>> user of this is expected to be ACPI unaware. But a QEMU bootloader later seems
>>>> to ignore that fact and is instead using e801 to find a place for initrd which
>>>> causes the tables to be erased. While arguably QEMU bootloader or SeaBIOS need
>>>> to be fixed / improved here, that is just one example of the potential problems
>>>> from using a reclaimable memory type.
>>>>
>>>> Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
>>>> described by the spec as non-reclaimable (so cannot ever be treated like RAM).
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>
>>>
>>
>> I don't see any stable and or fixes tags, but I assume this will go to
>> the stable trees (which have (a backport of)
>> 8efa46516c5f4cf185c8df179812c185d3c27eb6 in their staging branches) ?

Yes, I intend to queue this up, as I did the backport of the
earlier one.

> Yes, this should go to the stable branches as well. I don't usually see Fixes:
> tag being used on xen-devel (not sure if it's intentional or simply not
> customary).

Go look again - there's an increasing amount of use of it.

> It's also questionable whether it's a fix or a workaround for an
> issue with compatibility.

Indeed - it is for this reason that I didn't ask for such a
tag to be added here.

Jan


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16  0:39 [PATCH v2] hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region Igor Druzhinin
2020-10-16  6:34 ` Jan Beulich
2020-10-16 13:34   ` Sander Eikelenboom
2020-10-16 13:48     ` Igor Druzhinin
2020-10-16 15:13       ` Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git