Re: [edk2] [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Laszlo Ersek]

On 12/02/16 12:03, Leif Lindholm wrote:
> On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
>> where "QemuFwCfgDma.h" was added in the previous patch.
>>
>> Cc: Ard Biesheuvel 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
> 
> This is nice cleanup.
> One bit of bikeshedding below, address or ignore - regardless:
> Reviewed-by: Leif Lindholm 
> 
>> ---
>>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-
>>  1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
>> b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> index 2fd8d9050566..62a85dff328e 100644
>> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> @@ -25,6 +25,8 @@
>>  
>>  #include 
>>  
>> +#include 
>> +
> 
> So, I forget if we have official guidelines on this, but instinctively
> I would put IndustryStandard before Library (alphabetically).

Good point. But, in the next version, this #include directive will go
away anyway, because this file already includes
, and that header file will get the new macros
in v2 (based on Jordan's feedback).

Thanks!
Laszlo

> 
> Regards,
> 
> Leif
> 
>>  STATIC UINTN mFwCfgSelectorAddress;
>>  STATIC UINTN mFwCfgDataAddress;
>>  STATIC UINTN mFwCfgDmaAddress;
>> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>>  //
>>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>>  
>> -//
>> -// Communication structure for DmaReadBytes(). All fields are encoded in big
>> -// endian.
>> -//
>> -#pragma pack (1)
>> -typedef struct {
>> -  UINT32 Control;
>> -  UINT32 Length;
>> -  UINT64 Address;
>> -} FW_CFG_DMA_ACCESS;
>> -#pragma pack ()
>> -
>> -//
>> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
>> -//
>> -#define FW_CFG_DMA_CTL_ERROR  BIT0
>> -#define FW_CFG_DMA_CTL_READ   BIT1
>> -#define FW_CFG_DMA_CTL_SKIP   BIT2
>> -#define FW_CFG_DMA_CTL_SELECT BIT3
>> -
>>  
>>  /**
>>Returns a boolean indicating if the firmware configuration interface
>> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>>  
>>  QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>>  Features = QemuFwCfgRead32 ();
>> -if ((Features & BIT1) != 0) {
>> +if ((Features & FW_CFG_F_DMA) != 0) {
>>mFwCfgDmaAddress = FwCfgDmaAddress;
>>InternalQemuFwCfgReadBytes = DmaReadBytes;
>>  }
>> -- 
>> 2.9.2
>>
>>
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

Re: [edk2] [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Leif Lindholm]

On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
> where "QemuFwCfgDma.h" was added in the previous patch.
> 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 

This is nice cleanup.
One bit of bikeshedding below, address or ignore - regardless:
Reviewed-by: Leif Lindholm 

> ---
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
> b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 2fd8d9050566..62a85dff328e 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -25,6 +25,8 @@
>  
>  #include 
>  
> +#include 
> +

So, I forget if we have official guidelines on this, but instinctively
I would put IndustryStandard before Library (alphabetically).

Regards,

Leif

>  STATIC UINTN mFwCfgSelectorAddress;
>  STATIC UINTN mFwCfgDataAddress;
>  STATIC UINTN mFwCfgDmaAddress;
> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>  //
>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>  
> -//
> -// Communication structure for DmaReadBytes(). All fields are encoded in big
> -// endian.
> -//
> -#pragma pack (1)
> -typedef struct {
> -  UINT32 Control;
> -  UINT32 Length;
> -  UINT64 Address;
> -} FW_CFG_DMA_ACCESS;
> -#pragma pack ()
> -
> -//
> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
> -//
> -#define FW_CFG_DMA_CTL_ERROR  BIT0
> -#define FW_CFG_DMA_CTL_READ   BIT1
> -#define FW_CFG_DMA_CTL_SKIP   BIT2
> -#define FW_CFG_DMA_CTL_SELECT BIT3
> -
>  
>  /**
>Returns a boolean indicating if the firmware configuration interface
> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>  
>  QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>  Features = QemuFwCfgRead32 ();
> -if ((Features & BIT1) != 0) {
> +if ((Features & FW_CFG_F_DMA) != 0) {
>mFwCfgDmaAddress = FwCfgDmaAddress;
>InternalQemuFwCfgReadBytes = DmaReadBytes;
>  }
> -- 
> 2.9.2
> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Laszlo Ersek]

where "QemuFwCfgDma.h" was added in the previous patch.

Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 2fd8d9050566..62a85dff328e 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -25,6 +25,8 @@
 
 #include 
 
+#include 
+
 STATIC UINTN mFwCfgSelectorAddress;
 STATIC UINTN mFwCfgDataAddress;
 STATIC UINTN mFwCfgDmaAddress;
@@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
 //
 STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
 
-//
-// Communication structure for DmaReadBytes(). All fields are encoded in big
-// endian.
-//
-#pragma pack (1)
-typedef struct {
-  UINT32 Control;
-  UINT32 Length;
-  UINT64 Address;
-} FW_CFG_DMA_ACCESS;
-#pragma pack ()
-
-//
-// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
-//
-#define FW_CFG_DMA_CTL_ERROR  BIT0
-#define FW_CFG_DMA_CTL_READ   BIT1
-#define FW_CFG_DMA_CTL_SKIP   BIT2
-#define FW_CFG_DMA_CTL_SELECT BIT3
-
 
 /**
   Returns a boolean indicating if the firmware configuration interface
@@ -183,7 +165,7 @@ QemuFwCfgInitialize (
 
 QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
 Features = QemuFwCfgRead32 ();
-if ((Features & BIT1) != 0) {
+if ((Features & FW_CFG_F_DMA) != 0) {
   mFwCfgDmaAddress = FwCfgDmaAddress;
   InternalQemuFwCfgReadBytes = DmaReadBytes;
 }
-- 
2.9.2


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