The current code have use powerof2_roundup to get the 2^n, there was an
issue if used to get the 2^n of a macro, that's because macros have no
data type.

So this patch split the powerof2_roundup to powerof2_roundup32 and
powerof2_roundup64 for different case.

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/include/arch/x86/asm/vtd.h | 2 +-
hypervisor/include/lib/util.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hypervisor/include/arch/x86/asm/vtd.h b/hypervisor/include/arch/x86/asm/vtd.h
index 4d3518286..4df5b264b 100644
--- a/hypervisor/include/arch/x86/asm/vtd.h
+++ b/hypervisor/include/arch/x86/asm/vtd.h
@@ -46,7 +46,7 @@
#if (CONFIG_MAX_PT_IRQ_ENTRIES <= 256)
#define MAX_IR_ENTRIES 256
#else
-#define MAX_IR_ENTRIES powerof2_roundup(CONFIG_MAX_PT_IRQ_ENTRIES)
+#define MAX_IR_ENTRIES powerof2_roundup32(CONFIG_MAX_PT_IRQ_ENTRIES)
#endif

/* Values for entry_type in ACPI_DMAR_DEVICE_SCOPE - device types */
diff --git a/hypervisor/include/lib/util.h b/hypervisor/include/lib/util.h
index ca3296556..2e2a6c6df 100644
--- a/hypervisor/include/lib/util.h
+++ b/hypervisor/include/lib/util.h
@@ -44,7 +44,8 @@
#define ROUND8(x) (ROUND4(x) |(ROUND4(x)>>8))
#define ROUND16(x) (ROUND8(x) |(ROUND8(x)>>16))
#define ROUND32(x) (ROUND16(x) |(ROUND16(x)>>32))
-#define powerof2_roundup(x) ((ROUND32(x) == (~0UL)) ? ~0UL : (ROUND32(x) +1))
+#define powerof2_roundup32(x) ((ROUND16(x) == (~0UL)) ? ~0UL : (ROUND16(x) +1))
+#define powerof2_roundup64(x) ((ROUND32(x) == (~0UL)) ? ~0UL : (ROUND32(x) +1))

/* Macro used to check if a value is aligned to the required boundary.
* Returns TRUE if aligned; FALSE if not aligned
--
2.25.1

On Sat, May 07, 2022 at 06:09:02AM +0800, Dong, Eddie wrote:

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of chenli.wei
Sent: Friday, May 6, 2022 1:23 AM
To: Li, Fei1 <fei1.li@...>; acrn-dev@...
Cc: Wei, Chenli <chenli.wei@...>
Subject: [acrn-dev] [PATCH] hv: fix the issue of "right shift count"

The current code have use powerof2_roundup to get the 2^n, there was an
issue if used to get the 2^n of a macro, that's because macros have no data
type.

Didn't quite understand.


BTW, the current logic of powerof2_roundup() is very ugly, and I am wondering why we need the following logic:

#if (CONFIG_MAX_PT_IRQ_ENTRIES <= 256)
#define MAX_IR_ENTRIES 256
#else
#define MAX_IR_ENTRIES powerof2_roundup(CONFIG_MAX_PT_IRQ_ENTRIES)
#endif

Interrupt Remapping Table Address Register (IRTA_REG)

63:12 : Interrupt Remapping Table Address
This field points to the base of 4KB aligned interrupt remapping table.
Hardware may ignore and not implement bits 63:HAW, where HAW is the host
address width.

3:0 : Size
This field specifies the size of the interrupt remapping table. The
number of entries in the interrupt remapping table is 2 ^ X+1, where X is
the value programmed in this field.

And an Interrupt Remapping Table Entry (IRTE) takes 8 Bytes. So a 4KB page
would cover 256 Interrupt Remapping Table Entries (interrupt remapping table
takes a page at least).

Can we assume CONFIG_MAX_PT_IRQ_ENTRIES be in the power of 2 ? If yes, we can define both PT_IRQ_ENTRY_BITS, and MAX_PT_IRQ_ENTRIES= (1U << PT_IRQ_ENTRY_BITS). And refuse to boot if it is not.4
This way, it can be simplified.

Yes, but PT_IRQ_ENTRY_BITS maybe is not friendly to user, I think.

Could we still let user to configure CONFIG_MAX_PT_IRQ_ENTRIES, but let
config tool to help ACRN Hypervisor to do align check and calculate
the PT_IRQ_ENTRY_BITS ?

So this patch split the powerof2_roundup to powerof2_roundup32 and
powerof2_roundup64 for different case.

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/include/arch/x86/asm/vtd.h | 2 +-
hypervisor/include/lib/util.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hypervisor/include/arch/x86/asm/vtd.h
b/hypervisor/include/arch/x86/asm/vtd.h
index 4d3518286..4df5b264b 100644
--- a/hypervisor/include/arch/x86/asm/vtd.h
+++ b/hypervisor/include/arch/x86/asm/vtd.h
@@ -46,7 +46,7 @@
#if (CONFIG_MAX_PT_IRQ_ENTRIES <= 256)
#define MAX_IR_ENTRIES 256
#else
-#define MAX_IR_ENTRIES
powerof2_roundup(CONFIG_MAX_PT_IRQ_ENTRIES)
+#define MAX_IR_ENTRIES
powerof2_roundup32(CONFIG_MAX_PT_IRQ_ENTRIES)
#endif

/* Values for entry_type in ACPI_DMAR_DEVICE_SCOPE - device types */
diff --git a/hypervisor/include/lib/util.h b/hypervisor/include/lib/util.h index
ca3296556..2e2a6c6df 100644
--- a/hypervisor/include/lib/util.h
+++ b/hypervisor/include/lib/util.h
@@ -44,7 +44,8 @@
#define ROUND8(x) (ROUND4(x) |(ROUND4(x)>>8)) #define ROUND16(x)
(ROUND8(x) |(ROUND8(x)>>16)) #define ROUND32(x) (ROUND16(x)
|(ROUND16(x)>>32)) -#define powerof2_roundup(x) ((ROUND32(x) ==
(~0UL)) ? ~0UL : (ROUND32(x) +1))
+#define powerof2_roundup32(x) ((ROUND16(x) == (~0UL)) ? ~0UL :
+(ROUND16(x) +1)) #define powerof2_roundup64(x) ((ROUND32(x) == (~0UL))
+? ~0UL : (ROUND32(x) +1))

/* Macro used to check if a value is aligned to the required boundary.
* Returns TRUE if aligned; FALSE if not aligned
--
2.25.1

On 5/7/2022 11:17 AM, Li, Fei1 wrote:

On Sat, May 07, 2022 at 06:09:02AM +0800, Dong, Eddie wrote:

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of chenli.wei
Sent: Friday, May 6, 2022 1:23 AM
To: Li, Fei1 <fei1.li@...>; acrn-dev@...
Cc: Wei, Chenli <chenli.wei@...>
Subject: [acrn-dev] [PATCH] hv: fix the issue of "right shift count"

The current code have use powerof2_roundup to get the 2^n, there was an
issue if used to get the 2^n of a macro, that's because macros have no data
type.

Didn't quite understand.


BTW, the current logic of powerof2_roundup() is very ugly, and I am wondering why we need the following logic:

#if (CONFIG_MAX_PT_IRQ_ENTRIES <= 256)
#define MAX_IR_ENTRIES 256
#else
#define MAX_IR_ENTRIES powerof2_roundup(CONFIG_MAX_PT_IRQ_ENTRIES)
#endif

Interrupt Remapping Table Address Register (IRTA_REG)

63:12 : Interrupt Remapping Table Address
This field points to the base of 4KB aligned interrupt remapping table.
Hardware may ignore and not implement bits 63:HAW, where HAW is the host
address width.

3:0 : Size
This field specifies the size of the interrupt remapping table. The
number of entries in the interrupt remapping table is 2 ^ X+1, where X is
the value programmed in this field.

And an Interrupt Remapping Table Entry (IRTE) takes 8 Bytes. So a 4KB page
would cover 256 Interrupt Remapping Table Entries (interrupt remapping table
takes a page at least).

Can we assume CONFIG_MAX_PT_IRQ_ENTRIES be in the power of 2 ? If yes, we can define both PT_IRQ_ENTRY_BITS, and MAX_PT_IRQ_ENTRIES= (1U << PT_IRQ_ENTRY_BITS). And refuse to boot if it is not.4
This way, it can be simplified.

Yes, but PT_IRQ_ENTRY_BITS maybe is not friendly to user, I think.

Could we still let user to configure CONFIG_MAX_PT_IRQ_ENTRIES, but let
config tool to help ACRN Hypervisor to do align check and calculate
the PT_IRQ_ENTRY_BITS ?

Config tool could count the PT_IRQ_ENTRY_BITS by CONFIG_MAX_PT_IRQ_ENTRIE.

This way, we could fix the issue and simple HV code.

So this patch split the powerof2_roundup to powerof2_roundup32 and
powerof2_roundup64 for different case.

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/include/arch/x86/asm/vtd.h | 2 +-
hypervisor/include/lib/util.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hypervisor/include/arch/x86/asm/vtd.h
b/hypervisor/include/arch/x86/asm/vtd.h
index 4d3518286..4df5b264b 100644
--- a/hypervisor/include/arch/x86/asm/vtd.h
+++ b/hypervisor/include/arch/x86/asm/vtd.h
@@ -46,7 +46,7 @@
#if (CONFIG_MAX_PT_IRQ_ENTRIES <= 256)
#define MAX_IR_ENTRIES 256
#else
-#define MAX_IR_ENTRIES
powerof2_roundup(CONFIG_MAX_PT_IRQ_ENTRIES)
+#define MAX_IR_ENTRIES
powerof2_roundup32(CONFIG_MAX_PT_IRQ_ENTRIES)
#endif

/* Values for entry_type in ACPI_DMAR_DEVICE_SCOPE - device types */
diff --git a/hypervisor/include/lib/util.h b/hypervisor/include/lib/util.h index
ca3296556..2e2a6c6df 100644
--- a/hypervisor/include/lib/util.h
+++ b/hypervisor/include/lib/util.h
@@ -44,7 +44,8 @@
#define ROUND8(x) (ROUND4(x) |(ROUND4(x)>>8)) #define ROUND16(x)
(ROUND8(x) |(ROUND8(x)>>16)) #define ROUND32(x) (ROUND16(x)
|(ROUND16(x)>>32)) -#define powerof2_roundup(x) ((ROUND32(x) ==
(~0UL)) ? ~0UL : (ROUND32(x) +1))
+#define powerof2_roundup32(x) ((ROUND16(x) == (~0UL)) ? ~0UL :
+(ROUND16(x) +1)) #define powerof2_roundup64(x) ((ROUND32(x) == (~0UL))
+? ~0UL : (ROUND32(x) +1))

/* Macro used to check if a value is aligned to the required boundary.
* Returns TRUE if aligned; FALSE if not aligned
--
2.25.1

Can we assume CONFIG_MAX_PT_IRQ_ENTRIES be in the power of 2 ? If
yes, we can define both PT_IRQ_ENTRY_BITS, and

MAX_PT_IRQ_ENTRIES= (1U << PT_IRQ_ENTRY_BITS). And refuse to boot if
it is not.4 This way, it can be simplified.

Yes, but PT_IRQ_ENTRY_BITS maybe is not friendly to user, I think.

Good question. If offline tool can calculate automatically, we should persuade this, and avoid user configuration.

If not, Offline tool need to provide a user friendly interface to configure.
Some thing like to let user choose: Standard number of IO, Medium number of IO, and large number of IO.
Each of the item maps to 256, 512, 1024 corresponsive.

Could we still let user to configure CONFIG_MAX_PT_IRQ_ENTRIES, but let
config tool to help ACRN Hypervisor to do align check and calculate the
PT_IRQ_ENTRY_BITS ?

Yes

On 5/10/2022 1:01 AM, Eddie Dong wrote:

Can we assume CONFIG_MAX_PT_IRQ_ENTRIES be in the power of 2 ? If
yes, we can define both PT_IRQ_ENTRY_BITS, and

MAX_PT_IRQ_ENTRIES= (1U << PT_IRQ_ENTRY_BITS). And refuse to boot if
it is not.4 This way, it can be simplified.
Yes, but PT_IRQ_ENTRY_BITS maybe is not friendly to user, I think.

Good question. If offline tool can calculate automatically, we should persuade this, and avoid user configuration.

If not, Offline tool need to provide a user friendly interface to configure.
Some thing like to let user choose: Standard number of IO, Medium number of IO, and large number of IO.
Each of the item maps to 256, 512, 1024 corresponsive.

Offline tool could get the roundup power of 2 and define the #.

I will fix it and send another patch.

Could we still let user to configure CONFIG_MAX_PT_IRQ_ENTRIES, but let
config tool to help ACRN Hypervisor to do align check and calculate the
PT_IRQ_ENTRY_BITS ?

Yes

Done