[edk2,v2,13/24] ArmVirtPkg/BaseCachingPciExpressLib: construct at first invocation

Message ID 1460108711-12122-14-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 8, 2016, 9:45 a.m.
Instead of using a constructor, which may reference a dynamic PCD which is
set by the DXE entry point of its user, defer the assignment of the global
mPciExpressBaseAddress until the first the library is actually invoked.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf |  1 -
 ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c              | 16 ++++------------
 2 files changed, 4 insertions(+), 13 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 8, 2016, 7:15 p.m. | #1
On 04/08/16 11:45, Ard Biesheuvel wrote:
> Instead of using a constructor, which may reference a dynamic PCD which is

> set by the DXE entry point of its user, defer the assignment of the global

> mPciExpressBaseAddress until the first the library is actually invoked.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf |  1 -

>  ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c              | 16 ++++------------

>  2 files changed, 4 insertions(+), 13 deletions(-)

> 

> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

> index f6a346d49f22..5374f1b9a369 100644

> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

> @@ -23,7 +23,6 @@ [Defines]

>    MODULE_TYPE                    = BASE

>    VERSION_STRING                 = 1.0

>    LIBRARY_CLASS                  = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION

> -  CONSTRUCTOR                    = PciExpressLibInitialize

>  

>  #

>  #  VALID_ARCHITECTURES           = ARM AARCH64

> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

> index 6479f53b3714..0ce6d118483b 100644

> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

> @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess (

>    return RETURN_UNSUPPORTED;

>  }

>  

> -STATIC UINT64 mPciExpressBaseAddress;

> -

> -RETURN_STATUS

> -EFIAPI

> -PciExpressLibInitialize (

> -  VOID

> -  )

> -{

> -  mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

> -  return RETURN_SUCCESS;

> -}

> -

> +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64;

>  

>  /**

>    Gets the base address of PCI Express.

> @@ -92,6 +81,9 @@ GetPciExpressBaseAddress (

>    VOID

>    )

>  {

> +  if (mPciExpressBaseAddress == MAX_UINT64) {

> +    mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

> +  }

>    return (VOID*)(UINTN) mPciExpressBaseAddress;

>  }

>  

> 


I think this patch is not the right approach.

I would like to preserve the ability to write a DXE_DRIVER that accesses
PCI config space through this PciExpressLib instance, regardless of its
own dispatch order against PciHostBridgeDxe.

(Of course, given that PCI may not be available at all in the guest,
such a DXE_DRIVER will have to check PCI presence explicitly in its
entry point function, similarly to how PciHostBridgeDxe looks at
PcdPciExpressBaseAddress at startup.)

Therefore, I suggest the following:

- In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This
  value is never a valid MMCONFIG base address. It shall mean that
  PcdPciExpressBaseAddress has not been detected yet.

  We should continue to interpret the value 0 as "detected but
  unavailable".

  Any other value means "detected and available".

- Please keep the structure of this library instance (constructor etc).
  In the constructor, if the value of PcdPciExpressBaseAddress is not
  MAX_UINT64, then assign it to mPciExpressBaseAddress, and return
  immediately with success.

  Otherwise, parse the DTB as minimally as possible, using the client
  protocol (-> depex needed), just to determine the value of
  PcdPciExpressBaseAddress. If PCI is present, set the PCD to
  the (nonzero) base address. If PCI is absent (or there is some other
  parse error), set the PCD to zero. Either way, set
  mPciExpressBaseAddress to the same value, and then return with
  success.

For the next patch (i.e., PciHostBridgeDxe):

- The PCI presence check should be preserved in PciHostBridgeDxe, in
  the entry point function (PCD-based). This would (see above) consume
  the product of our one library instance that is linked into
  PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs
  that I mentioned above. Because PciHostBridgeDxe itself links against
  the library, the PCD will never be MAX_UINT64, when the driver entry
  point checks it.

- If that check passes, then PciHostBridgeDxe should implement its own,
  more complete FDT traversal (using the FDT client protocol), in order
  to find all the other characteristics of the pci-host node that it
  needs for operating.

- Going forward, the PCDs (from ArmPkg) that are currently used for
  communicating these "other characteristics" from VirtFdtDxe to
  PciHostBridgeDxe should be eliminated from ArmVirtPkg.

- Drop everything else too that becomes unused (to state the obvious).

Do you agree?

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 11, 2016, 12:17 p.m. | #2
On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/08/16 11:45, Ard Biesheuvel wrote:

>> Instead of using a constructor, which may reference a dynamic PCD which is

>> set by the DXE entry point of its user, defer the assignment of the global

>> mPciExpressBaseAddress until the first the library is actually invoked.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf |  1 -

>>  ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c              | 16 ++++------------

>>  2 files changed, 4 insertions(+), 13 deletions(-)

>>

>> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

>> index f6a346d49f22..5374f1b9a369 100644

>> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

>> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf

>> @@ -23,7 +23,6 @@ [Defines]

>>    MODULE_TYPE                    = BASE

>>    VERSION_STRING                 = 1.0

>>    LIBRARY_CLASS                  = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION

>> -  CONSTRUCTOR                    = PciExpressLibInitialize

>>

>>  #

>>  #  VALID_ARCHITECTURES           = ARM AARCH64

>> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

>> index 6479f53b3714..0ce6d118483b 100644

>> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

>> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c

>> @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess (

>>    return RETURN_UNSUPPORTED;

>>  }

>>

>> -STATIC UINT64 mPciExpressBaseAddress;

>> -

>> -RETURN_STATUS

>> -EFIAPI

>> -PciExpressLibInitialize (

>> -  VOID

>> -  )

>> -{

>> -  mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

>> -  return RETURN_SUCCESS;

>> -}

>> -

>> +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64;

>>

>>  /**

>>    Gets the base address of PCI Express.

>> @@ -92,6 +81,9 @@ GetPciExpressBaseAddress (

>>    VOID

>>    )

>>  {

>> +  if (mPciExpressBaseAddress == MAX_UINT64) {

>> +    mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

>> +  }

>>    return (VOID*)(UINTN) mPciExpressBaseAddress;

>>  }

>>

>>

>

> I think this patch is not the right approach.

>

> I would like to preserve the ability to write a DXE_DRIVER that accesses

> PCI config space through this PciExpressLib instance, regardless of its

> own dispatch order against PciHostBridgeDxe.

>

> (Of course, given that PCI may not be available at all in the guest,

> such a DXE_DRIVER will have to check PCI presence explicitly in its

> entry point function, similarly to how PciHostBridgeDxe looks at

> PcdPciExpressBaseAddress at startup.)

>

> Therefore, I suggest the following:

>

> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This

>   value is never a valid MMCONFIG base address. It shall mean that

>   PcdPciExpressBaseAddress has not been detected yet.

>

>   We should continue to interpret the value 0 as "detected but

>   unavailable".

>

>   Any other value means "detected and available".

>

> - Please keep the structure of this library instance (constructor etc).

>   In the constructor, if the value of PcdPciExpressBaseAddress is not

>   MAX_UINT64, then assign it to mPciExpressBaseAddress, and return

>   immediately with success.

>

>   Otherwise, parse the DTB as minimally as possible, using the client

>   protocol (-> depex needed), just to determine the value of

>   PcdPciExpressBaseAddress. If PCI is present, set the PCD to

>   the (nonzero) base address. If PCI is absent (or there is some other

>   parse error), set the PCD to zero. Either way, set

>   mPciExpressBaseAddress to the same value, and then return with

>   success.

>

> For the next patch (i.e., PciHostBridgeDxe):

>

> - The PCI presence check should be preserved in PciHostBridgeDxe, in

>   the entry point function (PCD-based). This would (see above) consume

>   the product of our one library instance that is linked into

>   PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs

>   that I mentioned above. Because PciHostBridgeDxe itself links against

>   the library, the PCD will never be MAX_UINT64, when the driver entry

>   point checks it.

>

> - If that check passes, then PciHostBridgeDxe should implement its own,

>   more complete FDT traversal (using the FDT client protocol), in order

>   to find all the other characteristics of the pci-host node that it

>   needs for operating.

>

> - Going forward, the PCDs (from ArmPkg) that are currently used for

>   communicating these "other characteristics" from VirtFdtDxe to

>   PciHostBridgeDxe should be eliminated from ArmVirtPkg.

>

> - Drop everything else too that becomes unused (to state the obvious).

>

> Do you agree?

>


Yes, that makes sense. I coded up the first part, which is
straightforward enough, but for the second part, I think it is better
to deal with the MAX_UINT64 case in the code as well. This way, the
module and the library are independent, and we don't ignore the 'reg'
property in the module, which is a bit strange as well considering
that we do consume the rest of the DT node,

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 11, 2016, 1:34 p.m. | #3
On 04/11/16 14:17, Ard Biesheuvel wrote:
> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote:


[snip]

>> I think this patch is not the right approach.

>>

>> I would like to preserve the ability to write a DXE_DRIVER that accesses

>> PCI config space through this PciExpressLib instance, regardless of its

>> own dispatch order against PciHostBridgeDxe.

>>

>> (Of course, given that PCI may not be available at all in the guest,

>> such a DXE_DRIVER will have to check PCI presence explicitly in its

>> entry point function, similarly to how PciHostBridgeDxe looks at

>> PcdPciExpressBaseAddress at startup.)

>>

>> Therefore, I suggest the following:

>>

>> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This

>>   value is never a valid MMCONFIG base address. It shall mean that

>>   PcdPciExpressBaseAddress has not been detected yet.

>>

>>   We should continue to interpret the value 0 as "detected but

>>   unavailable".

>>

>>   Any other value means "detected and available".

>>

>> - Please keep the structure of this library instance (constructor etc).

>>   In the constructor, if the value of PcdPciExpressBaseAddress is not

>>   MAX_UINT64, then assign it to mPciExpressBaseAddress, and return

>>   immediately with success.

>>

>>   Otherwise, parse the DTB as minimally as possible, using the client

>>   protocol (-> depex needed), just to determine the value of

>>   PcdPciExpressBaseAddress. If PCI is present, set the PCD to

>>   the (nonzero) base address. If PCI is absent (or there is some other

>>   parse error), set the PCD to zero. Either way, set

>>   mPciExpressBaseAddress to the same value, and then return with

>>   success.

>>

>> For the next patch (i.e., PciHostBridgeDxe):

>>

>> - The PCI presence check should be preserved in PciHostBridgeDxe, in

>>   the entry point function (PCD-based). This would (see above) consume

>>   the product of our one library instance that is linked into

>>   PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs

>>   that I mentioned above. Because PciHostBridgeDxe itself links against

>>   the library, the PCD will never be MAX_UINT64, when the driver entry

>>   point checks it.

>>

>> - If that check passes, then PciHostBridgeDxe should implement its own,

>>   more complete FDT traversal (using the FDT client protocol), in order

>>   to find all the other characteristics of the pci-host node that it

>>   needs for operating.

>>

>> - Going forward, the PCDs (from ArmPkg) that are currently used for

>>   communicating these "other characteristics" from VirtFdtDxe to

>>   PciHostBridgeDxe should be eliminated from ArmVirtPkg.

>>

>> - Drop everything else too that becomes unused (to state the obvious).

>>

>> Do you agree?

>>

> 

> Yes, that makes sense. I coded up the first part, which is

> straightforward enough, but for the second part, I think it is better

> to deal with the MAX_UINT64 case in the code as well. This way, the

> module and the library are independent, and we don't ignore the 'reg'

> property in the module, which is a bit strange as well considering

> that we do consume the rest of the DT node,


I disagree.

There are several points in your one paragraph above, so I'll have to
elaborate :) (I didn't want to fragment your paragraph into individual
sentences.)

Long version
------------

What you are saying (if I understand it correctly) means that any
DXE_DRIVER that needs PCI config space access would have to prepare
itself for an as-yet uninitialized PCD, and perform the initialization
(= the parsing of "reg") itself, explicitly.

I think this is wrong. I think my point may not have been clear enough:

>> I would like to preserve the ability to write a DXE_DRIVER that

>> accesses PCI config space through this PciExpressLib instance,

>> regardless of its own dispatch order against PciHostBridgeDxe.


(Or maybe it was clear, and it's just me not understanding your point.)

The idea is this: assume for the sake of argument that the "virt"
machine type grows a special (board-specific) PCI B/D/F that is
necessary for implementing a given PI or UEFI protocol, in a DXE_DRIVER
module, without relying on the PCI bus driver and/or PciHostBridgeDxe.

A good example for this, on x86 / OVMF, is the SmmControl2Dxe driver:

- It implements EFI_SMM_CONTROL2_PROTOCOL, in a DXE_RUNTIME_DRIVER.

- It accesses the config space registers of some fixed location PCI
  devices directly with PciLib. (Those B/D/F constants come from Intel's
  ICH9 / Q35 specs.)

- Its entry point function can run before or after the entry point
  function of PciHostBridgeDxe.

This kind of activity is valid for a DXE_(RUNTIME_)DRIVER, and I would
like to keep it available to ArmVirtPkg.

Now, the problem we have here is that PCI(e) is entirely optional in the
"virt" machtype of today, whereas the PciLib / PciExpressLib /
PciSegmentLib classes all assume that PCI(e) is available
unconditionally (and the only variable is "where"). So, these library
classes do not expose a function or variable (or a boolean PCD for that
matter) that allows client modules to interrogate the presense of PCI.

This is the reason why we have to commit a layering violation in all our
PCI-consuming DXE_DRIVER modules, and abuse PcdPciExpressBaseAddress to
see if PCI(e) is available at all.

(Normally a DXE_DRIVER that consumes PciLib / PciExpressLib /
PciSegmentLib has zero business looking at this PCD at all!)

But, I would like to keep this layering violation as minimal as
possible, and keep the detection of the MMCONFIG base address outside of
the client modules.

Which is why I disagree with "deal[ing] with the MAX_UINT64 case in the
[client] code as well". Namely, if a DXE_DRIVER like I described above
does come along sometime later, we will have to duplicate the handling
of MAX_UINT64 in it (the parsing of "reg"). IOW, both drivers would have
to be explicitly aware of the question, "am I the first guy here that
wants PCIe config space access?"

In the current (un-refactored) code, the "reg" property controls the
ConfigBase setting. This property is different from the other properties
because:

- All DXE_DRIVER modules that I describe above (that is, clients of
  PciLib / PciExpressLib / PciSegmentLib) need to rely on it.
  (Indirectly of course, i.e., through the library instance).

- Simultaneously, "reg" is the only such property.

Namely, "bus-range" is irrelevant for a DXE_DRIVER that wants config
space access to a predefined B/D/F location.

And "ranges" is entirely irrelevant for config space. Its entries
describe IO and MMIO ranges, hence "ranges" falls outside of the domain
of PciLib / PciExpressLib / PciSegmentLib.

Thus, "bus-range" and "ranges" are relevant only to PciHostBridgeDxe.

This is why I suggest to:
- handle the "reg" property in the library instance,
- handle everything except the "reg" property in PciHostBridgeDxe.

I don't think that an argument like "we consume the rest of the DT node
in PciHostBridgeDxe anyway" is very relevant here. After all, we handle
the entire device tree in VirtFdtDxe at the moment, and your primary
goal is to split that up along functional lines.

Well, speaking in UEFI and edk2 terms, such a functional line happens to
be drawn through the "pci-host-ecam-generic" node of the DT, between the
"reg" property, and other properties.

Short version
-------------

- The PciLib & co. classes allow a client module to access PCI config
  space. Enumeration, discovery, valid ranges etc. are not their goal.
  Instead, they should enable a client module that knows exact B/D/F
  locations (from hardware specs) to perform such config space accesses.

- Our PciExpressLib instance should support this goal as well,
  regardless of which of its client modules is dispatched first. Which
  implies that the "reg" property should be transparently parsed in the
  library constructor, so the PciLib & co interfaces just work.

- This parsing is expensive (even if it means just the one "reg"
  property). Therefore we should cache the parsed value. The common
  solution for caching the MMCONFIG base address is to save it in
  PcdPciExpressBaseAddress.

- Everything else (the "ranges" and the "bus-range" properties) are
  unrelated to the PciLib & co. classes, hence they should be handled by
  PciHostBridgeDxe separately, explicitly.

- The PciLib & co. classes don't allow a client module to ask "is
  PCI(e) available at all?" In order to cope with this, we need to
  violate the PciLib & co. abstraction a bit, but only as little as
  necessary.

  Our current way is to look at PcdPciExpressBaseAddress in the entry
  point of each such client module. This makes the "availability
  question" overlap the "caching question". If you want, we can
  introduce a new dynamic boolean PCD for the availability question
  separately.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 11, 2016, 1:43 p.m. | #4
On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/11/16 14:17, Ard Biesheuvel wrote:

>> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote:

>

> [snip]

>

>>> I think this patch is not the right approach.

>>>

>>> I would like to preserve the ability to write a DXE_DRIVER that accesses

>>> PCI config space through this PciExpressLib instance, regardless of its

>>> own dispatch order against PciHostBridgeDxe.

>>>

>>> (Of course, given that PCI may not be available at all in the guest,

>>> such a DXE_DRIVER will have to check PCI presence explicitly in its

>>> entry point function, similarly to how PciHostBridgeDxe looks at

>>> PcdPciExpressBaseAddress at startup.)

>>>

>>> Therefore, I suggest the following:

>>>

>>> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This

>>>   value is never a valid MMCONFIG base address. It shall mean that

>>>   PcdPciExpressBaseAddress has not been detected yet.

>>>

>>>   We should continue to interpret the value 0 as "detected but

>>>   unavailable".

>>>

>>>   Any other value means "detected and available".

>>>

>>> - Please keep the structure of this library instance (constructor etc).

>>>   In the constructor, if the value of PcdPciExpressBaseAddress is not

>>>   MAX_UINT64, then assign it to mPciExpressBaseAddress, and return

>>>   immediately with success.

>>>

>>>   Otherwise, parse the DTB as minimally as possible, using the client

>>>   protocol (-> depex needed), just to determine the value of

>>>   PcdPciExpressBaseAddress. If PCI is present, set the PCD to

>>>   the (nonzero) base address. If PCI is absent (or there is some other

>>>   parse error), set the PCD to zero. Either way, set

>>>   mPciExpressBaseAddress to the same value, and then return with

>>>   success.

>>>

>>> For the next patch (i.e., PciHostBridgeDxe):

>>>

>>> - The PCI presence check should be preserved in PciHostBridgeDxe, in

>>>   the entry point function (PCD-based). This would (see above) consume

>>>   the product of our one library instance that is linked into

>>>   PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs

>>>   that I mentioned above. Because PciHostBridgeDxe itself links against

>>>   the library, the PCD will never be MAX_UINT64, when the driver entry

>>>   point checks it.

>>>

>>> - If that check passes, then PciHostBridgeDxe should implement its own,

>>>   more complete FDT traversal (using the FDT client protocol), in order

>>>   to find all the other characteristics of the pci-host node that it

>>>   needs for operating.

>>>

>>> - Going forward, the PCDs (from ArmPkg) that are currently used for

>>>   communicating these "other characteristics" from VirtFdtDxe to

>>>   PciHostBridgeDxe should be eliminated from ArmVirtPkg.

>>>

>>> - Drop everything else too that becomes unused (to state the obvious).

>>>

>>> Do you agree?

>>>

>>

>> Yes, that makes sense. I coded up the first part, which is

>> straightforward enough, but for the second part, I think it is better

>> to deal with the MAX_UINT64 case in the code as well. This way, the

>> module and the library are independent, and we don't ignore the 'reg'

>> property in the module, which is a bit strange as well considering

>> that we do consume the rest of the DT node,

>

> I disagree.

>

> There are several points in your one paragraph above, so I'll have to

> elaborate :) (I didn't want to fragment your paragraph into individual

> sentences.)

>

> Long version

> ------------

>

> What you are saying (if I understand it correctly) means that any

> DXE_DRIVER that needs PCI config space access would have to prepare

> itself for an as-yet uninitialized PCD, and perform the initialization

> (= the parsing of "reg") itself, explicitly.

>

> I think this is wrong. I think my point may not have been clear enough:

>

>>> I would like to preserve the ability to write a DXE_DRIVER that

>>> accesses PCI config space through this PciExpressLib instance,

>>> regardless of its own dispatch order against PciHostBridgeDxe.

>

> (Or maybe it was clear, and it's just me not understanding your point.)

>


No, I did not get the PCI config space nuance here. I also think that
is highly unlikely (and a worse layering violation than the one we are
dealing with) to enumerate PCI host bridges dynamically via DT and
subsequently wire up a driver to a fixed B/D/F.

But I don't feel strongly about this at all, to be honest. The only
thing I will do is add an assertion that there is only a single
'pci-ecam-generic' DT node, since having multiple will break lots of
other stuff anyway, and it also validates the assumption that the
contents of PcdPciExpressBaseAddress correspond with the DT node we
are dealing with.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 11, 2016, 2:12 p.m. | #5
On 04/11/16 15:43, Ard Biesheuvel wrote:
> On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 04/11/16 14:17, Ard Biesheuvel wrote:

>>> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote:

>>

>> [snip]

>>

>>>> I think this patch is not the right approach.

>>>>

>>>> I would like to preserve the ability to write a DXE_DRIVER that accesses

>>>> PCI config space through this PciExpressLib instance, regardless of its

>>>> own dispatch order against PciHostBridgeDxe.

>>>>

>>>> (Of course, given that PCI may not be available at all in the guest,

>>>> such a DXE_DRIVER will have to check PCI presence explicitly in its

>>>> entry point function, similarly to how PciHostBridgeDxe looks at

>>>> PcdPciExpressBaseAddress at startup.)

>>>>

>>>> Therefore, I suggest the following:

>>>>

>>>> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This

>>>>   value is never a valid MMCONFIG base address. It shall mean that

>>>>   PcdPciExpressBaseAddress has not been detected yet.

>>>>

>>>>   We should continue to interpret the value 0 as "detected but

>>>>   unavailable".

>>>>

>>>>   Any other value means "detected and available".

>>>>

>>>> - Please keep the structure of this library instance (constructor etc).

>>>>   In the constructor, if the value of PcdPciExpressBaseAddress is not

>>>>   MAX_UINT64, then assign it to mPciExpressBaseAddress, and return

>>>>   immediately with success.

>>>>

>>>>   Otherwise, parse the DTB as minimally as possible, using the client

>>>>   protocol (-> depex needed), just to determine the value of

>>>>   PcdPciExpressBaseAddress. If PCI is present, set the PCD to

>>>>   the (nonzero) base address. If PCI is absent (or there is some other

>>>>   parse error), set the PCD to zero. Either way, set

>>>>   mPciExpressBaseAddress to the same value, and then return with

>>>>   success.

>>>>

>>>> For the next patch (i.e., PciHostBridgeDxe):

>>>>

>>>> - The PCI presence check should be preserved in PciHostBridgeDxe, in

>>>>   the entry point function (PCD-based). This would (see above) consume

>>>>   the product of our one library instance that is linked into

>>>>   PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs

>>>>   that I mentioned above. Because PciHostBridgeDxe itself links against

>>>>   the library, the PCD will never be MAX_UINT64, when the driver entry

>>>>   point checks it.

>>>>

>>>> - If that check passes, then PciHostBridgeDxe should implement its own,

>>>>   more complete FDT traversal (using the FDT client protocol), in order

>>>>   to find all the other characteristics of the pci-host node that it

>>>>   needs for operating.

>>>>

>>>> - Going forward, the PCDs (from ArmPkg) that are currently used for

>>>>   communicating these "other characteristics" from VirtFdtDxe to

>>>>   PciHostBridgeDxe should be eliminated from ArmVirtPkg.

>>>>

>>>> - Drop everything else too that becomes unused (to state the obvious).

>>>>

>>>> Do you agree?

>>>>

>>>

>>> Yes, that makes sense. I coded up the first part, which is

>>> straightforward enough, but for the second part, I think it is better

>>> to deal with the MAX_UINT64 case in the code as well. This way, the

>>> module and the library are independent, and we don't ignore the 'reg'

>>> property in the module, which is a bit strange as well considering

>>> that we do consume the rest of the DT node,

>>

>> I disagree.

>>

>> There are several points in your one paragraph above, so I'll have to

>> elaborate :) (I didn't want to fragment your paragraph into individual

>> sentences.)

>>

>> Long version

>> ------------

>>

>> What you are saying (if I understand it correctly) means that any

>> DXE_DRIVER that needs PCI config space access would have to prepare

>> itself for an as-yet uninitialized PCD, and perform the initialization

>> (= the parsing of "reg") itself, explicitly.

>>

>> I think this is wrong. I think my point may not have been clear enough:

>>

>>>> I would like to preserve the ability to write a DXE_DRIVER that

>>>> accesses PCI config space through this PciExpressLib instance,

>>>> regardless of its own dispatch order against PciHostBridgeDxe.

>>

>> (Or maybe it was clear, and it's just me not understanding your point.)

>>

> 

> No, I did not get the PCI config space nuance here.


Okay, understood.

> I also think that

> is highly unlikely (and a worse layering violation than the one we are

> dealing with) to enumerate PCI host bridges dynamically via DT and

> subsequently wire up a driver to a fixed B/D/F.


The point is exactly that it's not "subsequent".

Really, it is dictated by the PI and UEFI abstractions (and to some
extent the edk2 code base):

- The PciIo protocol interfaces come from the generic PCI bus driver,
  which is a UEFI_DRIVER, conforming to the UEFI driver model. Which
  implies that drivers that consume PciIo protocol interfaces:

  - have to be UEFI drivers themselves (conforming to the UEFI driver
    model),

  - or must register protocol notify callbacks for PciIo (and then
    their behavior would depend on the platform BDS policy, which
    decides about what UEFI drivers connect what devices)

- Whereas the PI spec dictates, for some protocols, to be produced by
  DXE_(RUNTIME_)DRIVERs, way before BDS is entered (and PciIo instances
  are produced). If these protocols are implementable by accessing the
  PCI config space of a (few) fixed B/D/F(s) on a given platform, then
  those accesses will occur before the PCI bus driver is dispatched (as
  part of BDS).

> But I don't feel strongly about this at all, to be honest. The only

> thing I will do is add an assertion that there is only a single

> 'pci-ecam-generic' DT node,


Add it where?

And add it on top of what else, or instead of what?

> since having multiple will break lots of

> other stuff anyway, and it also validates the assumption that the

> contents of PcdPciExpressBaseAddress correspond with the DT node we

> are dealing with.


Hmm. I assume you mean you'll add the assert in the entry point of
PciHostBridgeDxe?

I agree that >1 should be asserted against, but ==1 is not guaranteed.
==0 should be handled dynamically.

Do you have a fresh v3-wip branch somewhere? I'd like to try expressing
my proposal for patches #13 and #14 in code. (If you are okay with that.)

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 11, 2016, 2:34 p.m. | #6
On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/11/16 15:43, Ard Biesheuvel wrote:

>> On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 04/11/16 14:17, Ard Biesheuvel wrote:

>>>> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote:

>>>

>>> [snip]

>>>

>>>>> I think this patch is not the right approach.

>>>>>

>>>>> I would like to preserve the ability to write a DXE_DRIVER that accesses

>>>>> PCI config space through this PciExpressLib instance, regardless of its

>>>>> own dispatch order against PciHostBridgeDxe.

>>>>>

>>>>> (Of course, given that PCI may not be available at all in the guest,

>>>>> such a DXE_DRIVER will have to check PCI presence explicitly in its

>>>>> entry point function, similarly to how PciHostBridgeDxe looks at

>>>>> PcdPciExpressBaseAddress at startup.)

>>>>>

>>>>> Therefore, I suggest the following:

>>>>>

>>>>> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This

>>>>>   value is never a valid MMCONFIG base address. It shall mean that

>>>>>   PcdPciExpressBaseAddress has not been detected yet.

>>>>>

>>>>>   We should continue to interpret the value 0 as "detected but

>>>>>   unavailable".

>>>>>

>>>>>   Any other value means "detected and available".

>>>>>

>>>>> - Please keep the structure of this library instance (constructor etc).

>>>>>   In the constructor, if the value of PcdPciExpressBaseAddress is not

>>>>>   MAX_UINT64, then assign it to mPciExpressBaseAddress, and return

>>>>>   immediately with success.

>>>>>

>>>>>   Otherwise, parse the DTB as minimally as possible, using the client

>>>>>   protocol (-> depex needed), just to determine the value of

>>>>>   PcdPciExpressBaseAddress. If PCI is present, set the PCD to

>>>>>   the (nonzero) base address. If PCI is absent (or there is some other

>>>>>   parse error), set the PCD to zero. Either way, set

>>>>>   mPciExpressBaseAddress to the same value, and then return with

>>>>>   success.

>>>>>

>>>>> For the next patch (i.e., PciHostBridgeDxe):

>>>>>

>>>>> - The PCI presence check should be preserved in PciHostBridgeDxe, in

>>>>>   the entry point function (PCD-based). This would (see above) consume

>>>>>   the product of our one library instance that is linked into

>>>>>   PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs

>>>>>   that I mentioned above. Because PciHostBridgeDxe itself links against

>>>>>   the library, the PCD will never be MAX_UINT64, when the driver entry

>>>>>   point checks it.

>>>>>

>>>>> - If that check passes, then PciHostBridgeDxe should implement its own,

>>>>>   more complete FDT traversal (using the FDT client protocol), in order

>>>>>   to find all the other characteristics of the pci-host node that it

>>>>>   needs for operating.

>>>>>

>>>>> - Going forward, the PCDs (from ArmPkg) that are currently used for

>>>>>   communicating these "other characteristics" from VirtFdtDxe to

>>>>>   PciHostBridgeDxe should be eliminated from ArmVirtPkg.

>>>>>

>>>>> - Drop everything else too that becomes unused (to state the obvious).

>>>>>

>>>>> Do you agree?

>>>>>

>>>>

>>>> Yes, that makes sense. I coded up the first part, which is

>>>> straightforward enough, but for the second part, I think it is better

>>>> to deal with the MAX_UINT64 case in the code as well. This way, the

>>>> module and the library are independent, and we don't ignore the 'reg'

>>>> property in the module, which is a bit strange as well considering

>>>> that we do consume the rest of the DT node,

>>>

>>> I disagree.

>>>

>>> There are several points in your one paragraph above, so I'll have to

>>> elaborate :) (I didn't want to fragment your paragraph into individual

>>> sentences.)

>>>

>>> Long version

>>> ------------

>>>

>>> What you are saying (if I understand it correctly) means that any

>>> DXE_DRIVER that needs PCI config space access would have to prepare

>>> itself for an as-yet uninitialized PCD, and perform the initialization

>>> (= the parsing of "reg") itself, explicitly.

>>>

>>> I think this is wrong. I think my point may not have been clear enough:

>>>

>>>>> I would like to preserve the ability to write a DXE_DRIVER that

>>>>> accesses PCI config space through this PciExpressLib instance,

>>>>> regardless of its own dispatch order against PciHostBridgeDxe.

>>>

>>> (Or maybe it was clear, and it's just me not understanding your point.)

>>>

>>

>> No, I did not get the PCI config space nuance here.

>

> Okay, understood.

>

>> I also think that

>> is highly unlikely (and a worse layering violation than the one we are

>> dealing with) to enumerate PCI host bridges dynamically via DT and

>> subsequently wire up a driver to a fixed B/D/F.

>

> The point is exactly that it's not "subsequent".

>


I meant subsequent in Git commit log time, not in boot time.

> Really, it is dictated by the PI and UEFI abstractions (and to some

> extent the edk2 code base):

>

> - The PciIo protocol interfaces come from the generic PCI bus driver,

>   which is a UEFI_DRIVER, conforming to the UEFI driver model. Which

>   implies that drivers that consume PciIo protocol interfaces:

>

>   - have to be UEFI drivers themselves (conforming to the UEFI driver

>     model),

>

>   - or must register protocol notify callbacks for PciIo (and then

>     their behavior would depend on the platform BDS policy, which

>     decides about what UEFI drivers connect what devices)

>

> - Whereas the PI spec dictates, for some protocols, to be produced by

>   DXE_(RUNTIME_)DRIVERs, way before BDS is entered (and PciIo instances

>   are produced). If these protocols are implementable by accessing the

>   PCI config space of a (few) fixed B/D/F(s) on a given platform, then

>   those accesses will occur before the PCI bus driver is dispatched (as

>   part of BDS).

>

>> But I don't feel strongly about this at all, to be honest. The only

>> thing I will do is add an assertion that there is only a single

>> 'pci-ecam-generic' DT node,

>

> Add it where?

>

> And add it on top of what else, or instead of what?

>

>> since having multiple will break lots of

>> other stuff anyway, and it also validates the assumption that the

>> contents of PcdPciExpressBaseAddress correspond with the DT node we

>> are dealing with.

>

> Hmm. I assume you mean you'll add the assert in the entry point of

> PciHostBridgeDxe?

>

> I agree that >1 should be asserted against, but ==1 is not guaranteed.

> ==0 should be handled dynamically.

>


I simply want to check that the 'pci-ecam-generic' DT node we end up
consuming is the only one that exists in the device tree. In fact, I
think it makes sense to assert that PcdPciExpressBaseAddress equals
the DT node reg property when we encounter it.

> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>


Sure, give me 30 mins.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 11, 2016, 2:50 p.m. | #7
On 04/11/16 16:34, Ard Biesheuvel wrote:
> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 04/11/16 15:43, Ard Biesheuvel wrote:


[snip]

> I simply want to check that the 'pci-ecam-generic' DT node we end up

> consuming is the only one that exists in the device tree. In fact, I

> think it makes sense to assert that PcdPciExpressBaseAddress equals

> the DT node reg property when we encounter it.


Yes, that definitely makes sense (at least if I understand it
correctly). That is, the library constructor would do the "reg" parsing
/ PCD setting / caching etc (for any DXE_DRIVER that uses this instance,
really), and *specifically* PciHostBridgeDxe, when it parses the same
node (for those other two properties), could also look at "reg", and
assert that PcdPciExpressBaseAddress is *already set* the way it expects
it to be. I think that's a good idea.

>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

>> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>>

> 

> Sure, give me 30 mins.


Let's do the following instead (as we discussed off-list):

Please commit patches v2 1-10 (with the fixes I requested) to the master
branch, and then we can collaborate, on the list, with patches for
fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master
branch.

Cheers!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 11, 2016, 4:22 p.m. | #8
On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/11/16 16:34, Ard Biesheuvel wrote:

>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 04/11/16 15:43, Ard Biesheuvel wrote:

>

> [snip]

>

>> I simply want to check that the 'pci-ecam-generic' DT node we end up

>> consuming is the only one that exists in the device tree. In fact, I

>> think it makes sense to assert that PcdPciExpressBaseAddress equals

>> the DT node reg property when we encounter it.

>

> Yes, that definitely makes sense (at least if I understand it

> correctly). That is, the library constructor would do the "reg" parsing

> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance,

> really), and *specifically* PciHostBridgeDxe, when it parses the same

> node (for those other two properties), could also look at "reg", and

> assert that PcdPciExpressBaseAddress is *already set* the way it expects

> it to be. I think that's a good idea.

>

>>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

>>> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>>>

>>

>> Sure, give me 30 mins.

>

> Let's do the following instead (as we discussed off-list):

>

> Please commit patches v2 1-10 (with the fixes I requested) to the master

> branch, and then we can collaborate, on the list, with patches for

> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master

> branch.

>


OK, wip branch is here
git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3

Thanks
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 11, 2016, 7:14 p.m. | #9
On 04/11/16 18:22, Ard Biesheuvel wrote:
> On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 04/11/16 16:34, Ard Biesheuvel wrote:

>>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:

>>>> On 04/11/16 15:43, Ard Biesheuvel wrote:

>>

>> [snip]

>>

>>> I simply want to check that the 'pci-ecam-generic' DT node we end up

>>> consuming is the only one that exists in the device tree. In fact, I

>>> think it makes sense to assert that PcdPciExpressBaseAddress equals

>>> the DT node reg property when we encounter it.

>>

>> Yes, that definitely makes sense (at least if I understand it

>> correctly). That is, the library constructor would do the "reg" parsing

>> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance,

>> really), and *specifically* PciHostBridgeDxe, when it parses the same

>> node (for those other two properties), could also look at "reg", and

>> assert that PcdPciExpressBaseAddress is *already set* the way it expects

>> it to be. I think that's a good idea.

>>

>>>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

>>>> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>>>>

>>>

>>> Sure, give me 30 mins.

>>

>> Let's do the following instead (as we discussed off-list):

>>

>> Please commit patches v2 1-10 (with the fixes I requested) to the master

>> branch, and then we can collaborate, on the list, with patches for

>> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master

>> branch.

>>

> 

> OK, wip branch is here

> git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3


Great, thank you.

* Patch

  f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol

  is exactly what I had in mind.

  Please modify the commit message (as I pointed out previously, in the
  v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list
  of client drivers, plus update the count "three" to "four". With that,
  you can add my

  Reviewed-by: Laszlo Ersek <lersek@redhat.com>


  to the patch.

  I think it's also fine to push the first two patches to master
  afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol"
  and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node"
  (currently a889387fee53)).

After pushing these two patches above (with my requested updates in the
first one), can you please submit a smaller, separate series that
focuses on the conversion of the PCI host node exclusively? Please see
my comments below:

* Patch

  60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT
               client protocol

  is also precisely what I had in mind.

  - There is one typo in the commit message (with two instances):

    s/PciExpressBaseAddress/PcdPciExpressBaseAddress/

  - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF
    default values in the two DSC files, saying that the MAX_UINT64
    value means "undiscovered".

  - Furthermore, please modify the comment on PcdPciExpressBaseAddress
    in the INF file like this:

    ## CONSUMES ## SOMETIMES_PRODUCES

  With those tweaked:

  Reviewed-by: Laszlo Ersek <lersek@redhat.com>


  (However, this shouldn't be pushed without the rest below.)

* Can you please submit a separate patch that removes
  PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"?

* Patch

  8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol

  also looks mostly good. Some comments:

  - Please fix a typo in the commit message:

    s/host provided/host-provided/

  - Also, the paragraph "This means some PCI related PCDs...", in the
    commit message, belongs to the next patch
    ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please
    move it over.

  - The ProcessPciHost() function's leading comment has become stale.
    Please drop it.

  - Please do not introduce those global variables just for
    returning data from ProcessPciHost() to InitializePciHostBridge().
    Use locals and an appropriate signature for ProcessPciHost(). (You
    can collect those values in a new structure type too.)

    This might require you to bring back those explicit
    zero-assignments, before the "ranges" loop.

  - Since the MmioBase and MmioSize values are no longer kept in UINT32
    PCDs (but UINT64 variables), the explicit (UINT64) cast is
    unnecessary, in InitializePciHostBridge().

  - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are
    redundant, I think, after all of the GetNodeProperty() calls, in
    ProcessPciHost().

  - Can you factor out the setting of the /chosen node to a separate
    function? (Together with the Tmp variable.)

  - In this (v3-wip) iteration, you also removed the

    PcdSetBool (PcdPciDisableBusEnumeration, FALSE);

    call. I almost suggested to reinstate it, but doing so wouldn't be
    safe actually.

    Namely, according to the build report, two drivers consume
    PcdPciDisableBusEnumeration: PciBusDxe and
    QemuFwCfgAcpiPlatformDxe.

    The PciBusDxe case is safe, because it is a UEFI driver model
    driver that depends on protocols produced by PciHostBridgeDxe.
    However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it
    consumes PcdPciDisableBusEnumeration first thing in its entry point.

    This means that you will have to introduce another plugin library
    (in a separate patch) just for setting PcdPciDisableBusEnumeration,
    and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe.

    Also, in this plugin library, since the PCD is just a boolean, we
    cannot use the caching trick (through a special value of the PCD),
    hence minimally the presence of "pci-host-ecam-generic" will have to
    be verified every time.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 12, 2016, 10:10 a.m. | #10
On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/11/16 18:22, Ard Biesheuvel wrote:

>> On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 04/11/16 16:34, Ard Biesheuvel wrote:

>>>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:

>>>>> On 04/11/16 15:43, Ard Biesheuvel wrote:

>>>

>>> [snip]

>>>

>>>> I simply want to check that the 'pci-ecam-generic' DT node we end up

>>>> consuming is the only one that exists in the device tree. In fact, I

>>>> think it makes sense to assert that PcdPciExpressBaseAddress equals

>>>> the DT node reg property when we encounter it.

>>>

>>> Yes, that definitely makes sense (at least if I understand it

>>> correctly). That is, the library constructor would do the "reg" parsing

>>> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance,

>>> really), and *specifically* PciHostBridgeDxe, when it parses the same

>>> node (for those other two properties), could also look at "reg", and

>>> assert that PcdPciExpressBaseAddress is *already set* the way it expects

>>> it to be. I think that's a good idea.

>>>

>>>>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

>>>>> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>>>>>

>>>>

>>>> Sure, give me 30 mins.

>>>

>>> Let's do the following instead (as we discussed off-list):

>>>

>>> Please commit patches v2 1-10 (with the fixes I requested) to the master

>>> branch, and then we can collaborate, on the list, with patches for

>>> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master

>>> branch.

>>>

>>

>> OK, wip branch is here

>> git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3

>

> Great, thank you.

>

> * Patch

>

>   f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol

>

>   is exactly what I had in mind.

>

>   Please modify the commit message (as I pointed out previously, in the

>   v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list

>   of client drivers, plus update the count "three" to "four". With that,

>   you can add my

>

>   Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>

>   to the patch.

>

>   I think it's also fine to push the first two patches to master

>   afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol"

>   and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node"

>   (currently a889387fee53)).

>

> After pushing these two patches above (with my requested updates in the

> first one),


Done

> can you please submit a smaller, separate series that

> focuses on the conversion of the PCI host node exclusively? Please see

> my comments below:

>

> * Patch

>

>   60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT

>                client protocol

>

>   is also precisely what I had in mind.

>

>   - There is one typo in the commit message (with two instances):

>

>     s/PciExpressBaseAddress/PcdPciExpressBaseAddress/

>

>   - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF

>     default values in the two DSC files, saying that the MAX_UINT64

>     value means "undiscovered".

>

>   - Furthermore, please modify the comment on PcdPciExpressBaseAddress

>     in the INF file like this:

>

>     ## CONSUMES ## SOMETIMES_PRODUCES

>

>   With those tweaked:

>

>   Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>

>   (However, this shouldn't be pushed without the rest below.)

>


OK

> * Can you please submit a separate patch that removes

>   PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"?

>


Well, this patch needs to wait after the PCI references are removed
from VirtFdtDxe, and it is already removed by that patch. In fact, I
should probably drop PcdPciDisableBusEnumeration from ArmVirtXen.dsc
in that patch as well.

> * Patch

>

>   8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol

>

>   also looks mostly good. Some comments:

>

>   - Please fix a typo in the commit message:

>

>     s/host provided/host-provided/

>

>   - Also, the paragraph "This means some PCI related PCDs...", in the

>     commit message, belongs to the next patch

>     ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please

>     move it over.

>

>   - The ProcessPciHost() function's leading comment has become stale.

>     Please drop it.

>

>   - Please do not introduce those global variables just for

>     returning data from ProcessPciHost() to InitializePciHostBridge().

>     Use locals and an appropriate signature for ProcessPciHost(). (You

>     can collect those values in a new structure type too.)

>

>     This might require you to bring back those explicit

>     zero-assignments, before the "ranges" loop.

>

>   - Since the MmioBase and MmioSize values are no longer kept in UINT32

>     PCDs (but UINT64 variables), the explicit (UINT64) cast is

>     unnecessary, in InitializePciHostBridge().

>

>   - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are

>     redundant, I think, after all of the GetNodeProperty() calls, in

>     ProcessPciHost().

>

>   - Can you factor out the setting of the /chosen node to a separate

>     function? (Together with the Tmp variable.)

>

>   - In this (v3-wip) iteration, you also removed the

>

>     PcdSetBool (PcdPciDisableBusEnumeration, FALSE);

>

>     call. I almost suggested to reinstate it, but doing so wouldn't be

>     safe actually.

>

>     Namely, according to the build report, two drivers consume

>     PcdPciDisableBusEnumeration: PciBusDxe and

>     QemuFwCfgAcpiPlatformDxe.

>

>     The PciBusDxe case is safe, because it is a UEFI driver model

>     driver that depends on protocols produced by PciHostBridgeDxe.

>     However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it

>     consumes PcdPciDisableBusEnumeration first thing in its entry point.

>

>     This means that you will have to introduce another plugin library

>     (in a separate patch) just for setting PcdPciDisableBusEnumeration,

>     and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe.

>

>     Also, in this plugin library, since the PCD is just a boolean, we

>     cannot use the caching trick (through a special value of the PCD),

>     hence minimally the presence of "pci-host-ecam-generic" will have to

>     be verified every time.

>


OK, that all looks reasonable. I will send it out as a separate
series, and once we are happy with those, I will repost the remaining
patches.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 12, 2016, 10:15 a.m. | #11
On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/11/16 18:22, Ard Biesheuvel wrote:

>> On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 04/11/16 16:34, Ard Biesheuvel wrote:

>>>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote:

>>>>> On 04/11/16 15:43, Ard Biesheuvel wrote:

>>>

>>> [snip]

>>>

>>>> I simply want to check that the 'pci-ecam-generic' DT node we end up

>>>> consuming is the only one that exists in the device tree. In fact, I

>>>> think it makes sense to assert that PcdPciExpressBaseAddress equals

>>>> the DT node reg property when we encounter it.

>>>

>>> Yes, that definitely makes sense (at least if I understand it

>>> correctly). That is, the library constructor would do the "reg" parsing

>>> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance,

>>> really), and *specifically* PciHostBridgeDxe, when it parses the same

>>> node (for those other two properties), could also look at "reg", and

>>> assert that PcdPciExpressBaseAddress is *already set* the way it expects

>>> it to be. I think that's a good idea.

>>>

>>>>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing

>>>>> my proposal for patches #13 and #14 in code. (If you are okay with that.)

>>>>>

>>>>

>>>> Sure, give me 30 mins.

>>>

>>> Let's do the following instead (as we discussed off-list):

>>>

>>> Please commit patches v2 1-10 (with the fixes I requested) to the master

>>> branch, and then we can collaborate, on the list, with patches for

>>> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master

>>> branch.

>>>

>>

>> OK, wip branch is here

>> git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3

>

> Great, thank you.

>

> * Patch

>

>   f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol

>

>   is exactly what I had in mind.

>

>   Please modify the commit message (as I pointed out previously, in the

>   v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list

>   of client drivers, plus update the count "three" to "four". With that,

>   you can add my

>

>   Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>

>   to the patch.

>

>   I think it's also fine to push the first two patches to master

>   afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol"

>   and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node"

>   (currently a889387fee53)).

>

> After pushing these two patches above (with my requested updates in the

> first one), can you please submit a smaller, separate series that

> focuses on the conversion of the PCI host node exclusively? Please see

> my comments below:

>

> * Patch

>

>   60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT

>                client protocol

>

>   is also precisely what I had in mind.

>

>   - There is one typo in the commit message (with two instances):

>

>     s/PciExpressBaseAddress/PcdPciExpressBaseAddress/

>

>   - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF

>     default values in the two DSC files, saying that the MAX_UINT64

>     value means "undiscovered".

>

>   - Furthermore, please modify the comment on PcdPciExpressBaseAddress

>     in the INF file like this:

>

>     ## CONSUMES ## SOMETIMES_PRODUCES

>

>   With those tweaked:

>

>   Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>

>   (However, this shouldn't be pushed without the rest below.)

>

> * Can you please submit a separate patch that removes

>   PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"?

>

> * Patch

>

>   8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol

>

>   also looks mostly good. Some comments:

>

>   - Please fix a typo in the commit message:

>

>     s/host provided/host-provided/

>

>   - Also, the paragraph "This means some PCI related PCDs...", in the

>     commit message, belongs to the next patch

>     ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please

>     move it over.

>

>   - The ProcessPciHost() function's leading comment has become stale.

>     Please drop it.

>

>   - Please do not introduce those global variables just for

>     returning data from ProcessPciHost() to InitializePciHostBridge().

>     Use locals and an appropriate signature for ProcessPciHost(). (You

>     can collect those values in a new structure type too.)

>

>     This might require you to bring back those explicit

>     zero-assignments, before the "ranges" loop.

>

>   - Since the MmioBase and MmioSize values are no longer kept in UINT32

>     PCDs (but UINT64 variables), the explicit (UINT64) cast is

>     unnecessary, in InitializePciHostBridge().

>

>   - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are

>     redundant, I think, after all of the GetNodeProperty() calls, in

>     ProcessPciHost().

>

>   - Can you factor out the setting of the /chosen node to a separate

>     function? (Together with the Tmp variable.)

>

>   - In this (v3-wip) iteration, you also removed the

>

>     PcdSetBool (PcdPciDisableBusEnumeration, FALSE);

>

>     call. I almost suggested to reinstate it, but doing so wouldn't be

>     safe actually.

>

>     Namely, according to the build report, two drivers consume

>     PcdPciDisableBusEnumeration: PciBusDxe and

>     QemuFwCfgAcpiPlatformDxe.

>

>     The PciBusDxe case is safe, because it is a UEFI driver model

>     driver that depends on protocols produced by PciHostBridgeDxe.

>     However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it

>     consumes PcdPciDisableBusEnumeration first thing in its entry point.

>

>     This means that you will have to introduce another plugin library

>     (in a separate patch) just for setting PcdPciDisableBusEnumeration,

>     and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe.

>

>     Also, in this plugin library, since the PCD is just a boolean, we

>     cannot use the caching trick (through a special value of the PCD),

>     hence minimally the presence of "pci-host-ecam-generic" will have to

>     be verified every time.

>


Actually, looking at this again, wouldn't it make sense to have a
single plugin library that sets both PcdPciDisableBusEnumeration and
PcdPciExpressBaseAddress, and use it for these two modules and for the
BaseCachingPciExpressLib?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 12, 2016, 10:35 a.m. | #12
On 04/12/16 12:10, Ard Biesheuvel wrote:
> On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:


>> * Can you please submit a separate patch that removes

>>   PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"?

>>

> 

> Well, this patch needs to wait after the PCI references are removed

> from VirtFdtDxe, and it is already removed by that patch. In fact, I

> should probably drop PcdPciDisableBusEnumeration from ArmVirtXen.dsc

> in that patch as well.


Okay.

>> * Patch

>>

>>   8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol

>>

>>   also looks mostly good. Some comments:


> 

> OK, that all looks reasonable. I will send it out as a separate

> series, and once we are happy with those, I will repost the remaining

> patches.


That's what I thought as well.

Thanks!
Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 12, 2016, 10:59 a.m. | #13
On 04/12/16 12:15, Ard Biesheuvel wrote:
> On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:


>> * Patch

>>

>>   8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol

>>

>>   also looks mostly good. Some comments:

>>

>>   - Please fix a typo in the commit message:

>>

>>     s/host provided/host-provided/

>>

>>   - Also, the paragraph "This means some PCI related PCDs...", in the

>>     commit message, belongs to the next patch

>>     ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please

>>     move it over.

>>

>>   - The ProcessPciHost() function's leading comment has become stale.

>>     Please drop it.

>>

>>   - Please do not introduce those global variables just for

>>     returning data from ProcessPciHost() to InitializePciHostBridge().

>>     Use locals and an appropriate signature for ProcessPciHost(). (You

>>     can collect those values in a new structure type too.)

>>

>>     This might require you to bring back those explicit

>>     zero-assignments, before the "ranges" loop.

>>

>>   - Since the MmioBase and MmioSize values are no longer kept in UINT32

>>     PCDs (but UINT64 variables), the explicit (UINT64) cast is

>>     unnecessary, in InitializePciHostBridge().

>>

>>   - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are

>>     redundant, I think, after all of the GetNodeProperty() calls, in

>>     ProcessPciHost().

>>

>>   - Can you factor out the setting of the /chosen node to a separate

>>     function? (Together with the Tmp variable.)

>>

>>   - In this (v3-wip) iteration, you also removed the

>>

>>     PcdSetBool (PcdPciDisableBusEnumeration, FALSE);

>>

>>     call. I almost suggested to reinstate it, but doing so wouldn't be

>>     safe actually.

>>

>>     Namely, according to the build report, two drivers consume

>>     PcdPciDisableBusEnumeration: PciBusDxe and

>>     QemuFwCfgAcpiPlatformDxe.

>>

>>     The PciBusDxe case is safe, because it is a UEFI driver model

>>     driver that depends on protocols produced by PciHostBridgeDxe.

>>     However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it

>>     consumes PcdPciDisableBusEnumeration first thing in its entry point.

>>

>>     This means that you will have to introduce another plugin library

>>     (in a separate patch) just for setting PcdPciDisableBusEnumeration,

>>     and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe.

>>

>>     Also, in this plugin library, since the PCD is just a boolean, we

>>     cannot use the caching trick (through a special value of the PCD),

>>     hence minimally the presence of "pci-host-ecam-generic" will have to

>>     be verified every time.

>>

> 

> Actually, looking at this again, wouldn't it make sense to have a

> single plugin library that sets both PcdPciDisableBusEnumeration and

> PcdPciExpressBaseAddress, and use it for these two modules and for the

> BaseCachingPciExpressLib?

> 


This could be a valid idea, but it requires some care.

- BaseCachingPciExpressLib will have to spell out the dependency on
  this additional library class explicitly, otherwise the order of
  constructors would not be ensured. In other words, for clients of
  BaseCachingPciExpressLib, the new library instance would not be
  "plugged", but regularly inherited.

  It should also mean that the source code of the
  BaseCachingPciExpressLib instance doesn't change (relative to
  master), only the [LibraryClasses] section does, in its INF file.

- For modules that need PcdPciDisableBusEnumeration, the plugin lib
  instance would have to remain a plugin. I'm unsure about the case
  when a module gets the plugin both implicitly and explicitly.

- Should the condition (PcdPciExpressBaseAddress != MAX_UINT64) imply
  that PcdPciDisableBusEnumeration has been finalized as well? Probably
  so, but this should be documented in the DSC files, near the
  MAX_UINT64 default value.

- The above co-caching saves some cycles, and we avoid yet another
  library class+instance. That's good. However, we should check how
  much potential is there for wasted work.

  For a client of PciExpressLib, the waste is only a single
  PcdSetBool(), which is negligible.

  For a client that needs only PcdPciDisableBusEnumeration, the waste
  is: a PcdSet64(), which is negligible, and the fact that we don't
  just look up the "pci-host-ecam-generic" node, but also grab its
  "reg" property (plus a SwapBytes64() call). I don't think it's bad.

  Clearly, the "waste" in the above cases can only be called "waste" if
  nothing in the firmware needs that "other" thing later. And even
  then, the "waste" is spent only once, due to the caching through
  (PcdPciExpressBaseAddress != MAX_UINT64).

So, I think this is worth a shot!

I wonder what a good name could be, for this new plugin lib.
PciPcdProducerLib?

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 12, 2016, 11:15 a.m. | #14
On 04/12/16 12:59, Laszlo Ersek wrote:

> I wonder what a good name could be, for this new plugin lib.

> PciPcdProducerLib?


Maybe stick in a reference to "Fdt" as well...

Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
index f6a346d49f22..5374f1b9a369 100644
--- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
+++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
@@ -23,7 +23,6 @@  [Defines]
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
-  CONSTRUCTOR                    = PciExpressLibInitialize
 
 #
 #  VALID_ARCHITECTURES           = ARM AARCH64
diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c
index 6479f53b3714..0ce6d118483b 100644
--- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c
+++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c
@@ -68,18 +68,7 @@  PciExpressRegisterForRuntimeAccess (
   return RETURN_UNSUPPORTED;
 }
 
-STATIC UINT64 mPciExpressBaseAddress;
-
-RETURN_STATUS
-EFIAPI
-PciExpressLibInitialize (
-  VOID
-  )
-{
-  mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
-  return RETURN_SUCCESS;
-}
-
+STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64;
 
 /**
   Gets the base address of PCI Express.
@@ -92,6 +81,9 @@  GetPciExpressBaseAddress (
   VOID
   )
 {
+  if (mPciExpressBaseAddress == MAX_UINT64) {
+    mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
+  }
   return (VOID*)(UINTN) mPciExpressBaseAddress;
 }