Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges

["Leif Lindholm" <quic_llindhol@quicinc.com>]

Hi Jeff,

On Tue, Sep 12, 2023 at 19:04:26 +0000, Jeff Brasen wrote:
> Regarding the signed-off-by I wasn't sure the right way to handle
> this. Vidya was the author of this patch and applied the signed off
> on our internal repo during development. I was upstreaming it on his
> behalf. I was unsure if I should just replace his as I assumed just
> leaving his from this wouldn't be ideal as I figured we would want
> the signed-off by the submitter to the devel list.

If I was sending this patch out, I would drop reviewed-by and
signed-off-by both. Git format-patch sets the From: tag, so authorship
is preserved regardless. But I try to separate what i prefer from what
I think is absolutely needed.

And yes, you're 100% right we need your signed-off-by in any case.

> Here is the copy of the patch on our edk2 fork as
> well. https://github.com/NVIDIA/edk2/commit/0171b6c1f60500c5e5178ef3521fa14bcacd3488

Yeah, so with that providence, I'm OK with keeping both tags - *but*
I'd then appreciate having the statement
"this is me contributing
https://github.com/NVIDIA/edk2/commit/0171b6c1f60500c5e5178ef3521fa14bcacd3488
from our tree" to the commit message.

/
    Leif

> > -----Original Message-----
> > From: Leif Lindholm <quic_llindhol@quicinc.com>
> > Sent: Tuesday, September 12, 2023 3:36 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org;
> > Sami.Mujawar@arm.com; pierre.gondois@arm.com; Vidya Sagar
> > <vidyas@nvidia.com>; Shanker Donthineni <sdonthineni@nvidia.com>
> > Subject: Re: [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O
> > ranges
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Jeff,
> > 
> > On Mon, Sep 11, 2023 at 23:48:57 +0000, Jeff Brasen wrote:
> > > From: Vidya Sagar <vidyas@nvidia.com>
> > >
> > > Add helper functions to generate AML Resource Data describing I/O
> > > ranges of four words long. API AmlCodeGenRdQWordIo () is exposed.
> > >
> > > Reviewed-by: Shanker Donthineni <sdonthineni@nvidia.com>
> > 
> > The above isn't really applicable to upstream.
> > Although I feel less strongly about that than
> > 
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > 
> > The DCO is a statement that you have performed basic legal due diligence on
> > the provenance of the change. I'm uncomfortable with people making such
> > statements on behalf of others.
> > 
> > If this is being upstreamed from a downstream repository, such that the
> > review trail is available there, then both of these could be fine.
> > But I think it would be useful to include a link to the patch in that repository in
> > the commit message in that case.
> > 
> > One technical, but not necessarily for this set (it just made me spot it), note
> > below.
> > 
> > > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > > ---
> > >  .../Include/Library/AmlLib/AmlLib.h           | 67 ++++++++++++++
> > >  .../AmlLib/CodeGen/AmlResourceDataCodeGen.c   | 90
> > +++++++++++++++++++
> > >  2 files changed, 157 insertions(+)
> > >
> > > diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> > > b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> > > index 9210c5091548..8e24cecdd77b 100644
> > > --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> > > +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> > > @@ -683,6 +683,73 @@ AmlCodeGenRdWordBusNumber (
> > >    OUT       AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
> > >    );
> > >
> > > +/** Code generation for the "QWordIO ()" ASL function.
> > > +
> > > +  The Resource Data effectively created is a QWord Address Space
> > > + Resource  Data. Cf ACPI 6.4:
> > > +   - s6.4.3.5.1 "QWord Address Space Descriptor".
> > > +   - s19.6.109 "QWordIO".
> > > +
> > > +  The created resource data node can be:
> > > +   - appended to the list of resource data elements of the NameOpNode.
> > > +     In such case NameOpNode must be defined by a the "Name ()" ASL
> > statement
> > > +     and initially contain a "ResourceTemplate ()".
> > > +   - returned through the NewRdNode parameter.
> > > +
> > > +  See ACPI 6.4 spec, s19.6.109 for more.
> > > +
> > > +  @param [in]  IsResourceConsumer   ResourceUsage parameter.
> > > +  @param [in]  IsMinFixed           Minimum address is fixed.
> > > +  @param [in]  IsMaxFixed           Maximum address is fixed.
> > > +  @param [in]  IsPosDecode          Decode parameter
> > > +  @param [in]  IsaRanges            Possible values are:
> > > +                                     0-Reserved
> > > +                                     1-NonISAOnly
> > > +                                     2-ISAOnly
> > > +                                     3-EntireRange
> > 
> > This is an existing antipattern which this patch (rightly) adheres to when
> > adding an additional variant of an existing API. But this also pushes the count
> > to three functions in the same file where we're doing enum-but-in-doxygen
> > and then keep magic values in the code.
> > 
> > I think someone should rewrite this as an enum and get rid of the magic values
> > in the callers.
> > 
> > An additional antipattern is that because the doxygen stanza becomes
> > exessively bulky, it leaves out actually documenting the parameter at all.
> > 
> > But as I said, that's not the fault of this set, and does not need to be fixed by it.
> > 
> > /
> >     Leif
> > 
> > > +  @param [in]  AddressGranularity   Address granularity.
> > > +  @param [in]  AddressMinimum       Minimum address.
> > > +  @param [in]  AddressMaximum       Maximum address.
> > > +  @param [in]  AddressTranslation   Address translation.
> > > +  @param [in]  RangeLength          Range length.
> > > +  @param [in]  ResourceSourceIndex  Resource Source index.
> > > +                                    Unused. Must be 0.
> > > +  @param [in]  ResourceSource       Resource Source.
> > > +                                    Unused. Must be NULL.
> > > +  @param [in]  IsDenseTranslation   TranslationDensity parameter.
> > > +  @param [in]  IsTypeStatic         TranslationType parameter.
> > > +  @param [in]  NameOpNode           NameOp object node defining a named
> > object.
> > > +                                    If provided, append the new resource data
> > > +                                    node to the list of resource data elements
> > > +                                    of this node.
> > > +  @param [out] NewRdNode            If provided and success,
> > > +                                    contain the created node.
> > > +
> > > +  @retval EFI_SUCCESS             The function completed successfully.
> > > +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> > > +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +AmlCodeGenRdQWordIo (
> > > +  IN        BOOLEAN IsResourceConsumer,
> > > +  IN        BOOLEAN IsMinFixed,
> > > +  IN        BOOLEAN IsMaxFixed,
> > > +  IN        BOOLEAN IsPosDecode,
> > > +  IN        UINT8 IsaRanges,
> > > +  IN        UINT64 AddressGranularity,
> > > +  IN        UINT64 AddressMinimum,
> > > +  IN        UINT64 AddressMaximum,
> > > +  IN        UINT64 AddressTranslation,
> > > +  IN        UINT64 RangeLength,
> > > +  IN        UINT8 ResourceSourceIndex,
> > > +  IN  CONST CHAR8 *ResourceSource,
> > > +  IN        BOOLEAN IsDenseTranslation,
> > > +  IN        BOOLEAN IsTypeStatic,
> > > +  IN        AML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
> > > +  OUT       AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
> > > +  );
> > > +
> > >  /** Code generation for the "QWordMemory ()" ASL function.
> > >
> > >    The Resource Data effectively created is a QWord Address Space
> > > Resource diff --git
> > >
> > a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo
> > deGe
> > > n.c
> > >
> > b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo
> > deGe
> > > n.c index 4ca63ccd2396..9c6700b9e08c 100644
> > > ---
> > >
> > a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo
> > deGe
> > > n.c
> > > +++
> > b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo
> > > +++ deGen.c
> > > @@ -1012,6 +1012,96 @@ AmlCodeGenRdQWordSpace (
> > >    return LinkRdNode (RdNode, NameOpNode, NewRdNode);  }
> > >
> > > +/** Code generation for the "QWordIO ()" ASL function.
> > > +
> > > +  The Resource Data effectively created is a QWord Address Space
> > > + Resource  Data. Cf ACPI 6.4:
> > > +   - s6.4.3.5.1 "QWord Address Space Descriptor".
> > > +   - s19.6.109 "QWordIO".
> > > +
> > > +  The created resource data node can be:
> > > +   - appended to the list of resource data elements of the NameOpNode.
> > > +     In such case NameOpNode must be defined by a the "Name ()" ASL
> > statement
> > > +     and initially contain a "ResourceTemplate ()".
> > > +   - returned through the NewRdNode parameter.
> > > +
> > > +  See ACPI 6.4 spec, s19.6.109 for more.
> > > +
> > > +  @param [in]  IsResourceConsumer   ResourceUsage parameter.
> > > +  @param [in]  IsMinFixed           Minimum address is fixed.
> > > +  @param [in]  IsMaxFixed           Maximum address is fixed.
> > > +  @param [in]  IsPosDecode          Decode parameter
> > > +  @param [in]  IsaRanges            Possible values are:
> > > +                                     0-Reserved
> > > +                                     1-NonISAOnly
> > > +                                     2-ISAOnly
> > > +                                     3-EntireRange
> > > +  @param [in]  AddressGranularity   Address granularity.
> > > +  @param [in]  AddressMinimum       Minimum address.
> > > +  @param [in]  AddressMaximum       Maximum address.
> > > +  @param [in]  AddressTranslation   Address translation.
> > > +  @param [in]  RangeLength          Range length.
> > > +  @param [in]  ResourceSourceIndex  Resource Source index.
> > > +                                    Unused. Must be 0.
> > > +  @param [in]  ResourceSource       Resource Source.
> > > +                                    Unused. Must be NULL.
> > > +  @param [in]  IsDenseTranslation   TranslationDensity parameter.
> > > +  @param [in]  IsTypeStatic         TranslationType parameter.
> > > +  @param [in]  NameOpNode           NameOp object node defining a named
> > object.
> > > +                                    If provided, append the new resource data
> > > +                                    node to the list of resource data elements
> > > +                                    of this node.
> > > +  @param [out] NewRdNode            If provided and success,
> > > +                                    contain the created node.
> > > +
> > > +  @retval EFI_SUCCESS             The function completed successfully.
> > > +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> > > +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +AmlCodeGenRdQWordIo (
> > > +  IN        BOOLEAN IsResourceConsumer,
> > > +  IN        BOOLEAN IsMinFixed,
> > > +  IN        BOOLEAN IsMaxFixed,
> > > +  IN        BOOLEAN IsPosDecode,
> > > +  IN        UINT8 IsaRanges,
> > > +  IN        UINT64 AddressGranularity,
> > > +  IN        UINT64 AddressMinimum,
> > > +  IN        UINT64 AddressMaximum,
> > > +  IN        UINT64 AddressTranslation,
> > > +  IN        UINT64 RangeLength,
> > > +  IN        UINT8 ResourceSourceIndex,
> > > +  IN  CONST CHAR8 *ResourceSource,
> > > +  IN        BOOLEAN IsDenseTranslation,
> > > +  IN        BOOLEAN IsTypeStatic,
> > > +  IN        AML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL
> > > +  OUT       AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
> > > +  )
> > > +{
> > > +  return AmlCodeGenRdQWordSpace (
> > > +           ACPI_ADDRESS_SPACE_TYPE_IO,
> > > +           IsResourceConsumer,
> > > +           IsPosDecode,
> > > +           IsMinFixed,
> > > +           IsMaxFixed,
> > > +           RdIoRangeSpecificFlags (
> > > +             IsaRanges,
> > > +             IsDenseTranslation,
> > > +             IsTypeStatic
> > > +             ),
> > > +           AddressGranularity,
> > > +           AddressMinimum,
> > > +           AddressMaximum,
> > > +           AddressTranslation,
> > > +           RangeLength,
> > > +           ResourceSourceIndex,
> > > +           ResourceSource,
> > > +           NameOpNode,
> > > +           NewRdNode
> > > +           );
> > > +}
> > > +
> > >  /** Code generation for the "QWordMemory ()" ASL function.
> > >
> > >    The Resource Data effectively created is a QWord Address Space
> > > Resource
> > > --
> > > 2.25.1
> > >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108552): https://edk2.groups.io/g/devel/message/108552
Mute This Topic: https://groups.io/mt/101305535/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-