1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe

Ard Biesheuvel posted 1 patch 7 years, 1 month ago
Failed in applying to current master (apply log)
ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
2 files changed, 4 insertions(+)
[edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe
Posted by Ard Biesheuvel 7 years, 1 month ago 
The VariableRuntimeDxe driver may use CopyMem () on NOR flash regions,
assuming such regions always have full memory semantics. Given that
those regions cannot be mapped as ordinary memory on ARM (due to the
fact that the NOR flash requires device semantics while in write mode)
this prevents us from using BaseMemoryLibOptDxe in VariableRuntimeDxe,
since it may use unaligned accesses and/or DC ZVA instructions, both
of which are incompatible with mappings using device semantics.

Note that there is no way we can work around this by changing the
mapping type between 'memory' and 'device' when switching from read to
write mode and back, because the runtime mapping is created by the OS,
and cannot be changed at will.

So let's just switch to the unaccelerated version of BaseMemoryLib which
does not have the same problem.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 8a60b61f2aa6..7b220d6e3c31 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -261,6 +261,8 @@ [Components.common]
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
+      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   }
 !if $(SECURE_BOOT_ENABLE) == TRUE
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 9a31ec93ca06..7c032e1b07e0 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -252,6 +252,8 @@ [Components.common]
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
+      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   }
 !if $(SECURE_BOOT_ENABLE) == TRUE
   MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe
Posted by Laszlo Ersek 7 years, 1 month ago 
On 11/14/17 11:22, Ard Biesheuvel wrote:
> The VariableRuntimeDxe driver may use CopyMem () on NOR flash regions,
> assuming such regions always have full memory semantics. Given that
> those regions cannot be mapped as ordinary memory on ARM (due to the
> fact that the NOR flash requires device semantics while in write mode)
> this prevents us from using BaseMemoryLibOptDxe in VariableRuntimeDxe,
> since it may use unaligned accesses and/or DC ZVA instructions, both
> of which are incompatible with mappings using device semantics.
> 
> Note that there is no way we can work around this by changing the
> mapping type between 'memory' and 'device' when switching from read to
> write mode and back, because the runtime mapping is created by the OS,
> and cannot be changed at will.
> 
> So let's just switch to the unaccelerated version of BaseMemoryLib which
> does not have the same problem.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 8a60b61f2aa6..7b220d6e3c31 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -261,6 +261,8 @@ [Components.common]
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    }
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 9a31ec93ca06..7c032e1b07e0 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -252,6 +252,8 @@ [Components.common]
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    }
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Given that we've never seen the symptom reported by Shannon, I must
think Shannon is using some kind of new hardware. May I ask what
hardware that is?

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe
Posted by Ard Biesheuvel 7 years, 1 month ago 
On 15 November 2017 at 13:51, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/14/17 11:22, Ard Biesheuvel wrote:
>> The VariableRuntimeDxe driver may use CopyMem () on NOR flash regions,
>> assuming such regions always have full memory semantics. Given that
>> those regions cannot be mapped as ordinary memory on ARM (due to the
>> fact that the NOR flash requires device semantics while in write mode)
>> this prevents us from using BaseMemoryLibOptDxe in VariableRuntimeDxe,
>> since it may use unaligned accesses and/or DC ZVA instructions, both
>> of which are incompatible with mappings using device semantics.
>>
>> Note that there is no way we can work around this by changing the
>> mapping type between 'memory' and 'device' when switching from read to
>> write mode and back, because the runtime mapping is created by the OS,
>> and cannot be changed at will.
>>
>> So let's just switch to the unaccelerated version of BaseMemoryLib which
>> does not have the same problem.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
>>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index 8a60b61f2aa6..7b220d6e3c31 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -261,6 +261,8 @@ [Components.common]
>>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>      <LibraryClasses>
>>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>    }
>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> index 9a31ec93ca06..7c032e1b07e0 100644
>> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> @@ -252,6 +252,8 @@ [Components.common]
>>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>      <LibraryClasses>
>>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>    }
>>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Given that we've never seen the symptom reported by Shannon, I must
> think Shannon is using some kind of new hardware. May I ask what
> hardware that is?
>

I think this is equally likely to occur in any KVM hardware
virtualization context. It is simply a regression after moving to the
OptDxe BaseMemoryLib flavor.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe
Posted by Shannon Zhao 7 years, 1 month ago 

On 2017/11/15 22:03, Ard Biesheuvel wrote:
> On 15 November 2017 at 13:51, Laszlo Ersek <lersek@redhat.com> wrote:
>> > On 11/14/17 11:22, Ard Biesheuvel wrote:
>>> >> The VariableRuntimeDxe driver may use CopyMem () on NOR flash regions,
>>> >> assuming such regions always have full memory semantics. Given that
>>> >> those regions cannot be mapped as ordinary memory on ARM (due to the
>>> >> fact that the NOR flash requires device semantics while in write mode)
>>> >> this prevents us from using BaseMemoryLibOptDxe in VariableRuntimeDxe,
>>> >> since it may use unaligned accesses and/or DC ZVA instructions, both
>>> >> of which are incompatible with mappings using device semantics.
>>> >>
>>> >> Note that there is no way we can work around this by changing the
>>> >> mapping type between 'memory' and 'device' when switching from read to
>>> >> write mode and back, because the runtime mapping is created by the OS,
>>> >> and cannot be changed at will.
>>> >>
>>> >> So let's just switch to the unaccelerated version of BaseMemoryLib which
>>> >> does not have the same problem.
>>> >>
>>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> >> ---
>>> >>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
>>> >>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
>>> >>  2 files changed, 4 insertions(+)
>>> >>
>>> >> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> >> index 8a60b61f2aa6..7b220d6e3c31 100644
>>> >> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> >> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> >> @@ -261,6 +261,8 @@ [Components.common]
>>> >>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>> >>      <LibraryClasses>
>>> >>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>> >> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>>> >> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>> >>    }
>>> >>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>> >>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>> >> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>> >> index 9a31ec93ca06..7c032e1b07e0 100644
>>> >> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>> >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>> >> @@ -252,6 +252,8 @@ [Components.common]
>>> >>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>> >>      <LibraryClasses>
>>> >>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>> >> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>>> >> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>> >>    }
>>> >>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>> >>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>> >>
>> >
>> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> >
>> > Given that we've never seen the symptom reported by Shannon, I must
>> > think Shannon is using some kind of new hardware. May I ask what
>> > hardware that is?
>> >
> I think this is equally likely to occur in any KVM hardware
> virtualization context. It is simply a regression after moving to the
> OptDxe BaseMemoryLib flavor.

Exactly. I'm using Huawei D05. It works well when I use a older edk2
while fails when updating to UDK2017 branch.

Tested-by: Shannon Zhao <zhaoshenglong@huawei.com>

Thanks,
-- 
Shannon

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmVirtPkg/ArmVirtQemu: use non-accelerated CopyMem for VariableRuntimeDxe
Posted by Ard Biesheuvel 7 years, 1 month ago 
On 16 November 2017 at 01:01, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>
> On 2017/11/15 22:03, Ard Biesheuvel wrote:
>> On 15 November 2017 at 13:51, Laszlo Ersek <lersek@redhat.com> wrote:
>>> > On 11/14/17 11:22, Ard Biesheuvel wrote:
>>>> >> The VariableRuntimeDxe driver may use CopyMem () on NOR flash regions,
>>>> >> assuming such regions always have full memory semantics. Given that
>>>> >> those regions cannot be mapped as ordinary memory on ARM (due to the
>>>> >> fact that the NOR flash requires device semantics while in write mode)
>>>> >> this prevents us from using BaseMemoryLibOptDxe in VariableRuntimeDxe,
>>>> >> since it may use unaligned accesses and/or DC ZVA instructions, both
>>>> >> of which are incompatible with mappings using device semantics.
>>>> >>
>>>> >> Note that there is no way we can work around this by changing the
>>>> >> mapping type between 'memory' and 'device' when switching from read to
>>>> >> write mode and back, because the runtime mapping is created by the OS,
>>>> >> and cannot be changed at will.
>>>> >>
>>>> >> So let's just switch to the unaccelerated version of BaseMemoryLib which
>>>> >> does not have the same problem.
>>>> >>
>>>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> >> ---
>>>> >>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 ++
>>>> >>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
>>>> >>  2 files changed, 4 insertions(+)
>>>> >>
>>>> >> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>>> >> index 8a60b61f2aa6..7b220d6e3c31 100644
>>>> >> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>>> >> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>>> >> @@ -261,6 +261,8 @@ [Components.common]
>>>> >>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>>> >>      <LibraryClasses>
>>>> >>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>>> >> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>>>> >> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>>> >>    }
>>>> >>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>>> >>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>>> >> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>>> >> index 9a31ec93ca06..7c032e1b07e0 100644
>>>> >> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>>> >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>>>> >> @@ -252,6 +252,8 @@ [Components.common]
>>>> >>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>>> >>      <LibraryClasses>
>>>> >>        NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>>> >> +      # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
>>>> >> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>>> >>    }
>>>> >>  !if $(SECURE_BOOT_ENABLE) == TRUE
>>>> >>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>>>> >>
>>> >
>>> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> >
>>> > Given that we've never seen the symptom reported by Shannon, I must
>>> > think Shannon is using some kind of new hardware. May I ask what
>>> > hardware that is?
>>> >
>> I think this is equally likely to occur in any KVM hardware
>> virtualization context. It is simply a regression after moving to the
>> OptDxe BaseMemoryLib flavor.
>
> Exactly. I'm using Huawei D05. It works well when I use a older edk2
> while fails when updating to UDK2017 branch.
>
> Tested-by: Shannon Zhao <zhaoshenglong@huawei.com>
>

Pushed as 44d71c217ccb

Thanks.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.