From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Guo Heyi <heyi.guo@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
edk2-devel@lists.01.org,
Michael D Kinney <michael.d.kinney@intel.com>,
Laszlo Ersek <lersek@redhat.com>, Star Zeng <star.zeng@intel.com>
Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Wed, 28 Feb 2018 15:25:03 +0800 [thread overview]
Message-ID: <17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com> (raw)
In-Reply-To: <55c50dc9-4ce4-60a6-342c-35cd2f8d37cd@Intel.com>
On 2/28/2018 10:29 AM, Ni, Ruiyu wrote:
> On 2/27/2018 7:44 PM, Guo Heyi wrote:
>> Hi all,
>>
>> I believe we have come to conclusion for major parts, so I'm going to
>> send out
>> formal patches instead of RFC from now on, as well as applying changes to
>> the existing implementations of PciHostBridgeLib in the tree.
>>
>> Thanks for your comments and advice.
>
> Heyi,
> I saw you had a very good summary about where the Device Address is
> used, where the Host Address is used.
> I think it would be better if you could split them into two categories:
> Protocol interfaces and internal implementations.
>
> Only the following protocol interfaces assume Address is Device Address:
> (1). PciHostBridgeResourceAllocation.GetProposedResources()
> Otherwise PCI bus driver cannot set correct address into PCI BARs.
> (2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
> (3). PciRootBridgeIo.CopyMem()
> UEFI and PI spec have clear statements for all other protocol interfaces
> about the address type.
>
> Library interfaces and internal implementation:
> (1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> It is easy to check whether the address is below 4G or above 4G.
> (2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
> address, for they are allocated from GCD.
> (3). Address passed to PciHostBridgeResourceConflict is host address,
> for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.
>
> Thanks,
> Ray
>
>>
>> Heyi
>>
>> On Tue, Feb 27, 2018 at 06:45:01PM +0800, Guo Heyi wrote:
>>> On Tue, Feb 27, 2018 at 05:59:48PM +0800, Ni, Ruiyu wrote:
>>>> On 2/27/2018 5:33 PM, Guo Heyi wrote:
>>>>> Hi Ray,
>>>>>
>>>>> Thanks for your comments. It seems comments 10 and 12 are the major
>>>>> issue; let's
>>>>> discuss that first.
>>>>>
>>>>> 1. In current implementation, namely PciBusDxe and PciHostBridgeDxe
>>>>> both in
>>>>> MdeModulePkg, Address parameter passed to RootBridgeIoMemRead comes
>>>>> from
>>>>> PciIoMemRead().
>>>>> 2. In PciIoMemRead(), Offset is only an offset within selected BAR;
>>>>> then
>>>>> PciIoDevice->PciBar[BarIndex].BaseAddress is added to Offset in
>>>>> PciIoVerifyBarAccess().
>>>> Agree. I thought PciIoMemRead() gets the BAR base address from
>>>> GetBarAttributes().
>>>>
>>>>> 3. So whether the "Address" passed to RootBridgeIoMemRead() is host
>>>>> address or
>>>>> device address depends on the property of
>>>>> PciIoDevice->PciBar[BarIndex].BaseAddress.
>>>> RootBridgeIoMemRead() can choose to accept HOST address or Device
>>>> address.
>>>> Either can be implemented.
>>>> I am fine to your proposal to use Device Address.
>>>>
>>>>> 4. In PciBusDxe, we can see
>>>>> PciIoDevice->PciBar[BarIndex].BaseAddress simply
>>>>> comes from the value read from BAR register, which is absolutely a
>>>>> device
>>>>> address.
>>>>> 5. What we do in patch 3/3 is also to convert the device address in
>>>>> PciBar[BarIndex].BaseAddress to host address before returning to
>>>>> the caller of
>>>>> PciIoGetBarAttributes().
>>>>>
>>>>> Please let me know your comments.
>>>>>
>>>>> For other comments, I have no strong opinion and I can follow the
>>>>> conclusion of
>>>>> the community.
>>>>
>>>> So the issue (13) (not 12) is closed.
>>>> But what's your opinion to my comment (1): Explicitly add assertion and
>>>> if-check to make sure the alignment meets requirement.
>>>
>>> Ah, I thought it is obviously a good advice to place the restriction
>>> in the code
>>> rather than in documentation only, so I didn't reply it separately :)
>>>
>>> Thanks,
>>>
>>> Heyi
>>>
>>>>
>>>> Ard,
>>>> I provided the detailed comments because I felt the code is almost
>>>> finalized.
>>>> So the detailed comments can avoid creating more versions of patches.
>>>>
>>>> For the EMPTY comments and STATIC modifier, I created the first
>>>> version of
>>>> this driver, so I'd like to make the coding style in this driver is
>>>> consistent.
>>>> But if you insist to use a different style, I am fine as long as the
>>>> coding
>>>> style rule allows.
>>>>
>>>> Thanks,
>>>> Ray
>>>>
>>>>>
>>>>> Thanks and regards,
>>>>>
>>>>> Heyi
>>>>>
>>>>> On Tue, Feb 27, 2018 at 04:48:47PM +0800, Ni, Ruiyu wrote:
>>>>>> On 2/27/2018 10:09 AM, Heyi Guo wrote:
>>>>>>> PCI address translation is necessary for some non-x86 platforms. On
>>>>>>> such platforms, address value (denoted as "device address" or
>>>>>>> "address
>>>>>>> in PCI view") set to PCI BAR registers in configuration space
>>>>>>> might be
>>>>>>> different from the address which is used by CPU to access the
>>>>>>> registers in memory BAR or IO BAR spaces (denoted as "host
>>>>>>> address" or
>>>>>>> "address in CPU view"). The difference between the two addresses is
>>>>>>> called "Address Translation Offset" or simply "translation", and can
>>>>>>> be represented by "Address Translation Offset" in ACPI QWORD Address
>>>>>>> Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
>>>>>>> definitions of QWORD Address Space Descriptor, and we will follow
>>>>>>> UEFI
>>>>>>> definition on UEFI protocols, such as PCI root bridge IO protocol
>>>>>>> and
>>>>>>> PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is
>>>>>>> "Offset
>>>>>>> to apply to the Starting address to convert it to a PCI address".
>>>>>>> This
>>>>>>> means:
>>>>>>>
>>>>>>> 1. Translation = device address - host address.
>>>>>>>
>>>>>>> 2. PciRootBridgeIo->Configuration should return CPU view address, as
>>>>>>> well as PciIo->GetBarAttributes.
>>>>>>>
>>>>>>> Summary of addresses used:
>>>>>>>
>>>>>>> 1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address,
>>>>>>> for
>>>>>>> it is easy to check whether the address is below 4G or above 4G.
>>>>>>>
>>>>>>> 2. Address returned by
>>>>>>> EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources
>>>>>>>
>>>>>>> is device address, or else PCI bus driver cannot set correct address
>>>>>>> into PCI BAR registers.
>>>>>>>
>>>>>>> 3. Address returned by PciRootBridgeIo->Configuration is host
>>>>>>> address
>>>>>>> per UEFI 2.7 definition.
>>>>>>>
>>>>>>> 4. Addresses used in GCD manipulation are host address.
>>>>>>>
>>>>>>> 5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host
>>>>>>> address, for they are allocated from GCD.
>>>>>>>
>>>>>>> 6. Address passed to PciHostBridgeResourceConflict is host address,
>>>>>>> for it comes from ResAllocNode.
>>>>>>>
>>>>>>> RESTRICTION: to simplify the situation, we require the alignment of
>>>>>>> Translation must be larger than any BAR alignment in the same root
>>>>>>> bridge, so that resource allocation alignment can be applied to both
>>>>>>> device address and host address.
>>>>>> (1). I'd like to see this restriction be reflected in code.
>>>>>> Both assertion and if-check are needed to ensure debug firmware hang
>>>>>> and release firmware return fail status from PciHostBridge driver.
>>>>>>
>>>>>>>
>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Notes:
>>>>>>> v4:
>>>>>>> - Add ASSERT (FALSE) to default branch in
>>>>>>> GetTranslationByResourceType
>>>>>>> [Laszlo]
>>>>>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
>>>>>>> gDS->AllocateMemorySpace [Laszlo]
>>>>>>> - Add comment for applying alignment to both device address
>>>>>>> and host
>>>>>>> address, and add NOTE for the alignment requirement of
>>>>>>> Translation,
>>>>>>> as well as in commit message [Laszlo][Ray]
>>>>>>> - Improve indention for the code in CreateRootBridge [Laszlo]
>>>>>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
>>>>>>> definition [Laszlo]
>>>>>>> - Ignore translation of bus in CreateRootBridge
>>>>>>>
>>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 2 +
>>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 14 +++
>>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 85
>>>>>>> +++++++++++---
>>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 122
>>>>>>> +++++++++++++++++---
>>>>>>> 4 files changed, 194 insertions(+), 29 deletions(-)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>>>> index 8612c0c3251b..662c2dd59529 100644
>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
>>>>>>> @@ -38,6 +38,8 @@ typedef enum {
>>>>>>> typedef struct {
>>>>>>> PCI_RESOURCE_TYPE Type;
>>>>>>> + // Base is a host address instead of a device address when
>>>>>>> address translation
>>>>>>> + // exists and host address != device address
>>>>>> (2). Coding style issue. The comments need to be in style like:
>>>>>> //[EMPTY]
>>>>>> // xxxx
>>>>>> //[EMPTY]
>>>>>> And how about just simply say Base is a host address. No matter
>>>>>> address
>>>>>> translation exists or not, Base is a host address actually.
>>>>>>
>>>>>>> UINT64 Base;
>>>>>>> UINT64 Length;
>>>>>>> UINT64 Alignment;
>>>>>>> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>>>>>> b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>>>>>> index d42e9ecdd763..c842a4152e85 100644
>>>>>>> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>>>>>> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>>>>>>> @@ -20,8 +20,22 @@
>>>>>>> // (Base > Limit) indicates an aperture is not available.
>>>>>>> //
>>>>>>> typedef struct {
>>>>>>> + // Base and Limit are the device address instead of host
>>>>>>> address when
>>>>>>> + // Translation is not zero
>>>>>> (3). Similar comments to (2).
>>>>>>> UINT64 Base;
>>>>>>> UINT64 Limit;
>>>>>>> + // According to UEFI 2.7, Device Address = Host Address +
>>>>>>> Translation,
>>>>>>> + // so Translation = Device Address - Host Address.
>>>>>>> + // On platforms where Translation is not zero, the subtraction
>>>>>>> is probably to
>>>>>>> + // be performed with UINT64 wrap-around semantics, for we may
>>>>>>> translate an
>>>>>>> + // above-4G host address into a below-4G device address for
>>>>>>> legacy PCIe device
>>>>>>> + // compatibility.
>>>>>>> + // NOTE: The alignment of Translation is required to be larger
>>>>>>> than any BAR
>>>>>>> + // alignment in the same root bridge, so that the same
>>>>>>> alignment can be
>>>>>>> + // applied to both device address and host address, which
>>>>>>> simplifies the
>>>>>>> + // situation and makes the current resource allocation code in
>>>>>>> generic PCI
>>>>>>> + // host bridge driver still work.
>>>>>> (4). Still I'd like to see the alignment requirement be reflected
>>>>>> in code
>>>>>> through assertion and if-check.
>>>>>>
>>>>>>> + UINT64 Translation;
>>>>>>> } PCI_ROOT_BRIDGE_APERTURE;
>>>>>>> typedef struct {
>>>>>>> diff --git
>>>>>>> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>> index 1494848c3e8c..1e65faee9084 100644
>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>>>>>>> @@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
>>>>>>> EFI_EVENT mIoMmuEvent;
>>>>>>> VOID *mIoMmuRegistration;
>>>>>>> +STATIC
>>>>>> (5). Can you remove STATIC? personal preference:)
>>>>>>> +UINT64
>>>>>>> +GetTranslationByResourceType (
>>>>>>> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
>>>>>>> + IN PCI_RESOURCE_TYPE ResourceType
>>>>>>> + )
>>>>>>> +{
>>>>>>> + switch (ResourceType) {
>>>>>>> + case TypeIo:
>>>>>>> + return RootBridge->Io.Translation;
>>>>>>> + case TypeMem32:
>>>>>>> + return RootBridge->Mem.Translation;
>>>>>>> + case TypePMem32:
>>>>>>> + return RootBridge->PMem.Translation;
>>>>>>> + case TypeMem64:
>>>>>>> + return RootBridge->MemAbove4G.Translation;
>>>>>>> + case TypePMem64:
>>>>>>> + return RootBridge->PMemAbove4G.Translation;
>>>>>>> + default:
>>>>>>> + ASSERT (FALSE);
>>>>>>> + return 0;
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>> /**
>>>>>>> Ensure the compatibility of an IO space descriptor with the
>>>>>>> IO aperture.
>>>>>>> @@ -366,6 +390,7 @@ InitializePciHostBridge (
>>>>>>> UINTN MemApertureIndex;
>>>>>>> BOOLEAN ResourceAssigned;
>>>>>>> LIST_ENTRY *Link;
>>>>>>> + UINT64 TempHostAddress;
>>>>>> (6). Just use name HostAddress?
>>>>>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
>>>>>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
>>>>>>> @@ -411,18 +436,24 @@ InitializePciHostBridge (
>>>>>>> }
>>>>>>> if (RootBridges[Index].Io.Base <=
>>>>>>> RootBridges[Index].Io.Limit) {
>>>>>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
>>>>>>> address.
>>>>>>> + // According to UEFI 2.7, device address = host address +
>>>>>>> Translation.
>>>>>>> + // For GCD resource manipulation, we should use host
>>>>>>> address, so
>>>>>>> + // Translation is subtracted from device address here.
>>>>>> (7). Please adjust the comment style to have two EMPTY line of
>>>>>> comment above
>>>>>> and below. Same to all comments in the patch.
>>>>>>> Status = AddIoSpace (
>>>>>>> - RootBridges[Index].Io.Base,
>>>>>>> + RootBridges[Index].Io.Base -
>>>>>>> RootBridges[Index].Io.Translation,
>>>>>>> RootBridges[Index].Io.Limit -
>>>>>>> RootBridges[Index].Io.Base + 1
>>>>>>> );
>>>>>>> ASSERT_EFI_ERROR (Status);
>>>>>>> if (ResourceAssigned) {
>>>>>>> + TempHostAddress = RootBridges[Index].Io.Base -
>>>>>>> + RootBridges[Index].Io.Translation;
>>>>>>> Status = gDS->AllocateIoSpace (
>>>>>>> EfiGcdAllocateAddress,
>>>>>>> EfiGcdIoTypeIo,
>>>>>>> 0,
>>>>>>> RootBridges[Index].Io.Limit -
>>>>>>> RootBridges[Index].Io.Base + 1,
>>>>>>> - &RootBridges[Index].Io.Base,
>>>>>>> + &TempHostAddress,
>>>>>>> gImageHandle,
>>>>>>> NULL
>>>>>>> );
>>>>>>> @@ -443,14 +474,18 @@ InitializePciHostBridge (
>>>>>>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE
>>>>>>> (MemApertures); MemApertureIndex++) {
>>>>>>> if (MemApertures[MemApertureIndex]->Base <=
>>>>>>> MemApertures[MemApertureIndex]->Limit) {
>>>>>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
>>>>>>> address.
>>>>>>> + // According to UEFI 2.7, device address = host address
>>>>>>> + Translation.
>>>>>>> + // For GCD resource manipulation, we should use host
>>>>>>> address, so
>>>>>>> + // Translation is subtracted from device address here.
>>>>>>> Status = AddMemoryMappedIoSpace (
>>>>>>> - MemApertures[MemApertureIndex]->Base,
>>>>>>> + MemApertures[MemApertureIndex]->Base -
>>>>>>> MemApertures[MemApertureIndex]->Translation,
>>>>>>> MemApertures[MemApertureIndex]->Limit -
>>>>>>> MemApertures[MemApertureIndex]->Base + 1,
>>>>>>> EFI_MEMORY_UC
>>>>>>> );
>>>>>>> ASSERT_EFI_ERROR (Status);
>>>>>>> Status = gDS->SetMemorySpaceAttributes (
>>>>>>> - MemApertures[MemApertureIndex]->Base,
>>>>>>> + MemApertures[MemApertureIndex]->Base -
>>>>>>> MemApertures[MemApertureIndex]->Translation,
>>>>>>> MemApertures[MemApertureIndex]->Limit -
>>>>>>> MemApertures[MemApertureIndex]->Base + 1,
>>>>>>> EFI_MEMORY_UC
>>>>>>> );
>>>>>>> @@ -458,12 +493,14 @@ InitializePciHostBridge (
>>>>>>> DEBUG ((DEBUG_WARN, "PciHostBridge driver failed to
>>>>>>> set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status));
>>>>>>> }
>>>>>>> if (ResourceAssigned) {
>>>>>>> + TempHostAddress = MemApertures[MemApertureIndex]->Base -
>>>>>>> + MemApertures[MemApertureIndex]->Translation;
>>>>>>> Status = gDS->AllocateMemorySpace (
>>>>>>> EfiGcdAllocateAddress,
>>>>>>> EfiGcdMemoryTypeMemoryMappedIo,
>>>>>>> 0,
>>>>>>> MemApertures[MemApertureIndex]->Limit
>>>>>>> - MemApertures[MemApertureIndex]->Base + 1,
>>>>>>> - &MemApertures[MemApertureIndex]->Base,
>>>>>>> + &TempHostAddress,
>>>>>>> gImageHandle,
>>>>>>> NULL
>>>>>>> );
>>>>>>> @@ -654,6 +691,11 @@ AllocateResource (
>>>>>>> if (BaseAddress < Limit) {
>>>>>>> //
>>>>>>> // Have to make sure Aligment is handled since we are doing
>>>>>>> direct address allocation
>>>>>>> + // Strictly speaking, alignment requirement should be
>>>>>>> applied to device
>>>>>>> + // address instead of host address which is used in GCD
>>>>>>> manipulation below,
>>>>>>> + // but as we restrict the alignment of Translation to be
>>>>>>> larger than any BAR
>>>>>>> + // alignment in the root bridge, we can simplify the
>>>>>>> situation and consider
>>>>>>> + // the same alignment requirement is also applied to host
>>>>>>> address.
>>>>>>> //
>>>>>>> BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1,
>>>>>>> BitsOfAlignment));
>>>>>>> @@ -824,12 +866,17 @@ NotifyPhase (
>>>>>>> switch (Index) {
>>>>>>> case TypeIo:
>>>>>>> + // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are
>>>>>>> device address.
>>>>>>> + // According to UEFI 2.7, device address = host
>>>>>>> address + Translation.
>>>>>>> + // For AllocateResource is manipulating GCD
>>>>>>> resource, we should use
>>>>>>> + // host address here, so Translation is subtracted
>>>>>>> from Base and
>>>>>>> + // Limit.
>>>>>>> BaseAddress = AllocateResource (
>>>>>>> FALSE,
>>>>>>>
>>>>>>> RootBridge->ResAllocNode[Index].Length,
>>>>>>> MIN (15, BitsOfAlignment),
>>>>>>> - ALIGN_VALUE (RootBridge->Io.Base,
>>>>>>> Alignment + 1),
>>>>>>> - RootBridge->Io.Limit
>>>>>>> + ALIGN_VALUE (RootBridge->Io.Base,
>>>>>>> Alignment + 1) - RootBridge->Io.Translation,
>>>>>>> + RootBridge->Io.Limit -
>>>>>>> RootBridge->Io.Translation
>>>>>>> );
>>>>>>> break;
>>>>>>> @@ -838,8 +885,8 @@ NotifyPhase (
>>>>>> (8). Add assertion and if-check here to make sure alignment of
>>>>>> Translation
>>>>>> is no less than the Alignment.
>>>>>>> TRUE,
>>>>>>>
>>>>>>> RootBridge->ResAllocNode[Index].Length,
>>>>>>> MIN (63, BitsOfAlignment),
>>>>>>> - ALIGN_VALUE
>>>>>>> (RootBridge->MemAbove4G.Base, Alignment + 1),
>>>>>>> - RootBridge->MemAbove4G.Limit
>>>>>>> + ALIGN_VALUE
>>>>>>> (RootBridge->MemAbove4G.Base, Alignment + 1) -
>>>>>>> RootBridge->MemAbove4G.Translation,
>>>>>>> + RootBridge->MemAbove4G.Limit -
>>>>>>> RootBridge->MemAbove4G.Translation
>>>>>>> );
>>>>>>> if (BaseAddress != MAX_UINT64) {
>>>>>>> break;
>>>>>>> @@ -853,8 +900,8 @@ NotifyPhase (
>>>>>>> TRUE,
>>>>>>>
>>>>>>> RootBridge->ResAllocNode[Index].Length,
>>>>>>> MIN (31, BitsOfAlignment),
>>>>>>> - ALIGN_VALUE (RootBridge->Mem.Base,
>>>>>>> Alignment + 1),
>>>>>>> - RootBridge->Mem.Limit
>>>>>>> + ALIGN_VALUE (RootBridge->Mem.Base,
>>>>>>> Alignment + 1) - RootBridge->Mem.Translation,
>>>>>>> + RootBridge->Mem.Limit -
>>>>>>> RootBridge->Mem.Translation
>>>>>>> );
>>>>>>> break;
>>>>>>> @@ -863,8 +910,8 @@ NotifyPhase (
>>>>>>> TRUE,
>>>>>>>
>>>>>>> RootBridge->ResAllocNode[Index].Length,
>>>>>>> MIN (63, BitsOfAlignment),
>>>>>>> - ALIGN_VALUE
>>>>>>> (RootBridge->PMemAbove4G.Base, Alignment + 1),
>>>>>>> - RootBridge->PMemAbove4G.Limit
>>>>>>> + ALIGN_VALUE
>>>>>>> (RootBridge->PMemAbove4G.Base, Alignment + 1) -
>>>>>>> RootBridge->PMemAbove4G.Translation,
>>>>>>> + RootBridge->PMemAbove4G.Limit -
>>>>>>> RootBridge->PMemAbove4G.Translation
>>>>>>> );
>>>>>>> if (BaseAddress != MAX_UINT64) {
>>>>>>> break;
>>>>>>> @@ -877,8 +924,8 @@ NotifyPhase (
>>>>>>> TRUE,
>>>>>>>
>>>>>>> RootBridge->ResAllocNode[Index].Length,
>>>>>>> MIN (31, BitsOfAlignment),
>>>>>>> - ALIGN_VALUE (RootBridge->PMem.Base,
>>>>>>> Alignment + 1),
>>>>>>> - RootBridge->PMem.Limit
>>>>>>> + ALIGN_VALUE (RootBridge->PMem.Base,
>>>>>>> Alignment + 1) - RootBridge->PMem.Translation,
>>>>>>> + RootBridge->PMem.Limit -
>>>>>>> RootBridge->PMem.Translation
>>>>>>> );
>>>>>>> break;
>>>>>>> @@ -1152,6 +1199,7 @@ StartBusEnumeration (
>>>>>>> Descriptor->AddrSpaceGranularity = 0;
>>>>>>> Descriptor->AddrRangeMin = RootBridge->Bus.Base;
>>>>>>> Descriptor->AddrRangeMax = 0;
>>>>>>> + // Ignore translation offset for bus
>>>>>> (9). Per PI spec on StartBusEnumeration, AddrRangeMax is ignored.
>>>>>> So I don't
>>>>>> think we need add comments here.
>>>>>>> Descriptor->AddrTranslationOffset = 0;
>>>>>>> Descriptor->AddrLen = RootBridge->Bus.Limit
>>>>>>> - RootBridge->Bus.Base + 1;
>>>>>>> @@ -1421,7 +1469,12 @@ GetProposedResources (
>>>>>>> Descriptor->Desc =
>>>>>>> ACPI_ADDRESS_SPACE_DESCRIPTOR;
>>>>>>> Descriptor->Len = sizeof
>>>>>>> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
>>>>>>> Descriptor->GenFlag = 0;
>>>>>>> - Descriptor->AddrRangeMin =
>>>>>>> RootBridge->ResAllocNode[Index].Base;
>>>>>>> + // AddrRangeMin in Resource Descriptor here should be
>>>>>>> device address
>>>>>>> + // instead of host address, or else PCI bus driver
>>>>>>> cannot set correct
>>>>>>> + // address into PCI BAR registers.
>>>>>>> + // Base in ResAllocNode is a host address, so
>>>>>>> Translation is added.
>>>>>>> + Descriptor->AddrRangeMin =
>>>>>>> RootBridge->ResAllocNode[Index].Base +
>>>>>>> + GetTranslationByResourceType (RootBridge, Index);
>>>>>> (10). Do you think defining a PciHostBridgeDxe internal macro
>>>>>> TO_HOST_ADDRESS(DeviceAddress, Offset) and
>>>>>> TO_DEVICE_ADDRESS(HostAddress,
>>>>>> Offset) is better?
>>>>>>
>>>>>>> Descriptor->AddrRangeMax = 0;
>>>>>>> Descriptor->AddrTranslationOffset = (ResStatus ==
>>>>>>> ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
>>>>>>> Descriptor->AddrLen =
>>>>>>> RootBridge->ResAllocNode[Index].Length;
>>>>>>> diff --git
>>>>>>> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>>>> index dc06c16dc038..edaa0d48a441 100644
>>>>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>>>>> @@ -86,12 +86,28 @@ CreateRootBridge (
>>>>>>> (Bridge->AllocationAttributes &
>>>>>>> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " :
>>>>>>> L"",
>>>>>>> (Bridge->AllocationAttributes &
>>>>>>> EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
>>>>>>> ));
>>>>>>> + // We don't see any scenario for bus translation, so
>>>>>>> translation for bus is just ignored.
>>>>>>> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n",
>>>>>>> Bridge->Bus.Base, Bridge->Bus.Limit));
>>>>>>> - DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n",
>>>>>>> Bridge->Io.Base, Bridge->Io.Limit));
>>>>>>> - DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n",
>>>>>>> Bridge->Mem.Base, Bridge->Mem.Limit));
>>>>>>> - DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n",
>>>>>>> Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
>>>>>>> - DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n",
>>>>>>> Bridge->PMem.Base, Bridge->PMem.Limit));
>>>>>>> - DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n",
>>>>>>> Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
>>>>>>> + DEBUG ((
>>>>>>> + DEBUG_INFO, " Io: %lx - %lx Translation=%lx\n",
>>>>>>> + Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
>>>>>>> + ));
>>>>>>> + DEBUG ((
>>>>>>> + DEBUG_INFO, " Mem: %lx - %lx Translation=%lx\n",
>>>>>>> + Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
>>>>>>> + ));
>>>>>>> + DEBUG ((
>>>>>>> + DEBUG_INFO, " MemAbove4G: %lx - %lx Translation=%lx\n",
>>>>>>> + Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit,
>>>>>>> Bridge->MemAbove4G.Translation
>>>>>>> + ));
>>>>>>> + DEBUG ((
>>>>>>> + DEBUG_INFO, " PMem: %lx - %lx Translation=%lx\n",
>>>>>>> + Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
>>>>>>> + ));
>>>>>>> + DEBUG ((
>>>>>>> + DEBUG_INFO, " PMemAbove4G: %lx - %lx Translation=%lx\n",
>>>>>>> + Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit,
>>>>>>> Bridge->PMemAbove4G.Translation
>>>>>>> + ));
>>>>>>> //
>>>>>>> // Make sure Mem and MemAbove4G apertures are valid
>>>>>>> @@ -206,7 +222,15 @@ CreateRootBridge (
>>>>>>> }
>>>>>>> RootBridge->ResAllocNode[Index].Type = Index;
>>>>>>> if (Bridge->ResourceAssigned && (Aperture->Limit >=
>>>>>>> Aperture->Base)) {
>>>>>>> - RootBridge->ResAllocNode[Index].Base = Aperture->Base;
>>>>>>> + // Ignore translation for bus
>>>>>> (11). How about we only make sure the TranslationOffset is 0 for
>>>>>> TypeBus
>>>>>> when getting the aperture from PciHostBridgeLib.
>>>>>> So that later logic doesn't need to do special handling for the
>>>>>> TypeBus.
>>>>>> This way the code can be simpler.
>>>>>>> + if (Index == TypeBus) {
>>>>>>> + RootBridge->ResAllocNode[Index].Base = Aperture->Base;
>>>>>>> + } else {
>>>>>>> + // Base in ResAllocNode is a host address, while Base in
>>>>>>> Aperture is a
>>>>>>> + // device address, so translation needs to be subtracted.
>>>>>>> + RootBridge->ResAllocNode[Index].Base = Aperture->Base -
>>>>>>> + Aperture->Translation;
>>>>>> (12). Still. Do you think using TO_HOST_ADDRESS() is better?
>>>>>>> + }
>>>>>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit
>>>>>>> - Aperture->Base + 1;
>>>>>>> RootBridge->ResAllocNode[Index].Status = ResAllocated;
>>>>>>> } else {
>>>>>>> @@ -403,6 +427,28 @@ RootBridgeIoCheckParameter (
>>>>>>> return EFI_SUCCESS;
>>>>>>> }
>>>>>>> +EFI_STATUS
>>>>>>> +RootBridgeIoGetMemTranslationByAddress (
>>>>>>> + IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
>>>>>>> + IN UINT64 Address,
>>>>>>> + IN OUT UINT64 *Translation
>>>>>>> + )
>>>>>>> +{
>>>>>>> + if (Address >= RootBridge->Mem.Base && Address <=
>>>>>>> RootBridge->Mem.Limit) {
>>>>>>> + *Translation = RootBridge->Mem.Translation;
>>>>>>> + } else if (Address >= RootBridge->PMem.Base && Address <=
>>>>>>> RootBridge->PMem.Limit) {
>>>>>>> + *Translation = RootBridge->PMem.Translation;
>>>>>>> + } else if (Address >= RootBridge->MemAbove4G.Base && Address
>>>>>>> <= RootBridge->MemAbove4G.Limit) {
>>>>>>> + *Translation = RootBridge->MemAbove4G.Translation;
>>>>>>> + } else if (Address >= RootBridge->PMemAbove4G.Base && Address
>>>>>>> <= RootBridge->PMemAbove4G.Limit) {
>>>>>>> + *Translation = RootBridge->PMemAbove4G.Translation;
>>>>>>> + } else {
>>>>>>> + return EFI_INVALID_PARAMETER;
>>>>>>> + }
>>>>>>> +
>>>>>>> + return EFI_SUCCESS;
>>>>>>> +}
>>>>>>> +
>>>>>>> /**
>>>>>>> Polls an address in memory mapped I/O space until an exit
>>>>>>> condition is met,
>>>>>>> or a timeout occurs.
>>>>>>> @@ -658,13 +704,24 @@ RootBridgeIoMemRead (
>>>>>>> )
>>>>>>> {
>>>>>>> EFI_STATUS Status;
>>>>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>> + UINT64 Translation;
>>>>>>> Status = RootBridgeIoCheckParameter (This, MemOperation,
>>>>>>> Width, Address,
>>>>>>> Count, Buffer);
>>>>>>> if (EFI_ERROR (Status)) {
>>>>>>> return Status;
>>>>>>> }
>>>>>>> - return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address, Count, Buffer);
>>>>>>> +
>>>>>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>>>>>> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge,
>>>>>>> Address, &Translation);
>>>>>> (13). Why do you think the Address is a Device Address?
>>>>>> PciIo.GetBarAttributes() returns a HOST Address and the
>>>>>> Translation Offset.
>>>>>> I didn't see you change the PciIoMemRead() implementation.
>>>>>> So it means the same HOST Address is passed into the this function.
>>>>>>
>>>>>>> + if (EFI_ERROR (Status)) {
>>>>>>> + return Status;
>>>>>>> + }
>>>>>>> +
>>>>>>> + // Address passed to CpuIo->Mem.Read should be a host address
>>>>>>> instead of
>>>>>>> + // device address, so Translation should be subtracted.
>>>>>>> + return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address - Translation, Count, Buffer);
>>>>>>> }
>>>>>>> /**
>>>>>>> @@ -705,13 +762,24 @@ RootBridgeIoMemWrite (
>>>>>>> )
>>>>>>> {
>>>>>>> EFI_STATUS Status;
>>>>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>> + UINT64 Translation;
>>>>>>> Status = RootBridgeIoCheckParameter (This, MemOperation,
>>>>>>> Width, Address,
>>>>>>> Count, Buffer);
>>>>>>> if (EFI_ERROR (Status)) {
>>>>>>> return Status;
>>>>>>> }
>>>>>>> - return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address, Count, Buffer);
>>>>>>> +
>>>>>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>>>>>> + Status = RootBridgeIoGetMemTranslationByAddress (RootBridge,
>>>>>>> Address, &Translation);
>>>>>>> + if (EFI_ERROR (Status)) {
>>>>>>> + return Status;
>>>>>>> + }
>>>>>>> +
>>>>>>> + // Address passed to CpuIo->Mem.Write should be a host address
>>>>>>> instead of
>>>>>>> + // device address, so Translation should be subtracted.
>>>>>>> + return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address - Translation, Count, Buffer);
>>>>>> (14). Same as (13)
>>>>>>> }
>>>>>>> /**
>>>>>>> @@ -746,6 +814,8 @@ RootBridgeIoIoRead (
>>>>>>> )
>>>>>>> {
>>>>>>> EFI_STATUS Status;
>>>>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>> +
>>>>>>> Status = RootBridgeIoCheckParameter (
>>>>>>> This, IoOperation, Width,
>>>>>>> Address, Count, Buffer
>>>>>>> @@ -753,7 +823,12 @@ RootBridgeIoIoRead (
>>>>>>> if (EFI_ERROR (Status)) {
>>>>>>> return Status;
>>>>>>> }
>>>>>>> - return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address, Count, Buffer);
>>>>>>> +
>>>>>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>>>>>> +
>>>>>>> + // Address passed to CpuIo->Io.Read should be a host address
>>>>>>> instead of
>>>>>>> + // device address, so Translation should be subtracted.
>>>>>>> + return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address - RootBridge->Io.Translation, Count, Buffer);
>>>>>> (15). Same as (13)
>>>>>>> }
>>>>>>> /**
>>>>>>> @@ -788,6 +863,8 @@ RootBridgeIoIoWrite (
>>>>>>> )
>>>>>>> {
>>>>>>> EFI_STATUS Status;
>>>>>>> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
>>>>>>> +
>>>>>>> Status = RootBridgeIoCheckParameter (
>>>>>>> This, IoOperation, Width,
>>>>>>> Address, Count, Buffer
>>>>>>> @@ -795,7 +872,12 @@ RootBridgeIoIoWrite (
>>>>>>> if (EFI_ERROR (Status)) {
>>>>>>> return Status;
>>>>>>> }
>>>>>>> - return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address, Count, Buffer);
>>>>>>> +
>>>>>>> + RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>>>>>> +
>>>>>>> + // Address passed to CpuIo->Io.Write should be a host address
>>>>>>> instead of
>>>>>>> + // device address, so Translation should be subtracted.
>>>>>>> + return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
>>>>>>> Width, Address - RootBridge->Io.Translation, Count, Buffer);
>>>>>> (16). Same as (13)
>>>>>>> }
>>>>>>> /**
>>>>>>> @@ -1615,25 +1697,39 @@ RootBridgeIoConfiguration (
>>>>>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
>>>>>>> Descriptor->Len = sizeof
>>>>>>> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
>>>>>>> + // According to UEFI 2.7, RootBridgeIo->Configuration should
>>>>>>> return address
>>>>>>> + // range in CPU view (host address), and ResAllocNode->Base
>>>>>>> is already a CPU
>>>>>>> + // view address (host address).
>>>>>>> Descriptor->AddrRangeMin = ResAllocNode->Base;
>>>>>>> Descriptor->AddrRangeMax = ResAllocNode->Base +
>>>>>>> ResAllocNode->Length - 1;
>>>>>>> Descriptor->AddrLen = ResAllocNode->Length;
>>>>>>> switch (ResAllocNode->Type) {
>>>>>>> case TypeIo:
>>>>>>> - Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
>>>>>>> + Descriptor->ResType =
>>>>>>> ACPI_ADDRESS_SPACE_TYPE_IO;
>>>>>>> + Descriptor->AddrTranslationOffset =
>>>>>>> RootBridge->Io.Translation;
>>>>>> (17). Can GetTranslationByResourceType() be used to simplify the
>>>>>> code?
>>>>>>> break;
>>>>>>> case TypePMem32:
>>>>>>> - Descriptor->SpecificFlag =
>>>>>>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
>>>>>>> + Descriptor->SpecificFlag =
>>>>>>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
>>>>>>> + Descriptor->AddrTranslationOffset =
>>>>>>> RootBridge->PMem.Translation;
>>>>>>> + Descriptor->ResType =
>>>>>>> ACPI_ADDRESS_SPACE_TYPE_MEM;
>>>>>>> + Descriptor->AddrSpaceGranularity = 32;
>>>>>>> + break;
>>>>>>> +
>>>>>>> case TypeMem32:
>>>>>>> + Descriptor->AddrTranslationOffset =
>>>>>>> RootBridge->Mem.Translation;
>>>>>>> Descriptor->ResType =
>>>>>>> ACPI_ADDRESS_SPACE_TYPE_MEM;
>>>>>>> Descriptor->AddrSpaceGranularity = 32;
>>>>>>> break;
>>>>>>> case TypePMem64:
>>>>>>> - Descriptor->SpecificFlag =
>>>>>>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
>>>>>>> + Descriptor->SpecificFlag =
>>>>>>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
>>>>>>> + Descriptor->AddrTranslationOffset =
>>>>>>> RootBridge->PMemAbove4G.Translation;
>>>>>>> + Descriptor->ResType =
>>>>>>> ACPI_ADDRESS_SPACE_TYPE_MEM;
>>>>>>> + Descriptor->AddrSpaceGranularity = 64;
>>>>>>> case TypeMem64:
>>>>>>> + Descriptor->AddrTranslationOffset =
>>>>>>> RootBridge->MemAbove4G.Translation;
>>>>>>> Descriptor->ResType =
>>>>>>> ACPI_ADDRESS_SPACE_TYPE_MEM;
>>>>>>> Descriptor->AddrSpaceGranularity = 64;
>>>>>>> break;
>>>>>>>
>>>>>>
>>>>>> (18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py on
>>>>>> the
>>>>>> PciHostBridgeDxe driver to make sure coding style matches the
>>>>>> requirement.
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Ray
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Ray
>
>
Heyi,
My understanding is this whole change is backward compatible.
Which means an old version of PciHostBridgeLib + new PciHostBridgeLib.h
+ new PciHostBridgeDxe driver won't cause build failure.
right?
--
Thanks,
Ray
next prev parent reply other threads:[~2018-02-28 7:18 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-27 2:09 [RFC v4 0/3] Add translation support to generic PciHostBridge Heyi Guo
2018-02-27 2:09 ` [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-27 8:48 ` Ni, Ruiyu
2018-02-27 8:55 ` Ard Biesheuvel
2018-02-27 9:33 ` Guo Heyi
2018-02-27 9:59 ` Ni, Ruiyu
2018-02-27 10:14 ` Ard Biesheuvel
2018-02-27 10:45 ` Guo Heyi
2018-02-27 11:44 ` Guo Heyi
2018-02-28 2:29 ` Ni, Ruiyu
2018-02-28 7:25 ` Ni, Ruiyu [this message]
2018-02-28 7:53 ` Guo Heyi
2018-02-28 9:39 ` Laszlo Ersek
2018-02-28 23:31 ` Guo Heyi
2018-03-01 3:56 ` Guo Heyi
2018-03-01 4:44 ` Ni, Ruiyu
2018-02-27 2:09 ` [RFC v4 2/3] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-02-27 2:09 ` [RFC v4 3/3] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox