comments below (not really my cup of tea but still):
On 10/16/13 19:29, Olivier Martin wrote:
> Right now the ARM Platform driver does not do much, but
> I expect to move most platform specific code into platform
> specific driver in the future.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Olivier Martin <olivier.mar...@arm.com>
> ---
> .../ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c | 49
> +++++++++++++++++++-
> .../ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf | 3 +
> .../ArmVExpress-RTSM-AEMv8Ax4-foundation.dsc | 10 ++++
> .../ArmVExpress-RTSM-AEMv8Ax4-foundation.fdf | 6 ++
> .../ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.dsc | 5 ++
> .../ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.fdf | 1 +
> 6 files changed, 73 insertions(+), 1 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
> b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
> index 5242989..0cf5b7b 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.c
> @@ -13,6 +13,39 @@
> **/
>
> #include <Library/UefiLib.h>
> +#include <Library/VirtioMmioDeviceLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#define ARM_FVP_BASE_VIRTIO_BLOCK_BASE 0x1c130000
> +
> +typedef struct {
> + VENDOR_DEVICE_PATH Vendor;
> + EFI_DEVICE_PATH_PROTOCOL End;
> +} VIRTIO_BLK_DEVICE_PATH;
> +
(1) I think this struct should be enclosed in #pragma pack(1).
Otherwise, in theory, the compiler could insert padding between the two
device path nodes, which would break traversal.
> +VIRTIO_BLK_DEVICE_PATH mVirtioBlockDevicePath =
> +{
> + {
> + {
> + HARDWARE_DEVICE_PATH,
> + HW_VENDOR_DP,
> + {
> + (UINT8)( sizeof(VENDOR_DEVICE_PATH) ),
> + (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8)
> + }
> + },
> + EFI_CALLER_ID_GUID,
> + },
Can you please explain what EFI_CALLER_ID_GUID is? It seems to be
auto-generated, but I can't find "normative" documentation.
Hm... actually, I can. "Build_Spec_1.22_Errata_A_Final.pdf" says
8.3.5.4 Caller ID GUID definition.
The GUID value is the same as INF file GUID. The macro,
EFI_CALLER_ID_GUID, is generated only for non-library module.
So it allows us to refer to the INF GUID in C source.
And, we append no extra bytes after the GUID. OK.
> + {
> + END_DEVICE_PATH_TYPE,
> + END_ENTIRE_DEVICE_PATH_SUBTYPE,
> + {
whitespace damage
> + sizeof (EFI_DEVICE_PATH_PROTOCOL),
> + 0
> + }
> + }
> +};
>
> EFI_STATUS
> EFIAPI
> @@ -21,6 +54,20 @@ ArmFvpInitialise (
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> + EFI_STATUS Status;
> +
> + Status = gBS->InstallProtocolInterface (&ImageHandle,
> + &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE,
> + &mVirtioBlockDevicePath);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + // Declare the Virtio BlockIo device
> + Status = VirtioMmioInstallDevice (ARM_FVP_BASE_VIRTIO_BLOCK_BASE,
> ImageHandle);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR, "ArmFvpDxe: Failed to install Virtio block
> device\n"));
> + }
>
> - return EFI_SUCCESS;
> + return Status;
> }
What exactly is the reason for the new device path? Identification purposes?
Assuming two virtio-blk devices (at different base addresses), if that's
possible for the foundation model, shouldn't we install different vendor
device paths for them?
(Actually, with more than one virtio-blk device, we probably shouldn't
pass the same ImageHandle to VirtioMmioInstallDevice() for all devices
(which would result in several virtio protocol instances on the same
image handle). But we can discuss this later if needed, it's definitely
not a blocker for me.)
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> index 95e6fba..bc74758 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> @@ -26,7 +26,10 @@
> [Packages]
> MdePkg/MdePkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> + OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> UefiDriverEntryPoint
> UefiBootServicesTableLib
> + VirtioMmioDeviceLib
> + BaseMemoryLib
Seems OK I guess (can't see the direct need for BaseMemoryLib, but it
can't hurt, and it's probably an indirect dependency).
> diff --git
> a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.dsc
> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.dsc
> index 4ba827b..c67e695 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.dsc
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.dsc
> @@ -34,6 +34,10 @@
> ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.inf
>
> ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressFoundationLib.inf
>
> + # Virtio Support
> + VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> +
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> +
>
> ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
>
> TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
> @@ -225,6 +229,12 @@
> ArmPkg/Filesystem/SemihostFs/SemihostFs.inf
>
> #
> + # Platform Driver
> + #
> + ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> + OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
> +
> + #
> # FAT filesystem + GPT/MBR partitioning
> #
> MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
> diff --git
> a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.fdf
> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.fdf
> index a358b06..8bb0aa6 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.fdf
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4-foundation.fdf
> @@ -155,6 +155,12 @@ READ_LOCK_STATUS = TRUE
> INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>
> #
> + # Platform Driver
> + #
> + INF ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> + INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
> +
> + #
> # UEFI application (Shell Embedded Boot Loader)
> #
> INF ShellBinPkg/UefiShell/UefiShell.inf
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.dsc
> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.dsc
> index f01f42a..1426d8f 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.dsc
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.dsc
> @@ -40,6 +40,10 @@
>
> TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>
> + # VirtIo Support
> + VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> +
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> +
> [LibraryClasses.common.SEC]
> ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf
>
> ArmPlatformSecLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf
> @@ -258,6 +262,7 @@
> # Platform Driver
> #
> ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> + OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
>
> #
> # FAT filesystem + GPT/MBR partitioning
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.fdf
> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.fdf
> index 4210e6c..577b80b 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.fdf
> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-AEMv8Ax4.fdf
> @@ -168,6 +168,7 @@ READ_LOCK_STATUS = TRUE
> # Platform Driver
> #
> INF ArmPlatformPkg/ArmVExpressPkg/ArmFvpDxe/ArmFvpDxe.inf
> + INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
>
> #
> # UEFI application (Shell Embedded Boot Loader)
>
OK, this part was very confusing, but I think I understand what's going
on. I believe these modifications should be split differently between
09/11 and 10/11. Currently:
- In patch 09, you add the (then-empty) driver to
"ArmVExpress-RTSM-AEMv8Ax4".
- In patch 10, you add the driver to
"ArmVExpress-RTSM-AEMv8Ax4-foundation".
- In patch 10, you also add virtio-blk and its dependencies to both
"ArmVExpress-RTSM-AEMv8Ax4" and
"ArmVExpress-RTSM-AEMv8Ax4-foundation".
I believe patch 09 does too little and patch 10 does too much. Patch 09
should add the (then-empty) driver to both "ArmVExpress-RTSM-AEMv8Ax4"
and "ArmVExpress-RTSM-AEMv8Ax4-foundation". Patch 10 should only add the
virtio-blk stuff to both packages, the platform driver should already be
included by then.
I don't insist of course because I have no idea about the ARM packages.
So, I'm proposing fixing (1), and whatever else you deem appropriate.
Thanks,
Laszlo
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
