| Message ID | 1459959319-19293-7-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 04/06/16 18:15, Ard Biesheuvel wrote: > Instead of relying on VirtFdtDxe to populate the GIC related PCDs, move > this handling to our implementation of ArmGicArchLib, and retrieve the > required DT info using the new FDT client protocol. > > This removes one of the reasons we need to load VirtFdtDxe first using > an 'A PRIORI' declaration in the platform FDF. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 84 ++++++++++++++++++-- > ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 15 +++- > 2 files changed, 92 insertions(+), 7 deletions(-) Urgh. I guess the conversion (together with the next patch) should be "straightforward". However, before I try to validate the code movement, I have a small and a big question. (21) small one: what do you need <libfdt.h>, FdtLib, and (consequently) EmbeddedPkg/EmbeddedPkg.dec for? ... Nevermind, coming back from the source code below, I understand. I'll make a suggestion down there. Consider (21) a no-op. (22) The big question is this: how can I determine where this library instance is used? Namely, before the conversion, PcdGic*Base would be set only once (in VirtFdtDxe). That means two things: (a) it is set *for sure*, for any DXE driver that might want to look at the PCDs, plus (b) the PCDs are not set more than once. I can't see how this conversion guarantees either. Let's start with question (b). (Breaking (b) is not a catastrophe, but it's ugly.) I tried to round up users of the lib class ArmGicArchLib. I found two client INF files: - ArmPkg/Drivers/ArmGic/ArmGicLib.inf - ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So, I guess I have to find the clients of the lib class ArmGicLib. They are: - ArmPkg/Application/LinuxLoader/LinuxLoader.inf We use this in our ARM (32-bit) builds. It means whenever this app is run, it will re-set the PCDs. :/ ... Although, it looks like the application only depends on ArmGicLib in the AARCH64 case, precisely which architecture we don't build the application for. Okay, ultimately, but confusing... - ArmPkg/Drivers/ArmGic/ArmGicDxe.inf I guess this is our main target. This one is therefore certainly correct to set the PCDs. - ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf We never include this driver. Okay. - ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf - ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf - ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf What the heck, all of these library instances are for class ArmGicArchLib, which is what I started out with! We actually use the last one (this patch modifies exactly it); it creates a circular library dependency between ArmGicLib and ArmGicArchLib. Sweet $DEITY. Anyway, since I started out tracking down ArmGicArchLib, and we don't use the first two of the above, they don't add new things to follow. - ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf We have a library class resolution to this, in "ArmVirt.dsc.inc", for ArmPlatformSecExtraActionLib. That's a dead-end, thankfully; grepping for the lib class, only "ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/ArmVExpressSecLib.inf" depends on it, which I can't imagine we use. The resolution should be dropped from "ArmVirtPkg/ArmVirt.dsc.inc", probably. - ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf - ArmPlatformPkg/PrePi/PeiMPCore.inf These are SEC modules, and we don't use them. Okay. So, ultimately, the only user of this library instance is "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". ... Indeed, checking the build report file for ArmVirtQemu (AARCH64), I find ArmVirtGicArchLib (and ArmGicLib too) only under "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". Question (22)(b) is therefore cleared; the PCDs in question will only be set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched. Let's see (22)(a): can anything consume these PCDs before "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" sets them? - ArmPkg/Application/LinuxLoader/LinuxLoader.inf Same as above: uses the PCDs only on AARCH64, when we don't build it. - ArmPkg/Drivers/ArmGic/ArmGicDxe.inf The library sets the PCDs for it in time. - ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf We don't use this (it wants the PCDs as Fixed, anyway). - ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf - ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf - ArmPlatformPkg/PrePi/PeiMPCore.inf Ditto for these three. Alright! It was exhausting to verify it (you could have written it up too... :)), but the conversion seems safe. Ultimately, as (22), I'll make this request: (22) please document in the commit message, that in all our builds, only "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" links against this library, and only "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" consumes the PCDs. ... This is actually a "standard requirement" for the rest of the plugin-like patches, more or less. Do you think (knowing the rest of the series) that it might make sense for you to post a v2 after my comments thus far? Mainly, this "usage analysis" is exhausting, and for the rest of the series, I'd prefer to validate yours, not construct mine. :) Anyway, let's see the rest of this patch (INF file moved to the top -- can you perhaps adopt a diff-order file, for formatting the patches?): > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf > index c85b2d44d856..57086242de1f 100644 > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf > @@ -18,7 +18,7 @@ [Defines] > INF_VERSION = 0x00010005 > BASE_NAME = ArmVirtGicArchLib > FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed > - MODULE_TYPE = BASE > + MODULE_TYPE = DXE_DRIVER (23) I don't think it's necessary. You use neither ImageHandle nor SystemTable below. The client type restriction (on the LIBRARY_CLASS line below) suffices. > VERSION_STRING = 1.0 > LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION > CONSTRUCTOR = ArmVirtGicArchLibConstructor > @@ -30,11 +30,22 @@ [LibraryClasses] > PcdLib > DebugLib > ArmGicLib > + UefiBootServicesTableLib > + FdtLib > > [Packages] > MdePkg/MdePkg.dec > ArmPkg/ArmPkg.dec > ArmVirtPkg/ArmVirtPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + > +[Protocols] > + gFdtClientProtocolGuid > > [Pcd] > - gArmVirtTokenSpaceGuid.PcdArmGicRevision > + gArmTokenSpaceGuid.PcdGicDistributorBase > + gArmTokenSpaceGuid.PcdGicRedistributorsBase > + gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase > + > +[Depex] > + gFdtClientProtocolGuid > Looks okay (modulo (21) above). > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > index 732860cadfe6..686622228831 100644 > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > @@ -1,7 +1,7 @@ > /** @file > ArmGicArchLib library class implementation for DT based virt platforms > > - Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> > + Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR> > > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -19,21 +19,83 @@ > #include <Library/ArmGicArchLib.h> > #include <Library/PcdLib.h> > #include <Library/DebugLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <libfdt.h> > + > +#include <Protocol/FdtClient.h> > > STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; > > -RETURN_STATUS > +EFI_STATUS > EFIAPI > ArmVirtGicArchLibConstructor ( > - VOID > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > ) > { > - UINT32 IccSre; > + UINT32 IccSre; > + FDT_CLIENT_PROTOCOL *FdtClient; > + CONST VOID *Reg; > + UINTN RegElemSize, RegSize; > + UINTN GicRevision; > + EFI_STATUS Status; > + UINT64 DistBase, CpuBase, RedistBase; > > - switch (PcdGet32 (PcdArmGicRevision)) { > + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID **)&FdtClient); (24) This line is too long. Can you double-check the series that added lines don't exceed 79 characters? (I think "BaseTools/Scripts/PatchCheck.py" might help you verify this -- see a7e173b07a1e.) I guess I shouldn't obsess about code that is moved, but code you write genuinely for this work shouldn't be too wide. > + if (EFI_ERROR (Status)) { > + return Status; > + } (25) Given the DepEx, you might want to replace this with an ASSERT_EFI_ERROR(). Up to you. > + > + GicRevision = 2; > + Status = FdtClient->FindCompatibleNodeReg (FdtClient, > + "arm,cortex-a15-gic", > + &Reg, > + &RegElemSize, > + &RegSize); (26) The indentation of the arguments is not correct; they should be aligned with "FindCompatibleNodeReg", plus one or two spaces. Can you re-check the series for such indentation problems? > + if (Status == EFI_NOT_FOUND) { > + GicRevision = 3; > + Status = FdtClient->FindCompatibleNodeReg (FdtClient, > + "arm,gic-v3", > + &Reg, > + &RegElemSize, > + &RegSize); > + } > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + switch (GicRevision) { > > case 3: > // > + // The GIC v3 DT binding describes a series of at least 3 physical (base > + // addresses, size) pairs: the distributor interface (GICD), at least one > + // redistributor region (GICR) containing dedicated redistributor > + // interfaces for all individual CPUs, and the CPU interface (GICC). > + // Under virtualization, we assume that the first redistributor region > + // listed covers the boot CPU. Also, our GICv3 driver only supports the > + // system register CPU interface, so we can safely ignore the MMIO version > + // which is listed after the sequence of redistributor interfaces. > + // This means we are only interested in the first two memory regions > + // supplied, and ignore everything else. > + // > + ASSERT (RegSize >= 32); > + > + // RegProp[0..1] == { GICD base, GICD size } > + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); Sigh. So this is the answer to (21). Here's another proposal then: (27) can we perhaps dispense with the byte order conversion helpers from libfdt? I mean, it is annoying to include the FDT library *just* for this, when we have our shiny new protocol ready, for the rest of the device tree massaging. So the idea is: - If (knowing the rest of the series) you deem that libfdt is frequently needed in addition to the new protocol *because* we frequently manipulate the DTB beyond the capabilities of the new protocol (and in a way that is hard to extract into the protocol), then sure, libfdt and the protocol should co-exist in client code, and I have nothing against fdt64_to_cpu() - OTOH, if we mostly (only?) include LibFdt on top of the new protocol, in the rest of the series too, because we can't live without fdt64_to_cpu() and friends, then I suggest to remove the LibFdt dependency, and exploit that FDT internals are always big-endian, while UEFI is always little-endian. Therefore, use the SwapBytes64() function (and friends) directly, from BaseLib. What do you think? > + ASSERT (DistBase < MAX_UINT32); > + > + // RegProp[2..3] == { GICR base, GICR size } > + RedistBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); > + ASSERT (RedistBase < MAX_UINT32); > + > + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); > + PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase); > + > + DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n", > + DistBase, RedistBase)); > + > + // > // The default implementation of ArmGicArchLib is responsible for enabling > // the system register interface on the GICv3 if one is found. So let's do > // the same here. > @@ -55,6 +117,18 @@ ArmVirtGicArchLibConstructor ( > break; > > case 2: > + ASSERT (RegSize == 32); > + > + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); > + CpuBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); > + ASSERT (DistBase < MAX_UINT32); > + ASSERT (CpuBase < MAX_UINT32); > + > + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); > + PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase); > + > + DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase)); > + > mGicArchRevision = ARM_GIC_ARCH_REVISION_2; > break; > This looks good to me (with the above remarks in mind). Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/07/16 13:18, Laszlo Ersek wrote: > On 04/06/16 18:15, Ard Biesheuvel wrote: >> + >> + GicRevision = 2; >> + Status = FdtClient->FindCompatibleNodeReg (FdtClient, >> + "arm,cortex-a15-gic", >> + &Reg, >> + &RegElemSize, >> + &RegSize); > > (26) The indentation of the arguments is not correct; they should be > aligned with "FindCompatibleNodeReg", plus one or two spaces. > > Can you re-check the series for such indentation problems? FWIW, I personally subscribe to two indentation styles in edk2: the official one, and my own "compressed" variant of it. They are: Status = FdtClient->FindCompatibleNodeReg ( FdtClient, "arm,cortex-a15-gic", &Reg, &RegElemSize, &RegSize ); That is, no argument on the first line, indentation as described above, closing paren on separate line. Or (mine): Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,cortex-a15-gic", &Reg, &RegElemSize, &RegSize); Which is same as yours, except the arguments are not aligned with the first argument, but two columns to the right of the member function's name. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 7 April 2016 at 13:18, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/06/16 18:15, Ard Biesheuvel wrote: >> Instead of relying on VirtFdtDxe to populate the GIC related PCDs, move >> this handling to our implementation of ArmGicArchLib, and retrieve the >> required DT info using the new FDT client protocol. >> >> This removes one of the reasons we need to load VirtFdtDxe first using >> an 'A PRIORI' declaration in the platform FDF. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 84 ++++++++++++++++++-- >> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 15 +++- >> 2 files changed, 92 insertions(+), 7 deletions(-) > > Urgh. > > I guess the conversion (together with the next patch) should be > "straightforward". However, before I try to validate the code movement, > I have a small and a big question. > > (21) small one: what do you need <libfdt.h>, FdtLib, and (consequently) > EmbeddedPkg/EmbeddedPkg.dec for? > > ... Nevermind, coming back from the source code below, I understand. > I'll make a suggestion down there. Consider (21) a no-op. > > (22) The big question is this: how can I determine where this library > instance is used? > > Namely, before the conversion, PcdGic*Base would be set only once (in > VirtFdtDxe). That means two things: (a) it is set *for sure*, for any > DXE driver that might want to look at the PCDs, plus (b) the PCDs are > not set more than once. > > I can't see how this conversion guarantees either. > > Let's start with question (b). (Breaking (b) is not a catastrophe, but > it's ugly.) > > I tried to round up users of the lib class ArmGicArchLib. I found two > client INF files: > > - ArmPkg/Drivers/ArmGic/ArmGicLib.inf > - ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf > > These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So, > I guess I have to find the clients of the lib class ArmGicLib. They are: > Please don't ask :-) These predate my involvement with Tianocore, and my suspicion is that many of these choices are simply based on whatever happened to produce a working image. > - ArmPkg/Application/LinuxLoader/LinuxLoader.inf > > We use this in our ARM (32-bit) builds. It means whenever this app is > run, it will re-set the PCDs. :/ ... Although, it looks like the > application only depends on ArmGicLib in the AARCH64 case, precisely > which architecture we don't build the application for. Okay, > ultimately, but confusing... > Indeed. We would *love* the kill the built in linux loader (bill), and we don't include it in any AArch64 builds by default. However, like the ARM BDS, it is used in the wild, and we can't simply remove it just yet. In fact, now that my 32-bit ARM stub support is upstream, we should probably drop it from ArmVirtQemu altogether (since QEMU already has a built in linux loader if you run it without UEFI) > - ArmPkg/Drivers/ArmGic/ArmGicDxe.inf > > I guess this is our main target. This one is therefore certainly > correct to set the PCDs. > Ack > - ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf > > We never include this driver. Okay. > > - ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf > - ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf > - ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf > > What the heck, all of these library instances are for class > ArmGicArchLib, which is what I started out with! We actually use the > last one (this patch modifies exactly it); it creates a circular > library dependency between ArmGicLib and ArmGicArchLib. Sweet $DEITY. > Well, at least they don't all have constructors, right? > Anyway, since I started out tracking down ArmGicArchLib, and we don't > use the first two of the above, they don't add new things to follow. > Ack > - ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf > > We have a library class resolution to this, in "ArmVirt.dsc.inc", for > ArmPlatformSecExtraActionLib. That's a dead-end, thankfully; grepping > for the lib class, only > "ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/ArmVExpressSecLib.inf" > depends on it, which I can't imagine we use. The resolution should be > dropped from "ArmVirtPkg/ArmVirt.dsc.inc", probably. > Indeed, I will propose a separate patch for that. > - ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf > - ArmPlatformPkg/PrePi/PeiMPCore.inf > > These are SEC modules, and we don't use them. Okay. > > So, ultimately, the only user of this library instance is > "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". ... Indeed, checking the build > report file for ArmVirtQemu (AARCH64), I find ArmVirtGicArchLib (and > ArmGicLib too) only under "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". > > Question (22)(b) is therefore cleared; the PCDs in question will only be > set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched. > Ack, Thanks a lot for tracking that down > > Let's see (22)(a): can anything consume these PCDs before > "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" sets them? > > - ArmPkg/Application/LinuxLoader/LinuxLoader.inf > > Same as above: uses the PCDs only on AARCH64, when we don't build it. > > - ArmPkg/Drivers/ArmGic/ArmGicDxe.inf > > The library sets the PCDs for it in time. > > - ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf > > We don't use this (it wants the PCDs as Fixed, anyway). > > - ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf > - ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf > - ArmPlatformPkg/PrePi/PeiMPCore.inf > > Ditto for these three. > > Alright! It was exhausting to verify it (you could have written it up > too... :)), but the conversion seems safe. Ultimately, as (22), I'll > make this request: > > (22) please document in the commit message, that in all our builds, only > "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" links against this library, and > only "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" consumes the PCDs. > OK > ... This is actually a "standard requirement" for the rest of the > plugin-like patches, more or less. Do you think (knowing the rest of the > series) that it might make sense for you to post a v2 after my comments > thus far? Mainly, this "usage analysis" is exhausting, and for the rest > of the series, I'd prefer to validate yours, not construct mine. :) > Yes, I think that makes sense. > Anyway, let's see the rest of this patch (INF file moved to the top -- > can you perhaps adopt a diff-order file, for formatting the patches?): > >> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >> index c85b2d44d856..57086242de1f 100644 >> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >> @@ -18,7 +18,7 @@ [Defines] >> INF_VERSION = 0x00010005 >> BASE_NAME = ArmVirtGicArchLib >> FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed >> - MODULE_TYPE = BASE >> + MODULE_TYPE = DXE_DRIVER > > (23) I don't think it's necessary. You use neither ImageHandle nor > SystemTable below. The client type restriction (on the LIBRARY_CLASS > line below) suffices. > I wondered about that. If the only difference is the prototype of the constructor, then I should probable just drop this hunk (and the one that changes the constructor signature) >> VERSION_STRING = 1.0 >> LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION >> CONSTRUCTOR = ArmVirtGicArchLibConstructor >> @@ -30,11 +30,22 @@ [LibraryClasses] >> PcdLib >> DebugLib >> ArmGicLib >> + UefiBootServicesTableLib >> + FdtLib >> >> [Packages] >> MdePkg/MdePkg.dec >> ArmPkg/ArmPkg.dec >> ArmVirtPkg/ArmVirtPkg.dec >> + EmbeddedPkg/EmbeddedPkg.dec >> + >> +[Protocols] >> + gFdtClientProtocolGuid >> >> [Pcd] >> - gArmVirtTokenSpaceGuid.PcdArmGicRevision >> + gArmTokenSpaceGuid.PcdGicDistributorBase >> + gArmTokenSpaceGuid.PcdGicRedistributorsBase >> + gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase >> + >> +[Depex] >> + gFdtClientProtocolGuid >> > > Looks okay (modulo (21) above). > >> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> index 732860cadfe6..686622228831 100644 >> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> @@ -1,7 +1,7 @@ >> /** @file >> ArmGicArchLib library class implementation for DT based virt platforms >> >> - Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> >> + Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR> >> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> @@ -19,21 +19,83 @@ >> #include <Library/ArmGicArchLib.h> >> #include <Library/PcdLib.h> >> #include <Library/DebugLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> +#include <libfdt.h> >> + >> +#include <Protocol/FdtClient.h> >> >> STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; >> >> -RETURN_STATUS >> +EFI_STATUS >> EFIAPI >> ArmVirtGicArchLibConstructor ( >> - VOID >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> - UINT32 IccSre; >> + UINT32 IccSre; >> + FDT_CLIENT_PROTOCOL *FdtClient; >> + CONST VOID *Reg; >> + UINTN RegElemSize, RegSize; >> + UINTN GicRevision; >> + EFI_STATUS Status; >> + UINT64 DistBase, CpuBase, RedistBase; >> >> - switch (PcdGet32 (PcdArmGicRevision)) { >> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID **)&FdtClient); > > (24) This line is too long. > > Can you double-check the series that added lines don't exceed 79 > characters? (I think "BaseTools/Scripts/PatchCheck.py" might help you > verify this -- see a7e173b07a1e.) I guess I shouldn't obsess about code > that is moved, but code you write genuinely for this work shouldn't be > too wide. > Well, I think this is a good opportunity to get rid of these things, so please don't hesitate pointing them out. >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } > > (25) Given the DepEx, you might want to replace this with an > ASSERT_EFI_ERROR(). Up to you. > Yes. It think I may have done that in another patch in the series. >> + >> + GicRevision = 2; >> + Status = FdtClient->FindCompatibleNodeReg (FdtClient, >> + "arm,cortex-a15-gic", >> + &Reg, >> + &RegElemSize, >> + &RegSize); > > (26) The indentation of the arguments is not correct; they should be > aligned with "FindCompatibleNodeReg", plus one or two spaces. > > Can you re-check the series for such indentation problems? > Yep >> + if (Status == EFI_NOT_FOUND) { >> + GicRevision = 3; >> + Status = FdtClient->FindCompatibleNodeReg (FdtClient, >> + "arm,gic-v3", >> + &Reg, >> + &RegElemSize, >> + &RegSize); >> + } >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + switch (GicRevision) { >> >> case 3: >> // >> + // The GIC v3 DT binding describes a series of at least 3 physical (base >> + // addresses, size) pairs: the distributor interface (GICD), at least one >> + // redistributor region (GICR) containing dedicated redistributor >> + // interfaces for all individual CPUs, and the CPU interface (GICC). >> + // Under virtualization, we assume that the first redistributor region >> + // listed covers the boot CPU. Also, our GICv3 driver only supports the >> + // system register CPU interface, so we can safely ignore the MMIO version >> + // which is listed after the sequence of redistributor interfaces. >> + // This means we are only interested in the first two memory regions >> + // supplied, and ignore everything else. >> + // >> + ASSERT (RegSize >= 32); >> + >> + // RegProp[0..1] == { GICD base, GICD size } >> + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); > > Sigh. So this is the answer to (21). Here's another proposal then: > > (27) can we perhaps dispense with the byte order conversion helpers from > libfdt? I mean, it is annoying to include the FDT library *just* for > this, when we have our shiny new protocol ready, for the rest of the > device tree massaging. So the idea is: > > - If (knowing the rest of the series) you deem that libfdt is frequently > needed in addition to the new protocol *because* we frequently > manipulate the DTB beyond the capabilities of the new protocol (and in > a way that is hard to extract into the protocol), then sure, libfdt > and the protocol should co-exist in client code, and I have nothing > against fdt64_to_cpu() > > - OTOH, if we mostly (only?) include LibFdt on top of the new protocol, > in the rest of the series too, because we can't live without > fdt64_to_cpu() and friends, then I suggest to remove the LibFdt > dependency, and exploit that FDT internals are always big-endian, > while UEFI is always little-endian. Therefore, use the SwapBytes64() > function (and friends) directly, from BaseLib. > > What do you think? > I think SwapBytes () should be preferred, and I think that this is the only reason the dependency remains. The reason is that the new drivers don't have access to the DeviceTreeBaseAddress PCD, so many of the non-trivial libfdt functions are not even callable by them >> + ASSERT (DistBase < MAX_UINT32); >> + >> + // RegProp[2..3] == { GICR base, GICR size } >> + RedistBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); >> + ASSERT (RedistBase < MAX_UINT32); >> + >> + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); >> + PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase); >> + >> + DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n", >> + DistBase, RedistBase)); >> + >> + // >> // The default implementation of ArmGicArchLib is responsible for enabling >> // the system register interface on the GICv3 if one is found. So let's do >> // the same here. >> @@ -55,6 +117,18 @@ ArmVirtGicArchLibConstructor ( >> break; >> >> case 2: >> + ASSERT (RegSize == 32); >> + >> + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); >> + CpuBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); >> + ASSERT (DistBase < MAX_UINT32); >> + ASSERT (CpuBase < MAX_UINT32); >> + >> + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); >> + PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase); >> + >> + DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase)); >> + >> mGicArchRevision = ARM_GIC_ARCH_REVISION_2; >> break; >> > > This looks good to me (with the above remarks in mind). > Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/07/16 13:44, Ard Biesheuvel wrote: > On 7 April 2016 at 13:18, Laszlo Ersek <lersek@redhat.com> wrote: >> I tried to round up users of the lib class ArmGicArchLib. I found two >> client INF files: >> >> - ArmPkg/Drivers/ArmGic/ArmGicLib.inf >> - ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf >> >> These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So, >> I guess I have to find the clients of the lib class ArmGicLib. They are: >> > > Please don't ask :-) These predate my involvement with Tianocore, and > my suspicion is that many of these choices are simply based on > whatever happened to produce a working image. Right. Anyway, as we've repeatedly determined from both practice and the INF file spec, the MODULE_TYPE for a library instance doesn't seem to affect anything beyond the constructor's signature (if any). So, let's leave these the way they are. >> - ArmPkg/Application/LinuxLoader/LinuxLoader.inf >> >> We use this in our ARM (32-bit) builds. It means whenever this app is >> run, it will re-set the PCDs. :/ ... Although, it looks like the >> application only depends on ArmGicLib in the AARCH64 case, precisely >> which architecture we don't build the application for. Okay, >> ultimately, but confusing... >> > > Indeed. We would *love* the kill the built in linux loader (bill), and > we don't include it in any AArch64 builds by default. However, like > the ARM BDS, it is used in the wild, and we can't simply remove it > just yet. In fact, now that my 32-bit ARM stub support is upstream, we > should probably drop it from ArmVirtQemu altogether (since QEMU > already has a built in linux loader if you run it without UEFI) I certainly support the full removal of the linux loader app from ArmVirtQemu; if it eases patch review for me, then it's already worth it (for me). Feel free to post a separate patch for it, I guess. >> Question (22)(b) is therefore cleared; the PCDs in question will only be >> set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched. >> > > Ack, Thanks a lot for tracking that down I mentioned the build report file, but I guess it bears some stressing -- it is super helpful. I don't like to rely on the build report file exclusively, but it is very good for verifying a hypothesis. Even the PCD usage (see (22)(a)) can be confirmed with it! [snip] >>> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>> index c85b2d44d856..57086242de1f 100644 >>> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>> @@ -18,7 +18,7 @@ [Defines] >>> INF_VERSION = 0x00010005 >>> BASE_NAME = ArmVirtGicArchLib >>> FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed >>> - MODULE_TYPE = BASE >>> + MODULE_TYPE = DXE_DRIVER >> >> (23) I don't think it's necessary. You use neither ImageHandle nor >> SystemTable below. The client type restriction (on the LIBRARY_CLASS >> line below) suffices. >> > > I wondered about that. If the only difference is the prototype of the > constructor, It is, to my knowledge. > then I should probable just drop this hunk (and the one > that changes the constructor signature) I agree. [snip] >> (27) can we perhaps dispense with the byte order conversion helpers from >> libfdt? I mean, it is annoying to include the FDT library *just* for >> this, when we have our shiny new protocol ready, for the rest of the >> device tree massaging. So the idea is: >> >> - If (knowing the rest of the series) you deem that libfdt is frequently >> needed in addition to the new protocol *because* we frequently >> manipulate the DTB beyond the capabilities of the new protocol (and in >> a way that is hard to extract into the protocol), then sure, libfdt >> and the protocol should co-exist in client code, and I have nothing >> against fdt64_to_cpu() >> >> - OTOH, if we mostly (only?) include LibFdt on top of the new protocol, >> in the rest of the series too, because we can't live without >> fdt64_to_cpu() and friends, then I suggest to remove the LibFdt >> dependency, and exploit that FDT internals are always big-endian, >> while UEFI is always little-endian. Therefore, use the SwapBytes64() >> function (and friends) directly, from BaseLib. >> >> What do you think? >> > > I think SwapBytes () should be preferred, and I think that this is the > only reason the dependency remains. The reason is that the new drivers > don't have access to the DeviceTreeBaseAddress PCD, so many of the > non-trivial libfdt functions are not even callable by them Heh, very good point! So, I guess I'll await your v3, re-check patches 01-07 (wherever I had comments), and resume with v3 08/21. Is that alright? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 7 April 2016 at 14:02, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/07/16 13:44, Ard Biesheuvel wrote: >> On 7 April 2016 at 13:18, Laszlo Ersek <lersek@redhat.com> wrote: > >>> I tried to round up users of the lib class ArmGicArchLib. I found two >>> client INF files: >>> >>> - ArmPkg/Drivers/ArmGic/ArmGicLib.inf >>> - ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf >>> >>> These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So, >>> I guess I have to find the clients of the lib class ArmGicLib. They are: >>> >> >> Please don't ask :-) These predate my involvement with Tianocore, and >> my suspicion is that many of these choices are simply based on >> whatever happened to produce a working image. > > Right. > > Anyway, as we've repeatedly determined from both practice and the INF > file spec, the MODULE_TYPE for a library instance doesn't seem to affect > anything beyond the constructor's signature (if any). So, let's leave > these the way they are. > >>> - ArmPkg/Application/LinuxLoader/LinuxLoader.inf >>> >>> We use this in our ARM (32-bit) builds. It means whenever this app is >>> run, it will re-set the PCDs. :/ ... Although, it looks like the >>> application only depends on ArmGicLib in the AARCH64 case, precisely >>> which architecture we don't build the application for. Okay, >>> ultimately, but confusing... >>> >> >> Indeed. We would *love* the kill the built in linux loader (bill), and >> we don't include it in any AArch64 builds by default. However, like >> the ARM BDS, it is used in the wild, and we can't simply remove it >> just yet. In fact, now that my 32-bit ARM stub support is upstream, we >> should probably drop it from ArmVirtQemu altogether (since QEMU >> already has a built in linux loader if you run it without UEFI) > > I certainly support the full removal of the linux loader app from > ArmVirtQemu; if it eases patch review for me, then it's already worth it > (for me). Feel free to post a separate patch for it, I guess. > >>> Question (22)(b) is therefore cleared; the PCDs in question will only be >>> set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched. >>> >> >> Ack, Thanks a lot for tracking that down > > I mentioned the build report file, but I guess it bears some stressing > -- it is super helpful. I don't like to rely on the build report file > exclusively, but it is very good for verifying a hypothesis. > > Even the PCD usage (see (22)(a)) can be confirmed with it! > > [snip] > >>>> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>> index c85b2d44d856..57086242de1f 100644 >>>> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>> @@ -18,7 +18,7 @@ [Defines] >>>> INF_VERSION = 0x00010005 >>>> BASE_NAME = ArmVirtGicArchLib >>>> FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed >>>> - MODULE_TYPE = BASE >>>> + MODULE_TYPE = DXE_DRIVER >>> >>> (23) I don't think it's necessary. You use neither ImageHandle nor >>> SystemTable below. The client type restriction (on the LIBRARY_CLASS >>> line below) suffices. >>> >> >> I wondered about that. If the only difference is the prototype of the >> constructor, > > It is, to my knowledge. > >> then I should probable just drop this hunk (and the one >> that changes the constructor signature) > > I agree. > > [snip] > >>> (27) can we perhaps dispense with the byte order conversion helpers from >>> libfdt? I mean, it is annoying to include the FDT library *just* for >>> this, when we have our shiny new protocol ready, for the rest of the >>> device tree massaging. So the idea is: >>> >>> - If (knowing the rest of the series) you deem that libfdt is frequently >>> needed in addition to the new protocol *because* we frequently >>> manipulate the DTB beyond the capabilities of the new protocol (and in >>> a way that is hard to extract into the protocol), then sure, libfdt >>> and the protocol should co-exist in client code, and I have nothing >>> against fdt64_to_cpu() >>> >>> - OTOH, if we mostly (only?) include LibFdt on top of the new protocol, >>> in the rest of the series too, because we can't live without >>> fdt64_to_cpu() and friends, then I suggest to remove the LibFdt >>> dependency, and exploit that FDT internals are always big-endian, >>> while UEFI is always little-endian. Therefore, use the SwapBytes64() >>> function (and friends) directly, from BaseLib. >>> >>> What do you think? >>> >> >> I think SwapBytes () should be preferred, and I think that this is the >> only reason the dependency remains. The reason is that the new drivers >> don't have access to the DeviceTreeBaseAddress PCD, so many of the >> non-trivial libfdt functions are not even callable by them > > Heh, very good point! > > So, I guess I'll await your v3, re-check patches 01-07 (wherever I had > comments), and resume with v3 08/21. Is that alright? > Does that mean you are going to ignore my v2? _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/07/16 14:04, Ard Biesheuvel wrote: > On 7 April 2016 at 14:02, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/07/16 13:44, Ard Biesheuvel wrote: >>> On 7 April 2016 at 13:18, Laszlo Ersek <lersek@redhat.com> wrote: >> >>>> I tried to round up users of the lib class ArmGicArchLib. I found two >>>> client INF files: >>>> >>>> - ArmPkg/Drivers/ArmGic/ArmGicLib.inf >>>> - ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf >>>> >>>> These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So, >>>> I guess I have to find the clients of the lib class ArmGicLib. They are: >>>> >>> >>> Please don't ask :-) These predate my involvement with Tianocore, and >>> my suspicion is that many of these choices are simply based on >>> whatever happened to produce a working image. >> >> Right. >> >> Anyway, as we've repeatedly determined from both practice and the INF >> file spec, the MODULE_TYPE for a library instance doesn't seem to affect >> anything beyond the constructor's signature (if any). So, let's leave >> these the way they are. >> >>>> - ArmPkg/Application/LinuxLoader/LinuxLoader.inf >>>> >>>> We use this in our ARM (32-bit) builds. It means whenever this app is >>>> run, it will re-set the PCDs. :/ ... Although, it looks like the >>>> application only depends on ArmGicLib in the AARCH64 case, precisely >>>> which architecture we don't build the application for. Okay, >>>> ultimately, but confusing... >>>> >>> >>> Indeed. We would *love* the kill the built in linux loader (bill), and >>> we don't include it in any AArch64 builds by default. However, like >>> the ARM BDS, it is used in the wild, and we can't simply remove it >>> just yet. In fact, now that my 32-bit ARM stub support is upstream, we >>> should probably drop it from ArmVirtQemu altogether (since QEMU >>> already has a built in linux loader if you run it without UEFI) >> >> I certainly support the full removal of the linux loader app from >> ArmVirtQemu; if it eases patch review for me, then it's already worth it >> (for me). Feel free to post a separate patch for it, I guess. >> >>>> Question (22)(b) is therefore cleared; the PCDs in question will only be >>>> set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched. >>>> >>> >>> Ack, Thanks a lot for tracking that down >> >> I mentioned the build report file, but I guess it bears some stressing >> -- it is super helpful. I don't like to rely on the build report file >> exclusively, but it is very good for verifying a hypothesis. >> >> Even the PCD usage (see (22)(a)) can be confirmed with it! >> >> [snip] >> >>>>> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>>> index c85b2d44d856..57086242de1f 100644 >>>>> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>>> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >>>>> @@ -18,7 +18,7 @@ [Defines] >>>>> INF_VERSION = 0x00010005 >>>>> BASE_NAME = ArmVirtGicArchLib >>>>> FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed >>>>> - MODULE_TYPE = BASE >>>>> + MODULE_TYPE = DXE_DRIVER >>>> >>>> (23) I don't think it's necessary. You use neither ImageHandle nor >>>> SystemTable below. The client type restriction (on the LIBRARY_CLASS >>>> line below) suffices. >>>> >>> >>> I wondered about that. If the only difference is the prototype of the >>> constructor, >> >> It is, to my knowledge. >> >>> then I should probable just drop this hunk (and the one >>> that changes the constructor signature) >> >> I agree. >> >> [snip] >> >>>> (27) can we perhaps dispense with the byte order conversion helpers from >>>> libfdt? I mean, it is annoying to include the FDT library *just* for >>>> this, when we have our shiny new protocol ready, for the rest of the >>>> device tree massaging. So the idea is: >>>> >>>> - If (knowing the rest of the series) you deem that libfdt is frequently >>>> needed in addition to the new protocol *because* we frequently >>>> manipulate the DTB beyond the capabilities of the new protocol (and in >>>> a way that is hard to extract into the protocol), then sure, libfdt >>>> and the protocol should co-exist in client code, and I have nothing >>>> against fdt64_to_cpu() >>>> >>>> - OTOH, if we mostly (only?) include LibFdt on top of the new protocol, >>>> in the rest of the series too, because we can't live without >>>> fdt64_to_cpu() and friends, then I suggest to remove the LibFdt >>>> dependency, and exploit that FDT internals are always big-endian, >>>> while UEFI is always little-endian. Therefore, use the SwapBytes64() >>>> function (and friends) directly, from BaseLib. >>>> >>>> What do you think? >>>> >>> >>> I think SwapBytes () should be preferred, and I think that this is the >>> only reason the dependency remains. The reason is that the new drivers >>> don't have access to the DeviceTreeBaseAddress PCD, so many of the >>> non-trivial libfdt functions are not even callable by them >> >> Heh, very good point! >> >> So, I guess I'll await your v3, re-check patches 01-07 (wherever I had >> comments), and resume with v3 08/21. Is that alright? >> > > Does that mean you are going to ignore my v2? I certainly don't *want* to ignore it, but I think we have identified several aspects that may affect many of the remaining patches: (a) UINT32 vs. UINTN output parameters in the protocol interface (b) LibFdt usage / SwapBytes() (c) (important:) for the commit messages, tracking down the clients of the new plugin library instances that set PCDs (to avoid multi-setting), recursively; plus tracking down all the consumers of those PCDs (to ensure they will all see the updated PCD values). (d) line lengths (e) argument indentation in function calls Issues (a), (b), (d) and (e) will continue tripping me up in the review, and you can fix them up without me actually identifying each individual location. Issue (c) requires me to spend multiple tens of minutes per affected patch. I would prefer if you spent those minutes, wrote up your findings in the commit messages, so I'd only need verify your findings, not find them myself from scratch. In other words, the above issues should be fixed not just for the sake of the final code and messages that get into the repository, but also in order to help me review the rest of the series. Normally I do prefer to review all patches in a series, before asking for the next version, but I believe the above problems might affect the rest of the patches. My main worry is that if I keep getting tripped up by them, I will get de-sensitivised for version 3, and maybe miss something else that would be important. Consider, if I review the rest of v2, and point out twenty more instances of (d) and (e), then when I review v3, I will have to verify whether you fixed up every single one of those instances. This wastes a lot of gray matter. :) This is the kind of issue that you can locate yourself in the rest of the series (now that I've pointed out a few instances), so they wouldn't even exist to begin with, when I looked at the rest of the patches, in v3, for the first time. Of course, I could be wrong too. If you assure me that issues (a) through (e) will probably not impede my review in the remaining v2 patches :), then I'm more than happy to continue reviewing v2! To repeat: saving time for me (at the cost of your time) is just the small goal. The big goal is to preserve my sensitivity to your patches. Thanks. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 7 April 2016 at 14:26, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/07/16 14:04, Ard Biesheuvel wrote: >> On 7 April 2016 at 14:02, Laszlo Ersek <lersek@redhat.com> wrote: >> Does that mean you are going to ignore my v2? > > I certainly don't *want* to ignore it, but I think we have identified > several aspects that may affect many of the remaining patches: > > (a) UINT32 vs. UINTN output parameters in the protocol interface > > (b) LibFdt usage / SwapBytes() > > (c) (important:) for the commit messages, tracking down the clients of > the new plugin library instances that set PCDs (to avoid > multi-setting), recursively; plus tracking down all the consumers > of those PCDs (to ensure they will all see the updated PCD values). > > (d) line lengths > > (e) argument indentation in function calls > > Issues (a), (b), (d) and (e) will continue tripping me up in the review, > and you can fix them up without me actually identifying each individual > location. > > Issue (c) requires me to spend multiple tens of minutes per affected > patch. I would prefer if you spent those minutes, wrote up your findings > in the commit messages, so I'd only need verify your findings, not find > them myself from scratch. > > In other words, the above issues should be fixed not just for the sake > of the final code and messages that get into the repository, but also in > order to help me review the rest of the series. > > Normally I do prefer to review all patches in a series, before asking > for the next version, but I believe the above problems might affect the > rest of the patches. My main worry is that if I keep getting tripped up > by them, I will get de-sensitivised for version 3, and maybe miss > something else that would be important. > > Consider, if I review the rest of v2, and point out twenty more > instances of (d) and (e), then when I review v3, I will have to verify > whether you fixed up every single one of those instances. This wastes a > lot of gray matter. :) This is the kind of issue that you can locate > yourself in the rest of the series (now that I've pointed out a few > instances), so they wouldn't even exist to begin with, when I looked at > the rest of the patches, in v3, for the first time. > > Of course, I could be wrong too. If you assure me that issues (a) > through (e) will probably not impede my review in the remaining v2 > patches :), then I'm more than happy to continue reviewing v2! > > To repeat: saving time for me (at the cost of your time) is just the > small goal. The big goal is to preserve my sensitivity to your patches. > Dude, I was kidding. I haven't even sent out my v2 yet, the patches you have been looking at were v1. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/07/16 14:28, Ard Biesheuvel wrote: > On 7 April 2016 at 14:26, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/07/16 14:04, Ard Biesheuvel wrote: >>> On 7 April 2016 at 14:02, Laszlo Ersek <lersek@redhat.com> wrote: >>> Does that mean you are going to ignore my v2? >> >> I certainly don't *want* to ignore it, but I think we have identified >> several aspects that may affect many of the remaining patches: >> >> (a) UINT32 vs. UINTN output parameters in the protocol interface >> >> (b) LibFdt usage / SwapBytes() >> >> (c) (important:) for the commit messages, tracking down the clients of >> the new plugin library instances that set PCDs (to avoid >> multi-setting), recursively; plus tracking down all the consumers >> of those PCDs (to ensure they will all see the updated PCD values). >> >> (d) line lengths >> >> (e) argument indentation in function calls >> >> Issues (a), (b), (d) and (e) will continue tripping me up in the review, >> and you can fix them up without me actually identifying each individual >> location. >> >> Issue (c) requires me to spend multiple tens of minutes per affected >> patch. I would prefer if you spent those minutes, wrote up your findings >> in the commit messages, so I'd only need verify your findings, not find >> them myself from scratch. >> >> In other words, the above issues should be fixed not just for the sake >> of the final code and messages that get into the repository, but also in >> order to help me review the rest of the series. >> >> Normally I do prefer to review all patches in a series, before asking >> for the next version, but I believe the above problems might affect the >> rest of the patches. My main worry is that if I keep getting tripped up >> by them, I will get de-sensitivised for version 3, and maybe miss >> something else that would be important. >> >> Consider, if I review the rest of v2, and point out twenty more >> instances of (d) and (e), then when I review v3, I will have to verify >> whether you fixed up every single one of those instances. This wastes a >> lot of gray matter. :) This is the kind of issue that you can locate >> yourself in the rest of the series (now that I've pointed out a few >> instances), so they wouldn't even exist to begin with, when I looked at >> the rest of the patches, in v3, for the first time. >> >> Of course, I could be wrong too. If you assure me that issues (a) >> through (e) will probably not impede my review in the remaining v2 >> patches :), then I'm more than happy to continue reviewing v2! >> >> To repeat: saving time for me (at the cost of your time) is just the >> small goal. The big goal is to preserve my sensitivity to your patches. >> > > Dude, I was kidding. I haven't even sent out my v2 yet, the patches > you have been looking at were v1. Sigh. Did I mention de-sensitivization?... ;) I guess I'm lolling at myself. :) Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c index 732860cadfe6..686622228831 100644 --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c @@ -1,7 +1,7 @@ /** @file ArmGicArchLib library class implementation for DT based virt platforms - Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> + Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License @@ -19,21 +19,83 @@ #include <Library/ArmGicArchLib.h> #include <Library/PcdLib.h> #include <Library/DebugLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <libfdt.h> + +#include <Protocol/FdtClient.h> STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; -RETURN_STATUS +EFI_STATUS EFIAPI ArmVirtGicArchLibConstructor ( - VOID + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable ) { - UINT32 IccSre; + UINT32 IccSre; + FDT_CLIENT_PROTOCOL *FdtClient; + CONST VOID *Reg; + UINTN RegElemSize, RegSize; + UINTN GicRevision; + EFI_STATUS Status; + UINT64 DistBase, CpuBase, RedistBase; - switch (PcdGet32 (PcdArmGicRevision)) { + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID **)&FdtClient); + if (EFI_ERROR (Status)) { + return Status; + } + + GicRevision = 2; + Status = FdtClient->FindCompatibleNodeReg (FdtClient, + "arm,cortex-a15-gic", + &Reg, + &RegElemSize, + &RegSize); + if (Status == EFI_NOT_FOUND) { + GicRevision = 3; + Status = FdtClient->FindCompatibleNodeReg (FdtClient, + "arm,gic-v3", + &Reg, + &RegElemSize, + &RegSize); + } + if (EFI_ERROR (Status)) { + return Status; + } + + switch (GicRevision) { case 3: // + // The GIC v3 DT binding describes a series of at least 3 physical (base + // addresses, size) pairs: the distributor interface (GICD), at least one + // redistributor region (GICR) containing dedicated redistributor + // interfaces for all individual CPUs, and the CPU interface (GICC). + // Under virtualization, we assume that the first redistributor region + // listed covers the boot CPU. Also, our GICv3 driver only supports the + // system register CPU interface, so we can safely ignore the MMIO version + // which is listed after the sequence of redistributor interfaces. + // This means we are only interested in the first two memory regions + // supplied, and ignore everything else. + // + ASSERT (RegSize >= 32); + + // RegProp[0..1] == { GICD base, GICD size } + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); + ASSERT (DistBase < MAX_UINT32); + + // RegProp[2..3] == { GICR base, GICR size } + RedistBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); + ASSERT (RedistBase < MAX_UINT32); + + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); + PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase); + + DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n", + DistBase, RedistBase)); + + // // The default implementation of ArmGicArchLib is responsible for enabling // the system register interface on the GICv3 if one is found. So let's do // the same here. @@ -55,6 +117,18 @@ ArmVirtGicArchLibConstructor ( break; case 2: + ASSERT (RegSize == 32); + + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]); + CpuBase = fdt64_to_cpu (((UINT64 *)Reg)[2]); + ASSERT (DistBase < MAX_UINT32); + ASSERT (CpuBase < MAX_UINT32); + + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase); + PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase); + + DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase)); + mGicArchRevision = ARM_GIC_ARCH_REVISION_2; break; diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf index c85b2d44d856..57086242de1f 100644 --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf @@ -18,7 +18,7 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = ArmVirtGicArchLib FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed - MODULE_TYPE = BASE + MODULE_TYPE = DXE_DRIVER VERSION_STRING = 1.0 LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION CONSTRUCTOR = ArmVirtGicArchLibConstructor @@ -30,11 +30,22 @@ [LibraryClasses] PcdLib DebugLib ArmGicLib + UefiBootServicesTableLib + FdtLib [Packages] MdePkg/MdePkg.dec ArmPkg/ArmPkg.dec ArmVirtPkg/ArmVirtPkg.dec + EmbeddedPkg/EmbeddedPkg.dec + +[Protocols] + gFdtClientProtocolGuid [Pcd] - gArmVirtTokenSpaceGuid.PcdArmGicRevision + gArmTokenSpaceGuid.PcdGicDistributorBase + gArmTokenSpaceGuid.PcdGicRedistributorsBase + gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase + +[Depex] + gFdtClientProtocolGuid
Instead of relying on VirtFdtDxe to populate the GIC related PCDs, move this handling to our implementation of ArmGicArchLib, and retrieve the required DT info using the new FDT client protocol. This removes one of the reasons we need to load VirtFdtDxe first using an 'A PRIORI' declaration in the platform FDF. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 84 ++++++++++++++++++-- ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 15 +++- 2 files changed, 92 insertions(+), 7 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel