[edk2,06/21] ArmVirtPkg/ArmGicArchLib: move to FdtClient protocol

Message ID 1459959319-19293-7-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 6, 2016, 4:15 p.m.
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

Comments

Laszlo Ersek April 7, 2016, 11:18 a.m. | #1
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
Laszlo Ersek April 7, 2016, 11:25 a.m. | #2
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
Ard Biesheuvel April 7, 2016, 11:44 a.m. | #3
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
Laszlo Ersek April 7, 2016, 12:02 p.m. | #4
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
Ard Biesheuvel April 7, 2016, 12:04 p.m. | #5
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
Laszlo Ersek April 7, 2016, 12:26 p.m. | #6
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
Ard Biesheuvel April 7, 2016, 12:28 p.m. | #7
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
Laszlo Ersek April 7, 2016, 12:35 p.m. | #8
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

Patch

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