[PATCH v6 4/4] hv: refine the define of *PIO_IDX


chenli.wei
 

The current code define *PIO_IDX by macro, it's not a good style and
the UART PIO was in the middle of these macro which should move to the
end to conveniently modify.

This patch change these macro to enum and move the UART PIO to the end
of these PIO_IDX list.

v5-->v6:
1.code format

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/arch/x86/guest/pm.c | 2 +-
hypervisor/dm/io_req.c | 2 +-
.../include/arch/x86/asm/guest/vmx_io.h | 38 ++++++++++---------
3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 2af0dd259..a7f71fca7 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -249,7 +249,7 @@ static bool pm1ab_io_write(struct acrn_vcpu *vcpu, uint16_t addr, size_t width,
return true;
}

-static void register_gas_io_handler(struct acrn_vm *vm, uint32_t pio_idx, const struct acrn_acpi_generic_address *gas)
+static void register_gas_io_handler(struct acrn_vm *vm, enum port_io_index pio_idx, const struct acrn_acpi_generic_address *gas)
{
uint8_t io_len[5] = {0U, 1U, 2U, 4U, 8U};
struct vm_io_range gas_io;
diff --git a/hypervisor/dm/io_req.c b/hypervisor/dm/io_req.c
index b68a66cab..fbc38cf46 100644
--- a/hypervisor/dm/io_req.c
+++ b/hypervisor/dm/io_req.c
@@ -597,7 +597,7 @@ emulate_io(struct acrn_vcpu *vcpu, struct io_request *io_req)
* @param io_write_fn_ptr The handler for emulating writes to the given range
* @pre pio_idx < EMUL_PIO_IDX_MAX
*/
-void register_pio_emulation_handler(struct acrn_vm *vm, uint32_t pio_idx,
+void register_pio_emulation_handler(struct acrn_vm *vm, enum port_io_index pio_idx,
const struct vm_io_range *range, io_read_fn_t io_read_fn_ptr, io_write_fn_t io_write_fn_ptr)
{
if (is_service_vm(vm)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vmx_io.h b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
index 5c41b4d4c..3ddb46fee 100644
--- a/hypervisor/include/arch/x86/asm/guest/vmx_io.h
+++ b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
@@ -9,25 +9,27 @@

#include <types.h>

+enum port_io_index {
/* Define emulated port IO index */
-#define PIC_PRIMARY_PIO_IDX 0U
-#define PIC_SECONDARY_PIO_IDX (PIC_PRIMARY_PIO_IDX + 1U)
-#define PIC_ELC_PIO_IDX (PIC_SECONDARY_PIO_IDX + 1U)
-#define PCI_CFGADDR_PIO_IDX (PIC_ELC_PIO_IDX + 1U)
-#define PCI_CFGDATA_PIO_IDX (PCI_CFGADDR_PIO_IDX + 1U)
-/* MAX_VUART_NUM_PER_VM is 8, so allocate UART_PIO_IDX0~UART_PIO_IDX0 + 7 for 8 vuart */
-#define UART_PIO_IDX0 (PCI_CFGDATA_PIO_IDX + 1U)
-#define PM1A_EVT_PIO_IDX (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM)
-#define PM1A_CNT_PIO_IDX (PM1A_EVT_PIO_IDX + 1U)
-#define PM1B_EVT_PIO_IDX (PM1A_CNT_PIO_IDX + 1U)
-#define PM1B_CNT_PIO_IDX (PM1B_EVT_PIO_IDX + 1U)
-#define RTC_PIO_IDX (PM1B_CNT_PIO_IDX + 1U)
-#define VIRTUAL_PM1A_CNT_PIO_IDX (RTC_PIO_IDX + 1U)
-#define KB_PIO_IDX (VIRTUAL_PM1A_CNT_PIO_IDX + 1U)
-#define CF9_PIO_IDX (KB_PIO_IDX + 1U)
-#define PIO_RESET_REG_IDX (CF9_PIO_IDX + 1U)
-#define SLEEP_CTL_PIO_IDX (PIO_RESET_REG_IDX + 1U)
-#define EMUL_PIO_IDX_MAX (SLEEP_CTL_PIO_IDX + 1U)
+ PIC_PRIMARY_PIO_IDX,
+ PIC_SECONDARY_PIO_IDX,
+ PIC_ELC_PIO_IDX,
+ PCI_CFGADDR_PIO_IDX,
+ PCI_CFGDATA_PIO_IDX,
+ PM1A_EVT_PIO_IDX,
+ PM1A_CNT_PIO_IDX,
+ PM1B_EVT_PIO_IDX,
+ PM1B_CNT_PIO_IDX,
+ RTC_PIO_IDX,
+ VIRTUAL_PM1A_CNT_PIO_IDX,
+ KB_PIO_IDX,
+ CF9_PIO_IDX,
+ PIO_RESET_REG_IDX,
+ SLEEP_CTL_PIO_IDX,
+ UART_PIO_IDX0,
+ EMUL_PIO_IDX_MAX = (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM),
+};
+
/**
* @brief The handler of VM exits on I/O instructions
*
--
2.17.1


Junjie Mao
 

Chenli Wei <chenli.wei@...> writes:

The current code define *PIO_IDX by macro, it's not a good style and
the UART PIO was in the middle of these macro which should move to the
end to conveniently modify.

This patch change these macro to enum and move the UART PIO to the end
of these PIO_IDX list.

v5-->v6:
1.code format

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/arch/x86/guest/pm.c | 2 +-
hypervisor/dm/io_req.c | 2 +-
.../include/arch/x86/asm/guest/vmx_io.h | 38 ++++++++++---------
3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 2af0dd259..a7f71fca7 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -249,7 +249,7 @@ static bool pm1ab_io_write(struct acrn_vcpu *vcpu, uint16_t addr, size_t width,
return true;
}

-static void register_gas_io_handler(struct acrn_vm *vm, uint32_t pio_idx, const struct acrn_acpi_generic_address *gas)
+static void register_gas_io_handler(struct acrn_vm *vm, enum port_io_index pio_idx, const struct acrn_acpi_generic_address *gas)
{
uint8_t io_len[5] = {0U, 1U, 2U, 4U, 8U};
struct vm_io_range gas_io;
diff --git a/hypervisor/dm/io_req.c b/hypervisor/dm/io_req.c
index b68a66cab..fbc38cf46 100644
--- a/hypervisor/dm/io_req.c
+++ b/hypervisor/dm/io_req.c
@@ -597,7 +597,7 @@ emulate_io(struct acrn_vcpu *vcpu, struct io_request *io_req)
* @param io_write_fn_ptr The handler for emulating writes to the given range
* @pre pio_idx < EMUL_PIO_IDX_MAX
*/
-void register_pio_emulation_handler(struct acrn_vm *vm, uint32_t pio_idx,
+void register_pio_emulation_handler(struct acrn_vm *vm, enum port_io_index pio_idx,
const struct vm_io_range *range, io_read_fn_t io_read_fn_ptr, io_write_fn_t io_write_fn_ptr)
{
if (is_service_vm(vm)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vmx_io.h b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
index 5c41b4d4c..3ddb46fee 100644
--- a/hypervisor/include/arch/x86/asm/guest/vmx_io.h
+++ b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
@@ -9,25 +9,27 @@

#include <types.h>

+enum port_io_index {
/* Define emulated port IO index */
-#define PIC_PRIMARY_PIO_IDX 0U
-#define PIC_SECONDARY_PIO_IDX (PIC_PRIMARY_PIO_IDX + 1U)
-#define PIC_ELC_PIO_IDX (PIC_SECONDARY_PIO_IDX + 1U)
-#define PCI_CFGADDR_PIO_IDX (PIC_ELC_PIO_IDX + 1U)
-#define PCI_CFGDATA_PIO_IDX (PCI_CFGADDR_PIO_IDX + 1U)
-/* MAX_VUART_NUM_PER_VM is 8, so allocate UART_PIO_IDX0~UART_PIO_IDX0 + 7 for 8 vuart */
-#define UART_PIO_IDX0 (PCI_CFGDATA_PIO_IDX + 1U)
-#define PM1A_EVT_PIO_IDX (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM)
-#define PM1A_CNT_PIO_IDX (PM1A_EVT_PIO_IDX + 1U)
-#define PM1B_EVT_PIO_IDX (PM1A_CNT_PIO_IDX + 1U)
-#define PM1B_CNT_PIO_IDX (PM1B_EVT_PIO_IDX + 1U)
-#define RTC_PIO_IDX (PM1B_CNT_PIO_IDX + 1U)
-#define VIRTUAL_PM1A_CNT_PIO_IDX (RTC_PIO_IDX + 1U)
-#define KB_PIO_IDX (VIRTUAL_PM1A_CNT_PIO_IDX + 1U)
-#define CF9_PIO_IDX (KB_PIO_IDX + 1U)
-#define PIO_RESET_REG_IDX (CF9_PIO_IDX + 1U)
-#define SLEEP_CTL_PIO_IDX (PIO_RESET_REG_IDX + 1U)
-#define EMUL_PIO_IDX_MAX (SLEEP_CTL_PIO_IDX + 1U)
+ PIC_PRIMARY_PIO_IDX,
+ PIC_SECONDARY_PIO_IDX,
+ PIC_ELC_PIO_IDX,
+ PCI_CFGADDR_PIO_IDX,
+ PCI_CFGDATA_PIO_IDX,
+ PM1A_EVT_PIO_IDX,
+ PM1A_CNT_PIO_IDX,
+ PM1B_EVT_PIO_IDX,
+ PM1B_CNT_PIO_IDX,
+ RTC_PIO_IDX,
+ VIRTUAL_PM1A_CNT_PIO_IDX,
+ KB_PIO_IDX,
+ CF9_PIO_IDX,
+ PIO_RESET_REG_IDX,
+ SLEEP_CTL_PIO_IDX,
+ UART_PIO_IDX0,
+ EMUL_PIO_IDX_MAX = (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM),
+};
+
A reminder: using enumeration constants as integers is prohibited by
ACRN coding guideline.

In this case the enumeration above is not exhaustive (due to the
variable number of UARTs) and will eventually trigger enum to integer
conversions somewhere in the code.

--
Best Regards
Junjie Mao

/**
* @brief The handler of VM exits on I/O instructions
*


chenli.wei
 

On 5/12/2022 5:01 PM, Junjie Mao wrote:
Chenli Wei <chenli.wei@...> writes:

The current code define *PIO_IDX by macro, it's not a good style and
the UART PIO was in the middle of these macro which should move to the
end to conveniently modify.

This patch change these macro to enum and move the UART PIO to the end
of these PIO_IDX list.

v5-->v6:
1.code format

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/arch/x86/guest/pm.c | 2 +-
hypervisor/dm/io_req.c | 2 +-
.../include/arch/x86/asm/guest/vmx_io.h | 38 ++++++++++---------
3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 2af0dd259..a7f71fca7 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -249,7 +249,7 @@ static bool pm1ab_io_write(struct acrn_vcpu *vcpu, uint16_t addr, size_t width,
return true;
}
-static void register_gas_io_handler(struct acrn_vm *vm, uint32_t pio_idx, const struct acrn_acpi_generic_address *gas)
+static void register_gas_io_handler(struct acrn_vm *vm, enum port_io_index pio_idx, const struct acrn_acpi_generic_address *gas)
{
uint8_t io_len[5] = {0U, 1U, 2U, 4U, 8U};
struct vm_io_range gas_io;
diff --git a/hypervisor/dm/io_req.c b/hypervisor/dm/io_req.c
index b68a66cab..fbc38cf46 100644
--- a/hypervisor/dm/io_req.c
+++ b/hypervisor/dm/io_req.c
@@ -597,7 +597,7 @@ emulate_io(struct acrn_vcpu *vcpu, struct io_request *io_req)
* @param io_write_fn_ptr The handler for emulating writes to the given range
* @pre pio_idx < EMUL_PIO_IDX_MAX
*/
-void register_pio_emulation_handler(struct acrn_vm *vm, uint32_t pio_idx,
+void register_pio_emulation_handler(struct acrn_vm *vm, enum port_io_index pio_idx,
const struct vm_io_range *range, io_read_fn_t io_read_fn_ptr, io_write_fn_t io_write_fn_ptr)
{
if (is_service_vm(vm)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vmx_io.h b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
index 5c41b4d4c..3ddb46fee 100644
--- a/hypervisor/include/arch/x86/asm/guest/vmx_io.h
+++ b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
@@ -9,25 +9,27 @@
#include <types.h>
+enum port_io_index {
/* Define emulated port IO index */
-#define PIC_PRIMARY_PIO_IDX 0U
-#define PIC_SECONDARY_PIO_IDX (PIC_PRIMARY_PIO_IDX + 1U)
-#define PIC_ELC_PIO_IDX (PIC_SECONDARY_PIO_IDX + 1U)
-#define PCI_CFGADDR_PIO_IDX (PIC_ELC_PIO_IDX + 1U)
-#define PCI_CFGDATA_PIO_IDX (PCI_CFGADDR_PIO_IDX + 1U)
-/* MAX_VUART_NUM_PER_VM is 8, so allocate UART_PIO_IDX0~UART_PIO_IDX0 + 7 for 8 vuart */
-#define UART_PIO_IDX0 (PCI_CFGDATA_PIO_IDX + 1U)
-#define PM1A_EVT_PIO_IDX (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM)
-#define PM1A_CNT_PIO_IDX (PM1A_EVT_PIO_IDX + 1U)
-#define PM1B_EVT_PIO_IDX (PM1A_CNT_PIO_IDX + 1U)
-#define PM1B_CNT_PIO_IDX (PM1B_EVT_PIO_IDX + 1U)
-#define RTC_PIO_IDX (PM1B_CNT_PIO_IDX + 1U)
-#define VIRTUAL_PM1A_CNT_PIO_IDX (RTC_PIO_IDX + 1U)
-#define KB_PIO_IDX (VIRTUAL_PM1A_CNT_PIO_IDX + 1U)
-#define CF9_PIO_IDX (KB_PIO_IDX + 1U)
-#define PIO_RESET_REG_IDX (CF9_PIO_IDX + 1U)
-#define SLEEP_CTL_PIO_IDX (PIO_RESET_REG_IDX + 1U)
-#define EMUL_PIO_IDX_MAX (SLEEP_CTL_PIO_IDX + 1U)
+ PIC_PRIMARY_PIO_IDX,
+ PIC_SECONDARY_PIO_IDX,
+ PIC_ELC_PIO_IDX,
+ PCI_CFGADDR_PIO_IDX,
+ PCI_CFGDATA_PIO_IDX,
+ PM1A_EVT_PIO_IDX,
+ PM1A_CNT_PIO_IDX,
+ PM1B_EVT_PIO_IDX,
+ PM1B_CNT_PIO_IDX,
+ RTC_PIO_IDX,
+ VIRTUAL_PM1A_CNT_PIO_IDX,
+ KB_PIO_IDX,
+ CF9_PIO_IDX,
+ PIO_RESET_REG_IDX,
+ SLEEP_CTL_PIO_IDX,
+ UART_PIO_IDX0,
+ EMUL_PIO_IDX_MAX = (UART_PIO_IDX0 + MAX_VUART_NUM_PER_VM),
+};
+
A reminder: using enumeration constants as integers is prohibited by
ACRN coding guideline.

In this case the enumeration above is not exhaustive (due to the
variable number of UARTs) and will eventually trigger enum to integer
conversions somewhere in the code.
It seems we should confirm this patch again.

I will pause this patch and send others of this series to PR.


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of chenli.wei
Sent: Thursday, May 12, 2022 1:16 AM
To: Mao, Junjie <junjie.mao@...>; acrn-dev@...
Cc: Wei, Chenli <chenli.wei@...>
Subject: [acrn-dev] [PATCH v6 4/4] hv: refine the define of *PIO_IDX

The current code define *PIO_IDX by macro, it's not a good style and the
UART PIO was in the middle of these macro which should move to the end to
conveniently modify.

This patch change these macro to enum and move the UART PIO to the end
of these PIO_IDX list.

v5-->v6:
1.code format

Signed-off-by: Chenli Wei <chenli.wei@...>
---
hypervisor/arch/x86/guest/pm.c | 2 +-
hypervisor/dm/io_req.c | 2 +-
.../include/arch/x86/asm/guest/vmx_io.h | 38 ++++++++++---------
3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c
b/hypervisor/arch/x86/guest/pm.c index 2af0dd259..a7f71fca7 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -249,7 +249,7 @@ static bool pm1ab_io_write(struct acrn_vcpu *vcpu,
uint16_t addr, size_t width,
return true;
}

-static void register_gas_io_handler(struct acrn_vm *vm, uint32_t pio_idx,
const struct acrn_acpi_generic_address *gas)
+static void register_gas_io_handler(struct acrn_vm *vm, enum
+port_io_index pio_idx, const struct acrn_acpi_generic_address *gas)
{
uint8_t io_len[5] = {0U, 1U, 2U, 4U, 8U};
struct vm_io_range gas_io;
diff --git a/hypervisor/dm/io_req.c b/hypervisor/dm/io_req.c index
b68a66cab..fbc38cf46 100644
--- a/hypervisor/dm/io_req.c
+++ b/hypervisor/dm/io_req.c
@@ -597,7 +597,7 @@ emulate_io(struct acrn_vcpu *vcpu, struct
io_request *io_req)
* @param io_write_fn_ptr The handler for emulating writes to the given
range
* @pre pio_idx < EMUL_PIO_IDX_MAX
*/
-void register_pio_emulation_handler(struct acrn_vm *vm, uint32_t pio_idx,
+void register_pio_emulation_handler(struct acrn_vm *vm, enum
+port_io_index pio_idx,
const struct vm_io_range *range, io_read_fn_t
io_read_fn_ptr, io_write_fn_t io_write_fn_ptr) {
if (is_service_vm(vm)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vmx_io.h
b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
index 5c41b4d4c..3ddb46fee 100644
--- a/hypervisor/include/arch/x86/asm/guest/vmx_io.h
+++ b/hypervisor/include/arch/x86/asm/guest/vmx_io.h
@@ -9,25 +9,27 @@

#include <types.h>

+enum port_io_index {
/* Define emulated port IO index */
-#define PIC_PRIMARY_PIO_IDX 0U
-#define PIC_SECONDARY_PIO_IDX (PIC_PRIMARY_PIO_IDX +
1U)
-#define PIC_ELC_PIO_IDX (PIC_SECONDARY_PIO_IDX
+ 1U)
-#define PCI_CFGADDR_PIO_IDX (PIC_ELC_PIO_IDX + 1U)
-#define PCI_CFGDATA_PIO_IDX (PCI_CFGADDR_PIO_IDX + 1U)
-/* MAX_VUART_NUM_PER_VM is 8, so allocate
UART_PIO_IDX0~UART_PIO_IDX0 + 7 for 8 vuart */
-#define UART_PIO_IDX0 (PCI_CFGDATA_PIO_IDX +
1U)
-#define PM1A_EVT_PIO_IDX (UART_PIO_IDX0 +
MAX_VUART_NUM_PER_VM)
-#define PM1A_CNT_PIO_IDX (PM1A_EVT_PIO_IDX + 1U)
-#define PM1B_EVT_PIO_IDX (PM1A_CNT_PIO_IDX + 1U)
-#define PM1B_CNT_PIO_IDX (PM1B_EVT_PIO_IDX + 1U)
-#define RTC_PIO_IDX (PM1B_CNT_PIO_IDX + 1U)
-#define VIRTUAL_PM1A_CNT_PIO_IDX (RTC_PIO_IDX + 1U)
-#define KB_PIO_IDX (VIRTUAL_PM1A_CNT_PIO_IDX +
1U)
-#define CF9_PIO_IDX (KB_PIO_IDX + 1U)
-#define PIO_RESET_REG_IDX (CF9_PIO_IDX + 1U)
-#define SLEEP_CTL_PIO_IDX (PIO_RESET_REG_IDX + 1U)
-#define EMUL_PIO_IDX_MAX (SLEEP_CTL_PIO_IDX + 1U)
+ PIC_PRIMARY_PIO_IDX,
Should we add "=0" for the 1st element?

The rest is fine. Please PR with my ack.

+ PIC_SECONDARY_PIO_IDX,
+ PIC_ELC_PIO_IDX,
+ PCI_CFGADDR_PIO_IDX,
+ PCI_CFGDATA_PIO_IDX,
+ PM1A_EVT_PIO_IDX,
+ PM1A_CNT_PIO_IDX,
+ PM1B_EVT_PIO_IDX,
+ PM1B_CNT_PIO_IDX,
+ RTC_PIO_IDX,
+ VIRTUAL_PM1A_CNT_PIO_IDX,
+ KB_PIO_IDX,
+ CF9_PIO_IDX,
+ PIO_RESET_REG_IDX,
+ SLEEP_CTL_PIO_IDX,
+ UART_PIO_IDX0,
+ EMUL_PIO_IDX_MAX = (UART_PIO_IDX0 +
MAX_VUART_NUM_PER_VM), };
+
/**
* @brief The handler of VM exits on I/O instructions
*
--
2.17.1





Eddie Dong
 


A reminder: using enumeration constants as integers is prohibited by ACRN
coding guideline.
Aaa, If this is the case, we have to drop this patch.


In this case the enumeration above is not exhaustive (due to the variable
number of UARTs) and will eventually trigger enum to integer conversions
somewhere in the code.

--
Best Regards
Junjie Mao

/**
* @brief The handler of VM exits on I/O instructions
*



chenli.wei
 

On 5/13/2022 2:56 AM, Eddie Dong wrote:
A reminder: using enumeration constants as integers is prohibited by ACRN
coding guideline.
Aaa, If this is the case, we have to drop this patch.
OK.

I have drop this patch and send others to PR.


In this case the enumeration above is not exhaustive (due to the variable
number of UARTs) and will eventually trigger enum to integer conversions
somewhere in the code.

--
Best Regards
Junjie Mao

/**
* @brief The handler of VM exits on I/O instructions
*