ArmVirtPkg: Use ArmTrngLib For Qemu #516
Conversation
Use ArmTrngLib from ArmPkg instead of falling back on the NULL library. Signed-off-by: Ashish Singhal <[email protected]>
| [LibraryClasses.common] | ||
| ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf | ||
| ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf | ||
| ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf |
There was a problem hiding this comment.
I am not sure if this is the right fix. ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf is used for enabling Firmware based TRNG support.
Can you let me know the command line that you are using to launch Qemu, please?
Also is this issue reproducible on edk2 mainline? If so, it would be worth sending a bug fix PR to the edk2 mainline.
There was a problem hiding this comment.
@jpbrucker are you aware of this issue?
There was a problem hiding this comment.
The issue is not visible on the edk2 mainline but is visible on tianocore/edk2#6480. I would expect non RME VM boot to be just fine in both but that is not the case. The error I see is:
EFI stub: Booting Linux Kernel...
SetMemoryAttributes: BaseAddress == 0x54E40000, Length == 0x12B0000, Attributes == 0x20000
SetMemoryAttributes: BaseAddress == 0x560F0000, Length == 0x420000, Attributes == 0x4000
ASSERT [RngDxe] /home/ashishsingha/Workspace/tianocore/PR6480/edk2/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c(86): ((BOOLEAN)(0==1))
The command I used is:
qemu-system-aarch64
-cpu host
-m 512
-smp 1
-enable-kvm
-M virt,gic-version=3
-bios QEMU_EFI.fd
-drive if=none,file=${ROOTFS},format=raw,id=hd0
-device virtio-blk-pci,drive=hd0
-netdev user,id=net0
-device virtio-net-pci,netdev=net0
--nographic
ROOTFS is set to ramdisk device path I booted host from.
There was a problem hiding this comment.
For some reason I am not able to see this issue at all, both on edk2 mainline and edk2-staging/arm-cca.
Here is what I tried:
sudo qemu-system-aarch64 \
-m 1024 \
-nographic \
-pflash flash0.img \
-pflash flash1.img \
-drive if=none,file=plucky-server-cloudimg-arm64.raw,id=hd0 \
-device virtio-blk-pci,drive=hd0 \
-cpu host \
--enable-kvm \
-M virt,gic-version=3 \
-smp 1
Where flash1.img contains Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable
SetMemoryAttributes: BaseAddress == 0x70A00000, Length == 0x2AF0000, Attributes == 0x20000
SetMemoryAttributes: BaseAddress == 0x734F0000, Length == 0x1280000, Attributes == 0x4000
EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
EFI stub: Generating empty DTB
EFI stub: Exiting boot services...
One thing to note is that my host platform does not have the Arm Firmware TRNG interface. Can you confirm if your host platform supports the Arm Firmware TRNG, please?
Other than that, following is what I think is happening.
- ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf is defined in the [LibraryClasses.common] section in ArmVirtPkg/ArmVirt.dsc.inc, see https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/ArmVirt.dsc.inc#L160
- ArmVirt.dsc.inc is included at line No 55 in ArmVirtQemu.dsc, see https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/ArmVirtQemu.dsc#L55
- MdePkg/MdeLibs.dsc.inc defines ArmTrngLib|MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.inf in the [LibraryClasses] section of https://github.com/tianocore/edk2/blob/master/MdePkg/MdeLibs.dsc.inc#L24
- Therefore, ArmTrngLib is overridden by MdePkg/MdeLibs.dsc.inc being including at line 57 in ArmVirtQemu.dsc https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/ArmVirtQemu.dsc#L57
I think this is a side effect of the following patch:
tianocore/edk2@1f1182c#diff-55200ac934bbe6e81bb4158bfff4797992b13fb1afb95fe982853024c6018430
@ardbiesheuvel, @os-d Should the settings for Custom Stack Cookies be moved out to a separate StackCookies.dsc.inc?
There was a problem hiding this comment.
@samimujawar I have no preference, IIRC I originally had just set up ArmVirtQemu.dsc to use dynamic stack cookie support and during review it was asked to be moved to the common dsc.inc file. I am in general against dsc.inc files exactly because of this reason :) they are very easy to order incorrectly and a platform doesn’t explicitly know what they are including. So if it were entirely up to me, I would just add the stack cookie configuration to each dsc directly. But you can certainly have a separate StackCookies.dsc.inc in ArmVirtPkg if you want. OvmfPkg has one, I believe I created in the same PR, for the same reason, it being asked for during review.
I do however believe we should keep any new StackCookies.dsc.inc contained to ArmVirtPkg, not at MdePkg because stack cookie configuration is very platform specific on what to include where.
There was a problem hiding this comment.
@samimujawar Host platform we are using supports TRNG and BaseRngLib tells RNDR instruction is supported as well. I tried reverting the commit you pointed to and that indeed helps with the issue. Also, I was incorrect when I said I see the issue only with the PR. The issue is visible on edk2 mainline as well. As expected, if I disable RNDR support check in BaseRngLib, everything is fine.
There was a problem hiding this comment.
@samimujawar @os-d I have created tianocore/edk2#11052 and it fixes the issues caused by reordering.
There was a problem hiding this comment.
@samimujawar @os-d I have created tianocore/edk2#11052 and it fixes the issues caused by reordering.
There was a problem hiding this comment.
Thanks, @os-d, for approving the PR on edk2. Once @samimujawar approves it, too, and it gets merged, I will CP it to the staging branch we have for cca.
There was a problem hiding this comment.
@ashishsingha I will rebase the staging branch with edk2 mainline to get this change. I think that may be a much cleaner approach.
|
Isn't it the same issue as in tianocore/edk2#10997 ? |
|
@samimujawar tianocore/edk2#11052 has merged. I will close this PR. |
|
@ashishsingha I have rebased the edk2-staging/arm-cca [https://github.com/tianocore/edk2-staging/tree/arm-cca] branch with edk2 mainline. |
4 participants
Use ArmTrngLib from ArmPkg instead of falling back on the NULL library.