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

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

[Ard Biesheuvel]

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

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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.

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

[Shannon Zhao]

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

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

[Ard Biesheuvel]

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.