* [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB
@ 2016-09-08 7:50 Ard Biesheuvel
2016-09-08 7:54 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2016-09-08 7:50 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
In general, on an ARM system, mapping normal memory as device memory may
have unintended side effects, given that unaligned accesses or loads and
stores with special semantics (e.g., load/store exclusive) may fault or
may not work as expected.
Under KVM, the situation is even worse, since the host may not expect the
guest to perform uncached accesses, and so writes to such an uncached
region may get lost completely.
Since the only safe mapping type under KVM is EFI_MEMORY_WB, remove all
other memory type attributes.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/HighMemDxe/HighMemDxe.c | 3 +--
ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 ---
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 7fd7e8e9a539..4d56e6236b54 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -78,8 +78,7 @@ InitializeHighMemDxe (
Status = gDS->AddMemorySpace (
EfiGcdMemoryTypeSystemMemory,
CurBase, CurSize,
- EFI_MEMORY_WB | EFI_MEMORY_WC |
- EFI_MEMORY_WT | EFI_MEMORY_UC);
+ EFI_MEMORY_WB);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_ERROR,
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 251e5314e61d..6f3e54b7afcb 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -68,9 +68,6 @@ MemoryPeim (
ResourceAttributes = (
EFI_RESOURCE_ATTRIBUTE_PRESENT |
EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
- EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
- EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
- EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
EFI_RESOURCE_ATTRIBUTE_TESTED
);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB
2016-09-08 7:50 [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB Ard Biesheuvel
@ 2016-09-08 7:54 ` Laszlo Ersek
2016-09-08 7:57 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2016-09-08 7:54 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
On 09/08/16 09:50, Ard Biesheuvel wrote:
> In general, on an ARM system, mapping normal memory as device memory may
> have unintended side effects, given that unaligned accesses or loads and
> stores with special semantics (e.g., load/store exclusive) may fault or
> may not work as expected.
>
> Under KVM, the situation is even worse, since the host may not expect the
> guest to perform uncached accesses, and so writes to such an uncached
> region may get lost completely.
>
> Since the only safe mapping type under KVM is EFI_MEMORY_WB, remove all
> other memory type attributes.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 3 +--
> ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 ---
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> index 7fd7e8e9a539..4d56e6236b54 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> @@ -78,8 +78,7 @@ InitializeHighMemDxe (
> Status = gDS->AddMemorySpace (
> EfiGcdMemoryTypeSystemMemory,
> CurBase, CurSize,
> - EFI_MEMORY_WB | EFI_MEMORY_WC |
> - EFI_MEMORY_WT | EFI_MEMORY_UC);
> + EFI_MEMORY_WB);
>
> if (EFI_ERROR (Status)) {
> DEBUG ((EFI_D_ERROR,
> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 251e5314e61d..6f3e54b7afcb 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -68,9 +68,6 @@ MemoryPeim (
> ResourceAttributes = (
> EFI_RESOURCE_ATTRIBUTE_PRESENT |
> EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> - EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> - EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> - EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> EFI_RESOURCE_ATTRIBUTE_TESTED
> );
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Did you encounter any specific problem with these?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB
2016-09-08 7:54 ` Laszlo Ersek
@ 2016-09-08 7:57 ` Ard Biesheuvel
2016-09-08 9:38 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2016-09-08 7:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On 8 September 2016 at 08:54, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/08/16 09:50, Ard Biesheuvel wrote:
>> In general, on an ARM system, mapping normal memory as device memory may
>> have unintended side effects, given that unaligned accesses or loads and
>> stores with special semantics (e.g., load/store exclusive) may fault or
>> may not work as expected.
>>
>> Under KVM, the situation is even worse, since the host may not expect the
>> guest to perform uncached accesses, and so writes to such an uncached
>> region may get lost completely.
>>
>> Since the only safe mapping type under KVM is EFI_MEMORY_WB, remove all
>> other memory type attributes.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 3 +--
>> ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 ---
>> 2 files changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> index 7fd7e8e9a539..4d56e6236b54 100644
>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> @@ -78,8 +78,7 @@ InitializeHighMemDxe (
>> Status = gDS->AddMemorySpace (
>> EfiGcdMemoryTypeSystemMemory,
>> CurBase, CurSize,
>> - EFI_MEMORY_WB | EFI_MEMORY_WC |
>> - EFI_MEMORY_WT | EFI_MEMORY_UC);
>> + EFI_MEMORY_WB);
>>
>> if (EFI_ERROR (Status)) {
>> DEBUG ((EFI_D_ERROR,
>> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>> index 251e5314e61d..6f3e54b7afcb 100644
>> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>> @@ -68,9 +68,6 @@ MemoryPeim (
>> ResourceAttributes = (
>> EFI_RESOURCE_ATTRIBUTE_PRESENT |
>> EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
>> - EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>> - EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>> - EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
>> EFI_RESOURCE_ATTRIBUTE_TESTED
>> );
>>
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Did you encounter any specific problem with these?
>
No, but I am looking into using the new optimized BaseMemoryLibOptDxe,
and the AARCH64 version uses DC ZVA instructions for ZeroMem() [i.e.,
zero a cacheline, which is much faster than writing zeroes, obviously]
That does not work on uncached memory, and so while I was removing
/that/ attribute from ArmVirtPkg, I realized that WC and WT are not
safe either.
For bare metal platforms, we will keep WC and WT, but for ArmVirtQemu,
they don't make sense. Not sure about Xen though ...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB
2016-09-08 7:57 ` Ard Biesheuvel
@ 2016-09-08 9:38 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2016-09-08 9:38 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On 8 September 2016 at 08:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 September 2016 at 08:54, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/08/16 09:50, Ard Biesheuvel wrote:
>>> In general, on an ARM system, mapping normal memory as device memory may
>>> have unintended side effects, given that unaligned accesses or loads and
>>> stores with special semantics (e.g., load/store exclusive) may fault or
>>> may not work as expected.
>>>
>>> Under KVM, the situation is even worse, since the host may not expect the
>>> guest to perform uncached accesses, and so writes to such an uncached
>>> region may get lost completely.
>>>
>>> Since the only safe mapping type under KVM is EFI_MEMORY_WB, remove all
>>> other memory type attributes.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 3 +--
>>> ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 ---
>>> 2 files changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> index 7fd7e8e9a539..4d56e6236b54 100644
>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> @@ -78,8 +78,7 @@ InitializeHighMemDxe (
>>> Status = gDS->AddMemorySpace (
>>> EfiGcdMemoryTypeSystemMemory,
>>> CurBase, CurSize,
>>> - EFI_MEMORY_WB | EFI_MEMORY_WC |
>>> - EFI_MEMORY_WT | EFI_MEMORY_UC);
>>> + EFI_MEMORY_WB);
>>>
>>> if (EFI_ERROR (Status)) {
>>> DEBUG ((EFI_D_ERROR,
>>> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> index 251e5314e61d..6f3e54b7afcb 100644
>>> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> @@ -68,9 +68,6 @@ MemoryPeim (
>>> ResourceAttributes = (
>>> EFI_RESOURCE_ATTRIBUTE_PRESENT |
>>> EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
>>> - EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>>> - EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>>> - EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>>> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
>>> EFI_RESOURCE_ATTRIBUTE_TESTED
>>> );
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Did you encounter any specific problem with these?
>>
>
> No, but I am looking into using the new optimized BaseMemoryLibOptDxe,
> and the AARCH64 version uses DC ZVA instructions for ZeroMem() [i.e.,
> zero a cacheline, which is much faster than writing zeroes, obviously]
> That does not work on uncached memory, and so while I was removing
> /that/ attribute from ArmVirtPkg, I realized that WC and WT are not
> safe either.
>
> For bare metal platforms, we will keep WC and WT, but for ArmVirtQemu,
> they don't make sense. Not sure about Xen though ...
Pushed, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-08 9:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-08 7:50 [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB Ard Biesheuvel
2016-09-08 7:54 ` Laszlo Ersek
2016-09-08 7:57 ` Ard Biesheuvel
2016-09-08 9:38 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox