| Message ID | 53FFE37E.8000405@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 29 August 2014 04:20, Laszlo Ersek <lersek@redhat.com> wrote: > Apologies, I have some updates here: > > On 08/29/14 01:17, Laszlo Ersek wrote: >> On 08/28/14 17:40, Ard Biesheuvel wrote: >> > >>> + APRIORI DXE { >>> + INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf >>> + } >>> + INF MdeModulePkg/Core/Dxe/DxeMain.inf >>> + INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf >>> + INF ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf >>> + >>> + # >>> + # PI DXE Drivers producing Architectural Protocols (EFI Services) >>> + # >>> + INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf >>> + INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf >>> + INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf >>> + INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf >>> + INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf >>> + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >> >> from the prev. norflash patch, ok... >> >>> + INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf >>> + INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf >>> + INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf >>> + INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf >>> + INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >>> + >>> + # >>> + # Multiple Console IO support >>> + # >>> + INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf >>> + INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf >>> + INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf >>> + INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf >>> + INF EmbeddedPkg/SerialDxe/SerialDxe.inf >>> + >>> + INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf >>> + INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf >>> + INF ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf >> >> ditto, OK >> >>> + INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf >>> + >>> + # >>> + # FAT filesystem + GPT/MBR partitioning >>> + # >>> + INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf >>> + INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf >>> + INF FatBinPkg/EnhancedFatDxe/Fat.inf >>> + INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >>> + >>> + # >>> + # Platform Driver >>> + # >>> + INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf >>> + INF OvmfPkg/VirtioNetDxe/VirtioNet.inf >>> + INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf >>> + >>> + # >>> + # UEFI application (Shell Embedded Boot Loader) >>> + # >>> + INF ShellBinPkg/UefiShell/UefiShell.inf >>> + >>> + # >>> + # Bds >>> + # >>> + INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf >>> + INF ArmPlatformPkg/Bds/Bds.inf > > I. > > So, VirtFdtDxe sets the following PCDs, keying off the DTB: > > - PcdGicDistributorBase > - PcdGicInterruptInterfaceBase > - PcdPL031RtcBase > - PcdArmArchTimerSecIntrNum > - PcdArmArchTimerIntrNum > - PcdArmArchTimerVirtIntrNum > - PcdArmArchTimerHypIntrNum > > It is imperative that DXE drivers that depend on these PCDs run *after* > VirtFdtDxe. > > Ordering between DXE drivers can be ensured in several ways: > - the APRIORI DXE file > - Depex (some drivers produce protocols, and others depend on them) > - protocol installation callbacks (gBS->RegisterProtocolNotify()) > - theoretically, PcdSetXX() callbacks (I've never seen any use of this) > > Of these, I recommend the APRIORI DXE file, because > - there are no suitable protocols here, > - we're delaying cross-ARM-platform drivers, ie. nothing that's > specific to virt > > Please squash the first attached patch. > > I tested it. It makes no difference *in practice*, right now. The first > DXE driver that is loaded is PcdDxe.efi, the 2nd one is VirtFdtDxe.efi, > anyway. > > But that only happens because you placed the VirtFdtDxe line in the FDF > file quite "high". The ordering of the module lines in the FDF file, in > an [FV] section, has unspecified effect; you just got lucky that it > ended up creating a load order that you wanted. > > Again, this is not guaranteed, plus module lines can be reshuffled any > time in the [FV] section of the FDF. If you want to prescribe a dispatch > order, you must use the APRIORI file. Whatever drivers you reference > there will be dispatched first from the firmware volume, in the order > you listed them in the APRIORI file, and then the rest will be > dispatched, in unspecified order (subject to Depexes of course). > > Please refer to "2.4.4 APRIORI Scoping" in the FDF spec. > > So, please squash this patch -- it has no effect right now (luckily), > but it's needed for safety down the road. You can keep my Reviewed-by of > course. > OK. I did have it in the APRIORI section at some point, and noticed it didn't make a difference. It did for dynamic PCDs (none of the ARM platform use them), so I thought I couid drop it. But I will put it back > II. > > I compared the list of modules you build in the DSC file(s) against the > modules you include in the flash image with the FDF file. There are two > discrepancies: > > - EmbeddedPkg/SimpleTextInOutSerial/SimpleTextInOutSerial.inf is built > uselessly (simply drop it from the DSC) > - the networking modules that are built through the dsc.inc are not > included by the FDF. They should be. > > IOW, please squash the 2nd patch too. > OK > III. > > Notes about testing: > > - this series doesn't apply on current edk2 master; please rebase it for > v6. For testing, I applied it on git commit 421ccda3. > Yes, that is the branch point I used after rebasing the v1. Unfortunately, I am now seeing this build.py... /home/ard/build/uefi-next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...): error 1001: Module type [SEC] is not supported by library instance [/home/ard/build/uefi-next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf] consumed by [/home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] which i am trying to bisect now. > - the first QEMU patch that you link in > <https://wiki.linaro.org/LEG/UEFIforQEMU> doesn't apply on current QEMU > master (git-am can't cope with the context changes); I'm attaching a > forward-ported version. Maybe you can add it to the wiki page. > > IV. > > More notes about testing -- try this (if you haven't yet): > > (a) as root, on your host: > > G=$(id -g $YOUR_NORMAL_USER) > sysctl -w net.ipv4.ping_group_range="$G $G" > > (b) as your normal user: > > qemu-system-aarch64 \ > -m 1024 \ > -cpu cortex-a57 \ > -M virt \ > -bios Build/ArmVirtualizationQemu/DEBUG_GCC48/FV/QEMU_EFI.fd \ > -serial stdio \ > -netdev user,id=netdev0,hostname=aarch64-guest \ > -device virtio-net-device,netdev=netdev0 > > (c) drop to the UEFI shell > > (d) Shell> ifconfig -s eth0 dhcp > > (e) Shell> ping FAVORITE_PUBLIC_IPv4_ADDRESS > Shell> ifconfig -s eth0 dhcp Create an IP and start to get the default address Please wait, your console may stop responding for a while ... Default: 10.0.2.15 Shell> ping 8.8.8.8 Ping 8.8.8.8 16 data bytes 16 bytes from 8.8.8.8 : icmp_seq=1 ttl=0 time<0ms 16 bytes from 8.8.8.8 : icmp_seq=2 ttl=0 time<0ms 16 bytes from 8.8.8.8 : icmp_seq=3 ttl=0 time<0ms 16 bytes from 8.8.8.8 : icmp_seq=4 ttl=0 time<0ms 16 bytes from 8.8.8.8 : icmp_seq=5 ttl=0 time<0ms > This is what I've been wanting to see for months! :) > > - side note: backspace works perfectly for me in my xterm (my erase > character has been ^H for ~15 yrs) > I got used to it, and don't use the shell that much, so it doesn't bother me
On 29 August 2014 09:53, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 29 August 2014 04:20, Laszlo Ersek <lersek@redhat.com> wrote: >> Apologies, I have some updates here: >> >> On 08/29/14 01:17, Laszlo Ersek wrote: >>> On 08/28/14 17:40, Ard Biesheuvel wrote: >>> >> >>>> + APRIORI DXE { >>>> + INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf >>>> + } >>>> + INF MdeModulePkg/Core/Dxe/DxeMain.inf >>>> + INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf >>>> + INF ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf >>>> + >>>> + # >>>> + # PI DXE Drivers producing Architectural Protocols (EFI Services) >>>> + # >>>> + INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf >>>> + INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf >>>> + INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf >>>> + INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf >>>> + INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf >>>> + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf >>> >>> from the prev. norflash patch, ok... >>> >>>> + INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf >>>> + INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf >>>> + INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf >>>> + INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf >>>> + INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >>>> + >>>> + # >>>> + # Multiple Console IO support >>>> + # >>>> + INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf >>>> + INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf >>>> + INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf >>>> + INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf >>>> + INF EmbeddedPkg/SerialDxe/SerialDxe.inf >>>> + >>>> + INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf >>>> + INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf >>>> + INF ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf >>> >>> ditto, OK >>> >>>> + INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf >>>> + >>>> + # >>>> + # FAT filesystem + GPT/MBR partitioning >>>> + # >>>> + INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf >>>> + INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf >>>> + INF FatBinPkg/EnhancedFatDxe/Fat.inf >>>> + INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >>>> + >>>> + # >>>> + # Platform Driver >>>> + # >>>> + INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf >>>> + INF OvmfPkg/VirtioNetDxe/VirtioNet.inf >>>> + INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf >>>> + >>>> + # >>>> + # UEFI application (Shell Embedded Boot Loader) >>>> + # >>>> + INF ShellBinPkg/UefiShell/UefiShell.inf >>>> + >>>> + # >>>> + # Bds >>>> + # >>>> + INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf >>>> + INF ArmPlatformPkg/Bds/Bds.inf >> >> I. >> >> So, VirtFdtDxe sets the following PCDs, keying off the DTB: >> >> - PcdGicDistributorBase >> - PcdGicInterruptInterfaceBase >> - PcdPL031RtcBase >> - PcdArmArchTimerSecIntrNum >> - PcdArmArchTimerIntrNum >> - PcdArmArchTimerVirtIntrNum >> - PcdArmArchTimerHypIntrNum >> >> It is imperative that DXE drivers that depend on these PCDs run *after* >> VirtFdtDxe. >> >> Ordering between DXE drivers can be ensured in several ways: >> - the APRIORI DXE file >> - Depex (some drivers produce protocols, and others depend on them) >> - protocol installation callbacks (gBS->RegisterProtocolNotify()) >> - theoretically, PcdSetXX() callbacks (I've never seen any use of this) >> >> Of these, I recommend the APRIORI DXE file, because >> - there are no suitable protocols here, >> - we're delaying cross-ARM-platform drivers, ie. nothing that's >> specific to virt >> >> Please squash the first attached patch. >> >> I tested it. It makes no difference *in practice*, right now. The first >> DXE driver that is loaded is PcdDxe.efi, the 2nd one is VirtFdtDxe.efi, >> anyway. >> >> But that only happens because you placed the VirtFdtDxe line in the FDF >> file quite "high". The ordering of the module lines in the FDF file, in >> an [FV] section, has unspecified effect; you just got lucky that it >> ended up creating a load order that you wanted. >> >> Again, this is not guaranteed, plus module lines can be reshuffled any >> time in the [FV] section of the FDF. If you want to prescribe a dispatch >> order, you must use the APRIORI file. Whatever drivers you reference >> there will be dispatched first from the firmware volume, in the order >> you listed them in the APRIORI file, and then the rest will be >> dispatched, in unspecified order (subject to Depexes of course). >> >> Please refer to "2.4.4 APRIORI Scoping" in the FDF spec. >> >> So, please squash this patch -- it has no effect right now (luckily), >> but it's needed for safety down the road. You can keep my Reviewed-by of >> course. >> > > OK. > > I did have it in the APRIORI section at some point, and noticed it > didn't make a difference. It did for dynamic PCDs (none of the ARM > platform use them), so I thought I couid drop it. But I will put it > back > >> II. >> >> I compared the list of modules you build in the DSC file(s) against the >> modules you include in the flash image with the FDF file. There are two >> discrepancies: >> >> - EmbeddedPkg/SimpleTextInOutSerial/SimpleTextInOutSerial.inf is built >> uselessly (simply drop it from the DSC) >> - the networking modules that are built through the dsc.inc are not >> included by the FDF. They should be. >> >> IOW, please squash the 2nd patch too. >> > > OK > >> III. >> >> Notes about testing: >> >> - this series doesn't apply on current edk2 master; please rebase it for >> v6. For testing, I applied it on git commit 421ccda3. >> > > Yes, that is the branch point I used after rebasing the v1. > Unfortunately, I am now seeing this > > build.py... > /home/ard/build/uefi-next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...): > error 1001: Module type [SEC] is not supported by library instance > [/home/ard/build/uefi-next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf] > consumed by [/home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] > > which i am trying to bisect now. > OK, it turns out Olivier has submitted an incompatible change here: af16798ef77da84487ed8e64bc955fbd12ac9b1f [EmbeddedPkg/FdtLib: Added support to load Fdt from Semihosting] which adds functionality to FdtLib which makes it depend on the boot services table. I am going to go ahead and split that into two libraries, hopefully Olivier will be ok with that once he gets around to looking into this stuff >> - the first QEMU patch that you link in >> <https://wiki.linaro.org/LEG/UEFIforQEMU> doesn't apply on current QEMU >> master (git-am can't cope with the context changes); I'm attaching a >> forward-ported version. Maybe you can add it to the wiki page. >> >> IV. >> >> More notes about testing -- try this (if you haven't yet): >> >> (a) as root, on your host: >> >> G=$(id -g $YOUR_NORMAL_USER) >> sysctl -w net.ipv4.ping_group_range="$G $G" >> >> (b) as your normal user: >> >> qemu-system-aarch64 \ >> -m 1024 \ >> -cpu cortex-a57 \ >> -M virt \ >> -bios Build/ArmVirtualizationQemu/DEBUG_GCC48/FV/QEMU_EFI.fd \ >> -serial stdio \ >> -netdev user,id=netdev0,hostname=aarch64-guest \ >> -device virtio-net-device,netdev=netdev0 >> >> (c) drop to the UEFI shell >> >> (d) Shell> ifconfig -s eth0 dhcp >> >> (e) Shell> ping FAVORITE_PUBLIC_IPv4_ADDRESS >> > > Shell> ifconfig -s eth0 dhcp > Create an IP and start to get the default address > Please wait, your console may stop responding for a while ... > Default: 10.0.2.15 > Shell> ping 8.8.8.8 > Ping 8.8.8.8 16 data bytes > 16 bytes from 8.8.8.8 : icmp_seq=1 ttl=0 time<0ms > 16 bytes from 8.8.8.8 : icmp_seq=2 ttl=0 time<0ms > 16 bytes from 8.8.8.8 : icmp_seq=3 ttl=0 time<0ms > 16 bytes from 8.8.8.8 : icmp_seq=4 ttl=0 time<0ms > 16 bytes from 8.8.8.8 : icmp_seq=5 ttl=0 time<0ms > > >> This is what I've been wanting to see for months! :) >> >> - side note: backspace works perfectly for me in my xterm (my erase >> character has been ^H for ~15 yrs) >> > > I got used to it, and don't use the shell that much, so it doesn't bother me > > -- > Ard. ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/29/14 09:53, Ard Biesheuvel wrote: > On 29 August 2014 04:20, Laszlo Ersek <lersek@redhat.com> wrote: >> III. >> >> Notes about testing: >> >> - this series doesn't apply on current edk2 master; please rebase it for >> v6. For testing, I applied it on git commit 421ccda3. >> > > Yes, that is the branch point I used after rebasing the v1. > Unfortunately, I am now seeing this > > build.py... > /home/ard/build/uefi-next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...): > error 1001: Module type [SEC] is not supported by library instance > [/home/ard/build/uefi-next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf] > consumed by [/home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] > > which i am trying to bisect now. Hm. Interesting. UefiBootServicesTableLib is *genuinely* unusable in a SEC module. I can think of two possibilities: (a) The bug was always there in the series, and BuildTools didn't notice only because UefiBootServicesTableLib didn't communicate its module type restriction. Note that even this way UefiBootServicesTableLib could have never worked in a SEC module. In other words, you may have a dependency on it somewhere, in an INF file, but you never actually use anything from that library. So it should be a matter of dropping the reference in some INF file's [LibraryClasses] section. (b) Some generic ARM module that your series depends on (in a SEC module) introduced a new dependency on UefiBootServicesTableLib in the interim. Hence your code now inherits this dependency for the SEC module. This is invalid of course. In this case you might have to fork the generic ARM module for the virt Pkg. I wonder if adding --report-file=build.report to your build command line could help you see the library dependency chains. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/29/14 10:14, Ard Biesheuvel wrote: > On 29 August 2014 09:53, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> build.py... >> /home/ard/build/uefi-next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...): >> error 1001: Module type [SEC] is not supported by library instance >> [/home/ard/build/uefi-next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf] >> consumed by [/home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] >> >> which i am trying to bisect now. >> > > OK, it turns out Olivier has submitted an incompatible change here: > > af16798ef77da84487ed8e64bc955fbd12ac9b1f > [EmbeddedPkg/FdtLib: Added support to load Fdt from Semihosting] > > which adds functionality to FdtLib which makes it depend on the boot > services table. > I am going to go ahead and split that into two libraries, hopefully > Olivier will be ok with that once he gets around to looking into this > stuff That's case (2) then. And, indeed, it's *very* important to keep FdtLib a BASE library, with no dependencies on any UEFI-phase-specifics. I recently had a short email chat with David Gibson, where I said Wow, David (CC'd :)) *actually* meant this code to be embedded in firmware. and he said Yes, yes I did. [...] I deliberately kept the dependencies in libfdt to an absolute minimum to make it easy to use in limited environments. It uses a small handful of string.h functions, and that's it. No malloc required, no stdio required. So that's how it should remain in edk2 as well. (At least, such a compatible version should remain available.) Hm, let's see af16798ef77da84487ed8e64bc955fbd12ac9b1f. - New function InstallFdtIntoConfigurationTable() is not needed for modules that only want to parse the DTB. For a SEC module eg., the UEFI configuration table doesn't exist. - New function InstallFdtFromSemihosting() is similarly unrelated to DTB parsing. It's a way to retrieve the DTB, and just as use-case specific as our own peculiar way in virt. Doesn't belong to the base library. My proposal (trivial, of course): - revert this commit - simply introduce another library (new lib class even) in EmbeddedPkg - add a lib instance implementing this class, with these new functions - new lib instance should depend on (unmodified) FdtLib - add new lib class dependency to originally dependent modules Thanks! Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 29 August 2014 12:58, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/29/14 10:14, Ard Biesheuvel wrote: >> On 29 August 2014 09:53, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >>> build.py... >>> /home/ard/build/uefi-next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...): >>> error 1001: Module type [SEC] is not supported by library instance >>> [/home/ard/build/uefi-next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf] >>> consumed by [/home/ard/build/uefi-next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] >>> >>> which i am trying to bisect now. >>> >> >> OK, it turns out Olivier has submitted an incompatible change here: >> >> af16798ef77da84487ed8e64bc955fbd12ac9b1f >> [EmbeddedPkg/FdtLib: Added support to load Fdt from Semihosting] >> >> which adds functionality to FdtLib which makes it depend on the boot >> services table. >> I am going to go ahead and split that into two libraries, hopefully >> Olivier will be ok with that once he gets around to looking into this >> stuff > > That's case (2) then. > > And, indeed, it's *very* important to keep FdtLib a BASE library, with > no dependencies on any UEFI-phase-specifics. I recently had a short > email chat with David Gibson, where I said > > Wow, David (CC'd :)) *actually* meant this code to be embedded in > firmware. > > and he said > > Yes, yes I did. [...] > > I deliberately kept the dependencies in libfdt to an absolute > minimum to make it easy to use in limited environments. It uses a > small handful of string.h functions, and that's it. No malloc > required, no stdio required. > > So that's how it should remain in edk2 as well. (At least, such a > compatible version should remain available.) > > Hm, let's see af16798ef77da84487ed8e64bc955fbd12ac9b1f. > > - New function InstallFdtIntoConfigurationTable() is not needed for > modules that only want to parse the DTB. For a SEC module eg., the UEFI > configuration table doesn't exist. > > - New function InstallFdtFromSemihosting() is similarly unrelated to DTB > parsing. It's a way to retrieve the DTB, and just as use-case specific > as our own peculiar way in virt. Doesn't belong to the base library. > > My proposal (trivial, of course): > - revert this commit > - simply introduce another library (new lib class even) in EmbeddedPkg > - add a lib instance implementing this class, with these new functions > - new lib instance should depend on (unmodified) FdtLib > - add new lib class dependency to originally dependent modules > @Olivier: how do you propose we handle this? FdtLib has now become unusable in SEC or PEI
Yeap, splitting FdtLib into FdtLib and FdtLoadLib is fine for me. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 02 September 2014 13:53 > To: Laszlo Ersek > Cc: edk2-devel@lists.sourceforge.net; Olivier Martin; Peter Maydell; > Andrew Jones; Christoffer Dall; Ilias Biris; David Gibson > Subject: Re: [edk2] [PATCH v5 16/16] ArmVirtualizationPkg: add > ArmVirtualizationQemu platform > > On 29 August 2014 12:58, Laszlo Ersek <lersek@redhat.com> wrote: > > On 08/29/14 10:14, Ard Biesheuvel wrote: > >> On 29 August 2014 09:53, Ard Biesheuvel <ard.biesheuvel@linaro.org> > wrote: > > > >>> build.py... > >>> /home/ard/build/uefi- > next/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc(...) > : > >>> error 1001: Module type [SEC] is not supported by library instance > >>> [/home/ard/build/uefi- > next/MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.i > nf] > >>> consumed by [/home/ard/build/uefi- > next/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf] > >>> > >>> which i am trying to bisect now. > >>> > >> > >> OK, it turns out Olivier has submitted an incompatible change here: > >> > >> af16798ef77da84487ed8e64bc955fbd12ac9b1f > >> [EmbeddedPkg/FdtLib: Added support to load Fdt from Semihosting] > >> > >> which adds functionality to FdtLib which makes it depend on the boot > >> services table. > >> I am going to go ahead and split that into two libraries, hopefully > >> Olivier will be ok with that once he gets around to looking into > this > >> stuff > > > > That's case (2) then. > > > > And, indeed, it's *very* important to keep FdtLib a BASE library, > with > > no dependencies on any UEFI-phase-specifics. I recently had a short > > email chat with David Gibson, where I said > > > > Wow, David (CC'd :)) *actually* meant this code to be embedded in > > firmware. > > > > and he said > > > > Yes, yes I did. [...] > > > > I deliberately kept the dependencies in libfdt to an absolute > > minimum to make it easy to use in limited environments. It uses a > > small handful of string.h functions, and that's it. No malloc > > required, no stdio required. > > > > So that's how it should remain in edk2 as well. (At least, such a > > compatible version should remain available.) > > > > Hm, let's see af16798ef77da84487ed8e64bc955fbd12ac9b1f. > > > > - New function InstallFdtIntoConfigurationTable() is not needed for > > modules that only want to parse the DTB. For a SEC module eg., the > UEFI > > configuration table doesn't exist. > > > > - New function InstallFdtFromSemihosting() is similarly unrelated to > DTB > > parsing. It's a way to retrieve the DTB, and just as use-case > specific > > as our own peculiar way in virt. Doesn't belong to the base library. > > > > My proposal (trivial, of course): > > - revert this commit > > - simply introduce another library (new lib class even) in > EmbeddedPkg > > - add a lib instance implementing this class, with these new > functions > > - new lib instance should depend on (unmodified) FdtLib > > - add new lib class dependency to originally dependent modules > > > > @Olivier: how do you propose we handle this? FdtLib has now become > unusable in SEC or PEI > > -- > Ard. -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782 ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
>From 43c2037be5c25654656b75a1f237e23f7bb12e1e Mon Sep 17 00:00:00 2001 From: Peter Maydell <peter.maydell@linaro.org> Date: Fri, 29 Aug 2014 02:57:06 +0200 Subject: [PATCH] hw/arm/virt: Provide flash devices for boot ROMs Add two flash devices to the virt board, so that it can be used for running guests which want a bootrom image such as UEFI. We provide two flash devices to make it more convenient to provide both a read-only UEFI image and a read-write place to store guest-set UEFI config variables. The '-bios' command line option is set up to provide an image for the first of the two flash devices. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/arm/virt.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index bd206a0..61597c2 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -37,6 +37,7 @@ #include "sysemu/sysemu.h" #include "sysemu/kvm.h" #include "hw/boards.h" +#include "hw/loader.h" #include "exec/address-spaces.h" #include "qemu/bitops.h" #include "qemu/error-report.h" @@ -97,7 +98,6 @@ typedef struct VirtBoardInfo { * to accommodate guests using 64K pages. */ static const MemMapEntry a15memmap[] = { - /* Space up to 0x8000000 is reserved for a boot ROM */ [VIRT_FLASH] = { 0, 0x08000000 }, [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 }, /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */ @@ -437,6 +437,73 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic) } } +static void create_one_flash(const char *name, hwaddr flashbase, + hwaddr flashsize) +{ + /* Create and map a single flash device. We use the same + * parameters as the flash devices on the Versatile Express board. + */ + DriveInfo *dinfo = drive_get_next(IF_PFLASH); + DeviceState *dev = qdev_create(NULL, "cfi.pflash01"); + const uint64_t sectorlength = 256 * 1024; + + if (dinfo && qdev_prop_set_drive(dev, "drive", dinfo->bdrv)) { + abort(); + } + + qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength); + qdev_prop_set_uint64(dev, "sector-length", sectorlength); + qdev_prop_set_uint8(dev, "width", 4); + qdev_prop_set_uint8(dev, "device-width", 2); + qdev_prop_set_uint8(dev, "big-endian", 0); + qdev_prop_set_uint16(dev, "id0", 0x89); + qdev_prop_set_uint16(dev, "id1", 0x18); + qdev_prop_set_uint16(dev, "id2", 0x00); + qdev_prop_set_uint16(dev, "id3", 0x00); + qdev_prop_set_string(dev, "name", name); + qdev_init_nofail(dev); + + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, flashbase); +} + +static void create_flash(const VirtBoardInfo *vbi) +{ + /* Create two flash devices to fill the VIRT_FLASH space in the memmap. + * Any file passed via -bios goes in the first of these. + */ + hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2; + hwaddr flashbase = vbi->memmap[VIRT_FLASH].base; + char *nodename; + + if (bios_name) { + const char *fn; + + if (drive_get(IF_PFLASH, 0, 0)) { + error_report("The contents of the first flash device may be " + "specified with -bios or with -drive if=pflash... " + "but you cannot use both options at once"); + exit(1); + } + fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); + if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) { + error_report("Could not load ROM image '%s'", bios_name); + exit(1); + } + } + + create_one_flash("virt.flash0", flashbase, flashsize); + create_one_flash("virt.flash1", flashbase + flashsize, flashsize); + + nodename = g_strdup_printf("/flash@%" PRIx64, flashbase); + qemu_fdt_add_subnode(vbi->fdt, nodename); + qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash"); + qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", + 2, flashbase, 2, flashsize, + 2, flashbase + flashsize, 2, flashsize); + qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4); + g_free(nodename); +} + static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) { const VirtBoardInfo *board = (const VirtBoardInfo *)binfo; @@ -514,6 +581,8 @@ static void machvirt_init(MachineState *machine) vmstate_register_ram_global(ram); memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram); + create_flash(vbi); + create_gic(vbi, pic); create_uart(vbi, pic); -- 1.8.3.1