Date   

Is there ignore device parameter for acrn-dm

chen yi <yi.chen.xerox@...>
 

Hi

 

I got a problem when setting passthrough device

For example I plug a ethernet pcie card on my acrn system.

Build hypervisor and set ethernet pcie card 07:00.0 Ethernet controller: Intel Corporation I211 Gigabit Network Connection (rev 03) as a passthrough device on virtual machine in launch.xml.

It works fine, virtual machine can recognize ethernet pcie card and works well.

But when I unplug my ethernet pcie card I can not boot into my virtual machine.

Is there any parameter for acrn-dm that can ignore disappear passthrough devices?

If acrn-dm can not find ethernet pcie card it will ignore it and still boot virtual machine.

 

Best Regards,


Re: [PATCH 4/4] dm: Skip injecting _PPC and _PCT when _PSS is not constructed

Eddie Dong
 

+1

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Xu, Anthony
Sent: Wednesday, November 17, 2021 7:22 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH 4/4] dm: Skip injecting _PPC and _PCT when
_PSS is not constructed

Acked-by: Anthony Xu <anthony.xu@...>

No _PSS, then no _PPC or _PCT.
Please PR this patch.


Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 4/4] dm: Skip injecting _PPC and _PCT when
_PSS is not constructed

This patch is to eliminate kernel error msgs:
'ACPI Error: AE_NOT_FOUND, Evaluating _PSS'

This is caused by missing of _PSS table in guest ACPI. It would happen
when pstate is not injected to the guest.

Kernel ACPI pstate driver first probes _PPC(performance capabilites)
and _PCT(performance control) in ACPI. If they exist, then it loads
the _PSS(performance state).
If _PPC/_PCT are presented while _PSS is missing, it prints the error
msg.

In acrn-dm, _PPC/_PCT are hard-coded to all vCPUs, while _PSS are
constructed with the pCPUs' pstate data. This is base on assumption
that all VMs can have pstate.

Now the pstate is given to VM only when the VM is not sharing any
CPU(and no RTVM is setup in the scenario).
When the VM doesn't have pstate, the hypercall will return px_cnt=0,
and the _PSS is not constructed. In this case, _PPC/PCT should not be
injected, too.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
devicemodel/hw/platform/acpi/acpi_pm.c | 45
++++++++++++++------------
1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/devicemodel/hw/platform/acpi/acpi_pm.c
b/devicemodel/hw/platform/acpi/acpi_pm.c
index 9428db159..a1c48dd6b 100644
--- a/devicemodel/hw/platform/acpi/acpi_pm.c
+++ b/devicemodel/hw/platform/acpi/acpi_pm.c
@@ -254,7 +254,7 @@ static void dsdt_write_pct(void)

/* _PSS: Performance Supported States
*/
-static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
+static int dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
{
uint8_t vcpu_px_cnt;
int i;
@@ -262,12 +262,12 @@ static void dsdt_write_pss(struct vmctx *ctx,
int vcpu_id)

vcpu_px_cnt = get_vcpu_px_cnt(ctx, vcpu_id);
if (!vcpu_px_cnt) {
- return;
+ return -1;
}

vcpu_px_data = malloc(vcpu_px_cnt * sizeof(struct acrn_pstate_data));
if (!vcpu_px_data) {
- return;
+ return -1;
}

/* copy and validate px data first */ @@ -275,7 +275,7 @@ static
void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
if (get_vcpu_px_data(ctx, vcpu_id, i, vcpu_px_data + i)) {
/* something must be wrong, so skip the write. */
free(vcpu_px_data);
- return;
+ return -1;
}
}

@@ -313,11 +313,13 @@ static void dsdt_write_pss(struct vmctx *ctx,
int vcpu_id)

free(vcpu_px_data);

+ return 0;
}

void pm_write_dsdt(struct vmctx *ctx, int ncpu) {
int i;
+ int ret;

/* Scope (_PR) */
dsdt_line("");
@@ -339,24 +341,27 @@ void pm_write_dsdt(struct vmctx *ctx, int ncpu)
dsdt_line(" {");
dsdt_line("");

- dsdt_write_pss(ctx, i);
+ ret = dsdt_write_pss(ctx, i);
dsdt_write_cst(ctx, i);

- /* hard code _PPC and _PCT for all vpu */
- if (i == 0) {
- dsdt_write_ppc();
- dsdt_write_pct();
- } else {
- dsdt_line(" Method (_PPC, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PPC)");
- dsdt_line(" }");
- dsdt_line("");
- dsdt_line(" Method (_PCT, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PCT)");
- dsdt_line(" }");
- dsdt_line("");
+ /* if the vm can support px, hv will return the right px/cx cnt.
+ then hard code _PPC and _PCT for all vpu */
+ if (ret == 0) {
+ if (i == 0) {
+ dsdt_write_ppc();
+ dsdt_write_pct();
+ } else {
+ dsdt_line(" Method (_PPC, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PPC)");
+ dsdt_line(" }");
+ dsdt_line("");
+ dsdt_line(" Method (_PCT, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PCT)");
+ dsdt_line(" }");
+ dsdt_line("");
+ }
}

dsdt_line(" }");
--
2.17.1








Re: [PATCH 3/4] hv: hypercall: return 0 for empty PX or CX tables

Eddie Dong
 

My Ack too.

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Xu, Anthony
Sent: Wednesday, November 17, 2021 7:14 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH 3/4] hv: hypercall: return 0 for empty PX or CX
tables

Acked-by: Anthony Xu <anthony.xu@...>

Getting count should not fail.
You can PR this patch.


Anthony



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 3/4] hv: hypercall: return 0 for empty PX
or CX tables

From: Helmut Buchsbaum
<helmut.buchsbaum@...>

Avoid failing hypercalls by returning 0 for empty PX and CX tables on
HC_PM_GET_CPU_STATE/PMCMD_GET_PX_CNT and
HC_PM_GET_CPU_STATE/PMCMD_GET_CX_CNT.

Signed-off-by: Helmut Buchsbaum
<helmut.buchsbaum@...>
---
hypervisor/common/hypercall.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hypervisor/common/hypercall.c
b/hypervisor/common/hypercall.c index 7fa25abd3..ee3541453 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1094,9 +1094,7 @@ int32_t hcall_get_cpu_pm_state(struct
acrn_vcpu *vcpu, struct acrn_vm *target_vm
if (is_created_vm(target_vm)) {
switch (cmd & PMCMD_TYPE_MASK) {
case ACRN_PMCMD_GET_PX_CNT: {
- if (target_vm->pm.px_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2,
sizeof(target_vm->pm.px_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2,
+sizeof(target_vm->pm.px_cnt));
break;
}
case ACRN_PMCMD_GET_PX_DATA: {
@@ -1121,9 +1119,7 @@ int32_t hcall_get_cpu_pm_state(struct
acrn_vcpu *vcpu, struct acrn_vm *target_vm
break;
}
case ACRN_PMCMD_GET_CX_CNT: {
- if (target_vm->pm.cx_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2,
sizeof(target_vm->pm.cx_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2,
+sizeof(target_vm->pm.cx_cnt));
break;
}
case ACRN_PMCMD_GET_CX_DATA: {
--
2.17.1








Re: [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 4:47 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 2/4] hv: remove pstate/cstate support for VMs
that share pCPU

Only when the a post-launched VM is not sharing pCPUs and no RTVM is
setup in the scenario, the pCPU’s pstate/cstate can be passed-though to the
VM.

The VM_PX_CX_CAPABILITY macro is generated by config_tools, this patch
Using per VM bit inside the MACRO is not good.

Or to use a bit in guest_flags, plus a global MACRO to indicate whether RTVM is configured or not?

simply use it to determine whether a VM can have pstate/cstate.
If it is negative, the px_cnt and cx_cnt will be zero. The dm will also get 0 for
px_cnt and cx_cnt on hypercall, and the VM will get no pstate/cstate.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
hypervisor/arch/x86/guest/pm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c
b/hypervisor/arch/x86/guest/pm.c index 9771dda72..acf12bbcb 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -59,7 +59,8 @@ static void vm_setup_cpu_px(struct acrn_vm *vm)
vm->pm.px_cnt = 0U;
(void)memset(vm->pm.px_data, 0U, MAX_PSTATE * sizeof(struct
acrn_pstate_data));

- if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL))
{
+ if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL)
&&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->px_cnt <= MAX_PSTATE), "failed to setup
cpu px");

vm->pm.px_cnt = pm_state_info->px_cnt; @@ -76,7 +77,8 @@
static void vm_setup_cpu_cx(struct acrn_vm *vm)
vm->pm.cx_cnt = 0U;
(void)memset(vm->pm.cx_data, 0U, MAX_CSTATE * sizeof(struct
acrn_cstate_data));

- if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL))
{
+ if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL)
&&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->cx_cnt <= MAX_CX_ENTRY), "failed to
setup cpu cx");

vm->pm.cx_cnt = pm_state_info->cx_cnt;
--
2.17.1





Re: [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Xu, Anthony
 

Like VM flag in acrn-hypervisor/hypervisor/include/public/acrn_common.h

Anthony

-----Original Message-----
From: Xu, Anthony
Sent: Tuesday, November 16, 2021 3:07 PM
To: acrn-dev@...
Subject: RE: [acrn-dev] [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Can config tools generate a flag in vm_config for PX_CX?

Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Only when the a post-launched VM is not sharing pCPUs and no RTVM
is setup in the scenario, the pCPU’s pstate/cstate can be
passed-though to the VM.

The VM_PX_CX_CAPABILITY macro is generated by config_tools, this
patch simply use it to determine whether a VM can have pstate/cstate.
If it is negative, the px_cnt and cx_cnt will be zero. The dm will
also get 0 for px_cnt and cx_cnt on hypercall, and the VM will get
no pstate/cstate.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
hypervisor/arch/x86/guest/pm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 9771dda72..acf12bbcb 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -59,7 +59,8 @@ static void vm_setup_cpu_px(struct acrn_vm *vm)
vm->pm.px_cnt = 0U;
(void)memset(vm->pm.px_data, 0U, MAX_PSTATE * sizeof(struct acrn_pstate_data));

- if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL)) {
+ if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->px_cnt <= MAX_PSTATE), "failed to setup cpu px");

vm->pm.px_cnt = pm_state_info->px_cnt;
@@ -76,7 +77,8 @@ static void vm_setup_cpu_cx(struct acrn_vm *vm)
vm->pm.cx_cnt = 0U;
(void)memset(vm->pm.cx_data, 0U, MAX_CSTATE * sizeof(struct acrn_cstate_data));

- if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL)) {
+ if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->cx_cnt <= MAX_CX_ENTRY), "failed to setup cpu cx");

vm->pm.cx_cnt = pm_state_info->cx_cnt;
--
2.17.1





Re: [PATCH 4/4] dm: Skip injecting _PPC and _PCT when _PSS is not constructed

Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

No _PSS, then no _PPC or _PCT.
Please PR this patch.


Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 4/4] dm: Skip injecting _PPC and _PCT when _PSS is not constructed

This patch is to eliminate kernel error msgs:
'ACPI Error: AE_NOT_FOUND, Evaluating _PSS'

This is caused by missing of _PSS table in guest ACPI. It would
happen when pstate is not injected to the guest.

Kernel ACPI pstate driver first probes
_PPC(performance capabilites) and _PCT(performance control)
in ACPI. If they exist, then it loads the _PSS(performance state).
If _PPC/_PCT are presented while _PSS is missing, it prints
the error msg.

In acrn-dm, _PPC/_PCT are hard-coded to all vCPUs, while _PSS
are constructed with the pCPUs' pstate data. This is base on
assumption that all VMs can have pstate.

Now the pstate is given to VM only when the VM is not sharing
any CPU(and no RTVM is setup in the scenario).
When the VM doesn't have pstate, the hypercall will return px_cnt=0,
and the _PSS is not constructed. In this case, _PPC/PCT should not
be injected, too.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
devicemodel/hw/platform/acpi/acpi_pm.c | 45 ++++++++++++++------------
1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/devicemodel/hw/platform/acpi/acpi_pm.c b/devicemodel/hw/platform/acpi/acpi_pm.c
index 9428db159..a1c48dd6b 100644
--- a/devicemodel/hw/platform/acpi/acpi_pm.c
+++ b/devicemodel/hw/platform/acpi/acpi_pm.c
@@ -254,7 +254,7 @@ static void dsdt_write_pct(void)

/* _PSS: Performance Supported States
*/
-static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
+static int dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
{
uint8_t vcpu_px_cnt;
int i;
@@ -262,12 +262,12 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)

vcpu_px_cnt = get_vcpu_px_cnt(ctx, vcpu_id);
if (!vcpu_px_cnt) {
- return;
+ return -1;
}

vcpu_px_data = malloc(vcpu_px_cnt * sizeof(struct acrn_pstate_data));
if (!vcpu_px_data) {
- return;
+ return -1;
}

/* copy and validate px data first */
@@ -275,7 +275,7 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
if (get_vcpu_px_data(ctx, vcpu_id, i, vcpu_px_data + i)) {
/* something must be wrong, so skip the write. */
free(vcpu_px_data);
- return;
+ return -1;
}
}

@@ -313,11 +313,13 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)

free(vcpu_px_data);

+ return 0;
}

void pm_write_dsdt(struct vmctx *ctx, int ncpu)
{
int i;
+ int ret;

/* Scope (_PR) */
dsdt_line("");
@@ -339,24 +341,27 @@ void pm_write_dsdt(struct vmctx *ctx, int ncpu)
dsdt_line(" {");
dsdt_line("");

- dsdt_write_pss(ctx, i);
+ ret = dsdt_write_pss(ctx, i);
dsdt_write_cst(ctx, i);

- /* hard code _PPC and _PCT for all vpu */
- if (i == 0) {
- dsdt_write_ppc();
- dsdt_write_pct();
- } else {
- dsdt_line(" Method (_PPC, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PPC)");
- dsdt_line(" }");
- dsdt_line("");
- dsdt_line(" Method (_PCT, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PCT)");
- dsdt_line(" }");
- dsdt_line("");
+ /* if the vm can support px, hv will return the right px/cx cnt.
+ then hard code _PPC and _PCT for all vpu */
+ if (ret == 0) {
+ if (i == 0) {
+ dsdt_write_ppc();
+ dsdt_write_pct();
+ } else {
+ dsdt_line(" Method (_PPC, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PPC)");
+ dsdt_line(" }");
+ dsdt_line("");
+ dsdt_line(" Method (_PCT, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PCT)");
+ dsdt_line(" }");
+ dsdt_line("");
+ }
}

dsdt_line(" }");
--
2.17.1





Re: [PATCH 3/4] hv: hypercall: return 0 for empty PX or CX tables

Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

Getting count should not fail.
You can PR this patch.


Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 3/4] hv: hypercall: return 0 for empty PX or CX tables

From: Helmut Buchsbaum <helmut.buchsbaum@...>

Avoid failing hypercalls by returning 0 for empty PX and CX tables on
HC_PM_GET_CPU_STATE/PMCMD_GET_PX_CNT and
HC_PM_GET_CPU_STATE/PMCMD_GET_CX_CNT.

Signed-off-by: Helmut Buchsbaum <helmut.buchsbaum@...>
---
hypervisor/common/hypercall.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 7fa25abd3..ee3541453 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1094,9 +1094,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vcpu *vcpu, struct acrn_vm *target_vm
if (is_created_vm(target_vm)) {
switch (cmd & PMCMD_TYPE_MASK) {
case ACRN_PMCMD_GET_PX_CNT: {
- if (target_vm->pm.px_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2, sizeof(target_vm->pm.px_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2, sizeof(target_vm->pm.px_cnt));
break;
}
case ACRN_PMCMD_GET_PX_DATA: {
@@ -1121,9 +1119,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vcpu *vcpu, struct acrn_vm *target_vm
break;
}
case ACRN_PMCMD_GET_CX_CNT: {
- if (target_vm->pm.cx_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2, sizeof(target_vm->pm.cx_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2, sizeof(target_vm->pm.cx_cnt));
break;
}
case ACRN_PMCMD_GET_CX_DATA: {
--
2.17.1





Re: [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Xu, Anthony
 

Can config tools generate a flag in vm_config for PX_CX?

Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Only when the a post-launched VM is not sharing pCPUs and no RTVM
is setup in the scenario, the pCPU’s pstate/cstate can be
passed-though to the VM.

The VM_PX_CX_CAPABILITY macro is generated by config_tools, this
patch simply use it to determine whether a VM can have pstate/cstate.
If it is negative, the px_cnt and cx_cnt will be zero. The dm will
also get 0 for px_cnt and cx_cnt on hypercall, and the VM will get
no pstate/cstate.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
hypervisor/arch/x86/guest/pm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 9771dda72..acf12bbcb 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -59,7 +59,8 @@ static void vm_setup_cpu_px(struct acrn_vm *vm)
vm->pm.px_cnt = 0U;
(void)memset(vm->pm.px_data, 0U, MAX_PSTATE * sizeof(struct acrn_pstate_data));

- if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL)) {
+ if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->px_cnt <= MAX_PSTATE), "failed to setup cpu px");

vm->pm.px_cnt = pm_state_info->px_cnt;
@@ -76,7 +77,8 @@ static void vm_setup_cpu_cx(struct acrn_vm *vm)
vm->pm.cx_cnt = 0U;
(void)memset(vm->pm.cx_data, 0U, MAX_CSTATE * sizeof(struct acrn_cstate_data));

- if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL)) {
+ if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->cx_cnt <= MAX_CX_ENTRY), "failed to setup cpu cx");

vm->pm.cx_cnt = pm_state_info->cx_cnt;
--
2.17.1





Re: [PATCH 1/4] config_tools: generate vm pstate/cstate capability macro

Xu, Anthony
 

Do we need to consider SOS?

Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 12:47 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 1/4] config_tools: generate vm pstate/cstate capability macro

Currently acrn hypervisor doesn’t manage vm’s pstate/cstate. And The
pstate/cstate is passed-through to all post-launched Vms. Since the
VMs could share pCPUs, this is a bug.

Only when the a post-launched VM’s vCPUs is not sharing pCPUs, the
pCPU’s pstate/cstate can be passed-though to the vm.

By using the VM’s CPU affinity info, this patch generates a vm
pstate/cstate capability macro VM_PX_CX_CAPABILITY, so that dm can
use it (through hypercall) to determine whether the VMs can have
pstate/cstate.

Another thing is that when pstate/cstate changes on any CPU,
hardware will generate a bus lock, which is harmful to RT performance.
So when a RTVM is setup in the scenario, pstate/cstate is disabled
on all VMs.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
misc/config_tools/static_allocators/pm.py | 43 +++++++++++++++++++++++
misc/config_tools/xforms/misc_cfg.h.xsl | 6 ++++
2 files changed, 49 insertions(+)
create mode 100644 misc/config_tools/static_allocators/pm.py

diff --git a/misc/config_tools/static_allocators/pm.py b/misc/config_tools/static_allocators/pm.py
new file mode 100644
index 000000000..f99d34513
--- /dev/null
+++ b/misc/config_tools/static_allocators/pm.py
@@ -0,0 +1,43 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2021 Intel Corporation. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-3-Clause
+#
+
+import sys, os
+sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', 'library'))
+import common
+
+# post vms can have pstate/cstate on its vCPUs if those conditions are met:
+# 1.no share on the pCPUs
+# 2.the scenario has no rtvm(pre or post)
+# returns vm px/cx capability bit mask
+def get_post_vm_px_cx_capability(scenario_etree):
+ if common.get_node("//vm[vm_type = 'PRE_RT_VM']", scenario_etree) is not None:
+ return 0
+
+ if common.get_node("//vm[vm_type = 'POST_RT_VM']", scenario_etree) is not None:
+ return 0
+
+ all_cpus = scenario_etree.xpath("//vm/cpu_affinity/pcpu_id/text()")
+ post_vm_nodes = scenario_etree.xpath("//vm[vm_type = 'POST_STD_VM']")
+ vm_px_cx_mask = 0
+ for vm_node in post_vm_nodes:
+ vm_id = vm_node.get('id')
+ vm_cpus = vm_node.xpath('cpu_affinity/pcpu_id/text()')
+ vm_cpu_share = 0
+ for x in vm_cpus:
+ if all_cpus.count(x) != 1:
+ vm_cpu_share = 1
+ break
+ if vm_cpu_share == 0:
+ vm_px_cx_mask |= (1 << int(vm_id))
+
+ return vm_px_cx_mask
+
+def fn(board_etree, scenario_etree, allocation_etree):
+ vm_px_cx_cap = get_post_vm_px_cx_capability(scenario_etree)
+
+ common.append_node("/acrn-config/hv/pm/vm_px_cx_capability", hex(vm_px_cx_cap), allocation_etree)
+
diff --git a/misc/config_tools/xforms/misc_cfg.h.xsl b/misc/config_tools/xforms/misc_cfg.h.xsl
index 7ae8c1866..d97c9b6cb 100644
--- a/misc/config_tools/xforms/misc_cfg.h.xsl
+++ b/misc/config_tools/xforms/misc_cfg.h.xsl
@@ -26,6 +26,8 @@

<xsl:apply-templates select="allocation-data//ssram" />

+ <xsl:apply-templates select="allocation-data//hv/pm" />
+
<xsl:value-of select="acrn:include-guard-end('MISC_CFG_H')" />
</xsl:template>

@@ -52,6 +54,10 @@
<xsl:value-of select="acrn:define('PRE_RTVM_SW_SRAM_END_GPA', end_gpa, 'UL')" />
</xsl:template>

+ <xsl:template match="allocation-data//hv/pm">
+ <xsl:value-of select="acrn:define('VM_PX_CX_CAPABILITY', vm_px_cx_capability, 'UL')" />
+ </xsl:template>
+
<xsl:template match="BLOCK_DEVICE_INFO">
<xsl:variable name="block_devices_list_1" select="translate(current(), $newline, ',')" />
<xsl:variable name="block_devices_list_2" select="translate($block_devices_list_1, $whitespaces, '')" />
--
2.17.1





Re: [PATCH 0/4] Fix post-launched VM pstate/cstate bugs

Victor Sun
 

Hi Wu,
I don't think it is a good idea that use a VM bitmap to enable Px/Cx, this means the max VM num is limited.

BR,
Victor

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Zhou, Wu
Sent: Tuesday, November 16, 2021 4:47 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 0/4] Fix post-launched VM pstate/cstate bugs

Currently acrn hypervisor doesn’t manage vm’s pstate/cstate. And The
pstate/cstate is passed-through to all post-launched Vms. Since the VMs
could share pCPUs, they could operate on the same px/cx resources, this is a
bug.

So only when the a post-launched VM’s vCPUs is not sharing pCPUs, the
pCPU’s pstate/cstate can be passed-though to the vm.

By using the VM’s CPU affinity info, patch 1/4 generates a vm pstate/cstate
capability macro VM_PX_CX_CAPABILITY in config_tools, so that dm can use
it (through hypercall, it will return 0 for px_cnt/cx_cnt) to determine whether
the VMs can have pstate/cstate.

Another thing is that when pstate/cstate changes on any CPU, hardware will
generate a bus lock, which is harmful to RT performance.
So when a RTVM is setup in the scenario, pstate/cstate is disabled on all VMs.

If pstate is not given to a guest, linux kernel would print error msg:
'ACPI Error: AE_NOT_FOUND, Evaluating _PSS'
This is caused by hard-coded _PPC/_PCT in acrn-dm. Fixed in 4/4

*** BLURB HERE ***

Helmut Buchsbaum (1):
hv: hypercall: return 0 for empty PX or CX tables

Zhou, Wu (3):
config_tools: generate vm pstate/cstate capability macro
hv: remove pstate/cstate support for VMs that share pCPU
dm: Skip injecting _PPC and _PCT when _PSS is not constructed

devicemodel/hw/platform/acpi/acpi_pm.c | 45 +++++++++++++----------
hypervisor/arch/x86/guest/pm.c | 6 ++-
hypervisor/common/hypercall.c | 8 +---
misc/config_tools/static_allocators/pm.py | 43 ++++++++++++++++++++++
misc/config_tools/xforms/misc_cfg.h.xsl | 6 +++
5 files changed, 80 insertions(+), 28 deletions(-) create mode 100644
misc/config_tools/static_allocators/pm.py

--
2.17.1





[PATCH 4/4] dm: Skip injecting _PPC and _PCT when _PSS is not constructed

Zhou, Wu
 

This patch is to eliminate kernel error msgs:
'ACPI Error: AE_NOT_FOUND, Evaluating _PSS'

This is caused by missing of _PSS table in guest ACPI. It would
happen when pstate is not injected to the guest.

Kernel ACPI pstate driver first probes
_PPC(performance capabilites) and _PCT(performance control)
in ACPI. If they exist, then it loads the _PSS(performance state).
If _PPC/_PCT are presented while _PSS is missing, it prints
the error msg.

In acrn-dm, _PPC/_PCT are hard-coded to all vCPUs, while _PSS
are constructed with the pCPUs' pstate data. This is base on
assumption that all VMs can have pstate.

Now the pstate is given to VM only when the VM is not sharing
any CPU(and no RTVM is setup in the scenario).
When the VM doesn't have pstate, the hypercall will return px_cnt=0,
and the _PSS is not constructed. In this case, _PPC/PCT should not
be injected, too.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
devicemodel/hw/platform/acpi/acpi_pm.c | 45 ++++++++++++++------------
1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/devicemodel/hw/platform/acpi/acpi_pm.c b/devicemodel/hw/platform/acpi/acpi_pm.c
index 9428db159..a1c48dd6b 100644
--- a/devicemodel/hw/platform/acpi/acpi_pm.c
+++ b/devicemodel/hw/platform/acpi/acpi_pm.c
@@ -254,7 +254,7 @@ static void dsdt_write_pct(void)

/* _PSS: Performance Supported States
*/
-static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
+static int dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
{
uint8_t vcpu_px_cnt;
int i;
@@ -262,12 +262,12 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)

vcpu_px_cnt = get_vcpu_px_cnt(ctx, vcpu_id);
if (!vcpu_px_cnt) {
- return;
+ return -1;
}

vcpu_px_data = malloc(vcpu_px_cnt * sizeof(struct acrn_pstate_data));
if (!vcpu_px_data) {
- return;
+ return -1;
}

/* copy and validate px data first */
@@ -275,7 +275,7 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)
if (get_vcpu_px_data(ctx, vcpu_id, i, vcpu_px_data + i)) {
/* something must be wrong, so skip the write. */
free(vcpu_px_data);
- return;
+ return -1;
}
}

@@ -313,11 +313,13 @@ static void dsdt_write_pss(struct vmctx *ctx, int vcpu_id)

free(vcpu_px_data);

+ return 0;
}

void pm_write_dsdt(struct vmctx *ctx, int ncpu)
{
int i;
+ int ret;

/* Scope (_PR) */
dsdt_line("");
@@ -339,24 +341,27 @@ void pm_write_dsdt(struct vmctx *ctx, int ncpu)
dsdt_line(" {");
dsdt_line("");

- dsdt_write_pss(ctx, i);
+ ret = dsdt_write_pss(ctx, i);
dsdt_write_cst(ctx, i);

- /* hard code _PPC and _PCT for all vpu */
- if (i == 0) {
- dsdt_write_ppc();
- dsdt_write_pct();
- } else {
- dsdt_line(" Method (_PPC, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PPC)");
- dsdt_line(" }");
- dsdt_line("");
- dsdt_line(" Method (_PCT, 0, NotSerialized)");
- dsdt_line(" {");
- dsdt_line(" Return (^^CPU0._PCT)");
- dsdt_line(" }");
- dsdt_line("");
+ /* if the vm can support px, hv will return the right px/cx cnt.
+ then hard code _PPC and _PCT for all vpu */
+ if (ret == 0) {
+ if (i == 0) {
+ dsdt_write_ppc();
+ dsdt_write_pct();
+ } else {
+ dsdt_line(" Method (_PPC, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PPC)");
+ dsdt_line(" }");
+ dsdt_line("");
+ dsdt_line(" Method (_PCT, 0, NotSerialized)");
+ dsdt_line(" {");
+ dsdt_line(" Return (^^CPU0._PCT)");
+ dsdt_line(" }");
+ dsdt_line("");
+ }
}

dsdt_line(" }");
--
2.17.1


[PATCH 3/4] hv: hypercall: return 0 for empty PX or CX tables

Zhou, Wu
 

From: Helmut Buchsbaum <helmut.buchsbaum@...>

Avoid failing hypercalls by returning 0 for empty PX and CX tables on
HC_PM_GET_CPU_STATE/PMCMD_GET_PX_CNT and
HC_PM_GET_CPU_STATE/PMCMD_GET_CX_CNT.

Signed-off-by: Helmut Buchsbaum <helmut.buchsbaum@...>
---
hypervisor/common/hypercall.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 7fa25abd3..ee3541453 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1094,9 +1094,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vcpu *vcpu, struct acrn_vm *target_vm
if (is_created_vm(target_vm)) {
switch (cmd & PMCMD_TYPE_MASK) {
case ACRN_PMCMD_GET_PX_CNT: {
- if (target_vm->pm.px_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2, sizeof(target_vm->pm.px_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.px_cnt), param2, sizeof(target_vm->pm.px_cnt));
break;
}
case ACRN_PMCMD_GET_PX_DATA: {
@@ -1121,9 +1119,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vcpu *vcpu, struct acrn_vm *target_vm
break;
}
case ACRN_PMCMD_GET_CX_CNT: {
- if (target_vm->pm.cx_cnt != 0U) {
- ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2, sizeof(target_vm->pm.cx_cnt));
- }
+ ret = copy_to_gpa(vm, &(target_vm->pm.cx_cnt), param2, sizeof(target_vm->pm.cx_cnt));
break;
}
case ACRN_PMCMD_GET_CX_DATA: {
--
2.17.1


[PATCH 1/4] config_tools: generate vm pstate/cstate capability macro

Zhou, Wu
 

Currently acrn hypervisor doesn’t manage vm’s pstate/cstate. And The
pstate/cstate is passed-through to all post-launched Vms. Since the
VMs could share pCPUs, this is a bug.

Only when the a post-launched VM’s vCPUs is not sharing pCPUs, the
pCPU’s pstate/cstate can be passed-though to the vm.

By using the VM’s CPU affinity info, this patch generates a vm
pstate/cstate capability macro VM_PX_CX_CAPABILITY, so that dm can
use it (through hypercall) to determine whether the VMs can have
pstate/cstate.

Another thing is that when pstate/cstate changes on any CPU,
hardware will generate a bus lock, which is harmful to RT performance.
So when a RTVM is setup in the scenario, pstate/cstate is disabled
on all VMs.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
misc/config_tools/static_allocators/pm.py | 43 +++++++++++++++++++++++
misc/config_tools/xforms/misc_cfg.h.xsl | 6 ++++
2 files changed, 49 insertions(+)
create mode 100644 misc/config_tools/static_allocators/pm.py

diff --git a/misc/config_tools/static_allocators/pm.py b/misc/config_tools/static_allocators/pm.py
new file mode 100644
index 000000000..f99d34513
--- /dev/null
+++ b/misc/config_tools/static_allocators/pm.py
@@ -0,0 +1,43 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2021 Intel Corporation. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-3-Clause
+#
+
+import sys, os
+sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', 'library'))
+import common
+
+# post vms can have pstate/cstate on its vCPUs if those conditions are met:
+# 1.no share on the pCPUs
+# 2.the scenario has no rtvm(pre or post)
+# returns vm px/cx capability bit mask
+def get_post_vm_px_cx_capability(scenario_etree):
+ if common.get_node("//vm[vm_type = 'PRE_RT_VM']", scenario_etree) is not None:
+ return 0
+
+ if common.get_node("//vm[vm_type = 'POST_RT_VM']", scenario_etree) is not None:
+ return 0
+
+ all_cpus = scenario_etree.xpath("//vm/cpu_affinity/pcpu_id/text()")
+ post_vm_nodes = scenario_etree.xpath("//vm[vm_type = 'POST_STD_VM']")
+ vm_px_cx_mask = 0
+ for vm_node in post_vm_nodes:
+ vm_id = vm_node.get('id')
+ vm_cpus = vm_node.xpath('cpu_affinity/pcpu_id/text()')
+ vm_cpu_share = 0
+ for x in vm_cpus:
+ if all_cpus.count(x) != 1:
+ vm_cpu_share = 1
+ break
+ if vm_cpu_share == 0:
+ vm_px_cx_mask |= (1 << int(vm_id))
+
+ return vm_px_cx_mask
+
+def fn(board_etree, scenario_etree, allocation_etree):
+ vm_px_cx_cap = get_post_vm_px_cx_capability(scenario_etree)
+
+ common.append_node("/acrn-config/hv/pm/vm_px_cx_capability", hex(vm_px_cx_cap), allocation_etree)
+
diff --git a/misc/config_tools/xforms/misc_cfg.h.xsl b/misc/config_tools/xforms/misc_cfg.h.xsl
index 7ae8c1866..d97c9b6cb 100644
--- a/misc/config_tools/xforms/misc_cfg.h.xsl
+++ b/misc/config_tools/xforms/misc_cfg.h.xsl
@@ -26,6 +26,8 @@

<xsl:apply-templates select="allocation-data//ssram" />

+ <xsl:apply-templates select="allocation-data//hv/pm" />
+
<xsl:value-of select="acrn:include-guard-end('MISC_CFG_H')" />
</xsl:template>

@@ -52,6 +54,10 @@
<xsl:value-of select="acrn:define('PRE_RTVM_SW_SRAM_END_GPA', end_gpa, 'UL')" />
</xsl:template>

+ <xsl:template match="allocation-data//hv/pm">
+ <xsl:value-of select="acrn:define('VM_PX_CX_CAPABILITY', vm_px_cx_capability, 'UL')" />
+ </xsl:template>
+
<xsl:template match="BLOCK_DEVICE_INFO">
<xsl:variable name="block_devices_list_1" select="translate(current(), $newline, ',')" />
<xsl:variable name="block_devices_list_2" select="translate($block_devices_list_1, $whitespaces, '')" />
--
2.17.1


[PATCH 2/4] hv: remove pstate/cstate support for VMs that share pCPU

Zhou, Wu
 

Only when the a post-launched VM is not sharing pCPUs and no RTVM
is setup in the scenario, the pCPU’s pstate/cstate can be
passed-though to the VM.

The VM_PX_CX_CAPABILITY macro is generated by config_tools, this
patch simply use it to determine whether a VM can have pstate/cstate.
If it is negative, the px_cnt and cx_cnt will be zero. The dm will
also get 0 for px_cnt and cx_cnt on hypercall, and the VM will get
no pstate/cstate.

Signed-off-by: Zhou, Wu <wu.zhou@...>
---
hypervisor/arch/x86/guest/pm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hypervisor/arch/x86/guest/pm.c b/hypervisor/arch/x86/guest/pm.c
index 9771dda72..acf12bbcb 100644
--- a/hypervisor/arch/x86/guest/pm.c
+++ b/hypervisor/arch/x86/guest/pm.c
@@ -59,7 +59,8 @@ static void vm_setup_cpu_px(struct acrn_vm *vm)
vm->pm.px_cnt = 0U;
(void)memset(vm->pm.px_data, 0U, MAX_PSTATE * sizeof(struct acrn_pstate_data));

- if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL)) {
+ if ((pm_state_info->px_cnt != 0U) && (pm_state_info->px_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->px_cnt <= MAX_PSTATE), "failed to setup cpu px");

vm->pm.px_cnt = pm_state_info->px_cnt;
@@ -76,7 +77,8 @@ static void vm_setup_cpu_cx(struct acrn_vm *vm)
vm->pm.cx_cnt = 0U;
(void)memset(vm->pm.cx_data, 0U, MAX_CSTATE * sizeof(struct acrn_cstate_data));

- if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL)) {
+ if ((pm_state_info->cx_cnt != 0U) && (pm_state_info->cx_data != NULL) &&
+ (((1UL << vm->vm_id) & VM_PX_CX_CAPABILITY) != 0)) {
ASSERT((pm_state_info->cx_cnt <= MAX_CX_ENTRY), "failed to setup cpu cx");

vm->pm.cx_cnt = pm_state_info->cx_cnt;
--
2.17.1


[PATCH 0/4] Fix post-launched VM pstate/cstate bugs

Zhou, Wu
 

Currently acrn hypervisor doesn’t manage vm’s pstate/cstate. And The
pstate/cstate is passed-through to all post-launched Vms. Since the
VMs could share pCPUs, they could operate on the same px/cx resources,
this is a bug.

So only when the a post-launched VM’s vCPUs is not sharing pCPUs, the
pCPU’s pstate/cstate can be passed-though to the vm.

By using the VM’s CPU affinity info, patch 1/4 generates a vm
pstate/cstate capability macro VM_PX_CX_CAPABILITY in config_tools,
so that dm can use it (through hypercall, it will return 0 for px_cnt/cx_cnt)
to determine whether the VMs can have pstate/cstate.

Another thing is that when pstate/cstate changes on any CPU,
hardware will generate a bus lock, which is harmful to RT performance.
So when a RTVM is setup in the scenario, pstate/cstate is disabled
on all VMs.

If pstate is not given to a guest, linux kernel would print error msg:
'ACPI Error: AE_NOT_FOUND, Evaluating _PSS'
This is caused by hard-coded _PPC/_PCT in acrn-dm. Fixed in 4/4

*** BLURB HERE ***

Helmut Buchsbaum (1):
hv: hypercall: return 0 for empty PX or CX tables

Zhou, Wu (3):
config_tools: generate vm pstate/cstate capability macro
hv: remove pstate/cstate support for VMs that share pCPU
dm: Skip injecting _PPC and _PCT when _PSS is not constructed

devicemodel/hw/platform/acpi/acpi_pm.c | 45 +++++++++++++----------
hypervisor/arch/x86/guest/pm.c | 6 ++-
hypervisor/common/hypercall.c | 8 +---
misc/config_tools/static_allocators/pm.py | 43 ++++++++++++++++++++++
misc/config_tools/xforms/misc_cfg.h.xsl | 6 +++
5 files changed, 80 insertions(+), 28 deletions(-)
create mode 100644 misc/config_tools/static_allocators/pm.py

--
2.17.1


Re: [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

Yu Wang
 

On Tue, Nov 16, 2021 at 01:45:16PM +0800, Yu Wang wrote:
On Tue, Nov 16, 2021 at 02:48:38AM +0000, Eddie Dong wrote:

The CONFIG_MAX_PCI_BUSNO is generated by configuration tool which is
used for describe the native PCI bus number, right?
No, it's defficult for the configuration tool to discover the VF's bus number.
Why? We can not let user to input. It is too complicated and it requires too
much knowledge.

I think this is 2 different things: 1) The MACRO name, 2) The message the offline tool shows to developers.


#1 is completely internal stuff. We can use whatever it is. But a good name helps the developer.
CONFIG_IOMMU_BUS_NUM is the current one, and I second Anthony, we can live with it.
OK, we will keep the current one.


For #2, it is not only a word, but it is a sentence we can hint to developers. I am fine for whatever sentence to use here.
This one is needed actually. It needs to be removed. The
typo... This one is needn't actually. We can remove it and let it equal
to the max physical bus number.

CONFIG_IOMMU_BUS_NUM should equal to the max physical bus number which
can be set by configuration tool.





As all PCI devices have to go through IOMMU for security isolation,
then we can remove the CONFIG_MAX_IOMMU_ROOT_TABLE_NUM and
use the
CONFIG_MAX_PCI_BUSNO directly?
yes







Re: [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

Yu Wang
 

On Tue, Nov 16, 2021 at 02:48:38AM +0000, Eddie Dong wrote:

The CONFIG_MAX_PCI_BUSNO is generated by configuration tool which is
used for describe the native PCI bus number, right?
No, it's defficult for the configuration tool to discover the VF's bus number.
Why? We can not let user to input. It is too complicated and it requires too
much knowledge.

I think this is 2 different things: 1) The MACRO name, 2) The message the offline tool shows to developers.


#1 is completely internal stuff. We can use whatever it is. But a good name helps the developer.
CONFIG_IOMMU_BUS_NUM is the current one, and I second Anthony, we can live with it.
OK, we will keep the current one.


For #2, it is not only a word, but it is a sentence we can hint to developers. I am fine for whatever sentence to use here.
This one is needed actually. It needs to be removed. The
CONFIG_IOMMU_BUS_NUM should equal to the max physical bus number which
can be set by configuration tool.





As all PCI devices have to go through IOMMU for security isolation,
then we can remove the CONFIG_MAX_IOMMU_ROOT_TABLE_NUM and
use the
CONFIG_MAX_PCI_BUSNO directly?
yes




Re: [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

Yu Wang
 

On Tue, Nov 16, 2021 at 04:33:14AM +0000, Xu, Anthony wrote:


-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Yu Wang
Sent: Monday, November 15, 2021 5:42 PM
To: Li, Fei1 <fei1.li@...>
Cc: Xu, Anthony <anthony.xu@...>; acrn-dev@...; Liu, Long <long.liu@...>;
yuanyuan.zhao@...
Subject: Re: [acrn-dev] [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

On Tue, Nov 16, 2021 at 09:46:58AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 09:10:11AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 09:27:57AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 09:02:32AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 09:16:26AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 08:25:38AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 05:21:32AM +0800, Xu, Anthony wrote:
One context table corresponds to one pci bus. BUS_NUM is easy to understand to me.
BUS_NUM is used to save some memory.
Someone may confuse about the "IOMMU_BUS"..
How about CONFIG_MAX_IOMMU_ROOT_TABLE_NUM?
How about CONFIG_MAX_PCI_BUSNO ? Then define MAX_IOMMU_ROOT_TABLE_NUM to
CONFIG_MAX_PCI_BUSNO.
In this way, we could hide the VT-D from the common user.
The CONFIG_MAX_PCI_BUSNO is generated by configuration tool which is
used for describe the native PCI bus number, right?
No, it's defficult for the configuration tool to discover the VF's bus number.
Why? We can not let user to input. It is too complicated and it requires
too much knowledge.
The SRIOV is not enabled by default. So these VFs are not aviable until the user who want to use them.
The configuration tool could discover "The Secondary Bus Number" under a type 1 PCI device,
which means the configuration tool could discover the MAX_PCI_BUSNO in theory. But it could not
discover the MAX_PCI_BUSNO in real world.
Through the TotalVFs, First VF Offset, VF Stride, the number of BUS can
be cacualte directly, right?
Likely VFs is enabled in linux, can we find out how linux calculate it?
We can use the same method in configuration tools, and make it configurable just in case .
I think the algorithm has already defined in the SR-IOV spec. We can
follow the spec.



Anthony





Even it can not, we can let board inspector tool to enable the max VFs
then scan the max bus number.


For us, we could use configuration tool to define MAX_PCI_BUSNO to the theoretical value.
than would waste a little memory. We also could define MAX_PCI_BUSNO to 255 by default,
that would waster more memory, but it could simplfy the logic of configuration tool.
The value can't use a theoretical value, otherwise it may cause security
issue if the value is smaller than the real bus number.


Besides, I don't think PCI BUS Number is to difficult to understand, even for a common user.
The user could change the value if she/him how to change it, otherwise, the default value 255
is always works.
The IOMMU bus number can be removed and enforce to equal to the PCI bus
number. The open is how to decide the PCI bus number. It should not let
user to input which cause bad DX.



As all PCI devices have to go through IOMMU for security isolation, then
we can remove the CONFIG_MAX_IOMMU_ROOT_TABLE_NUM and use the
CONFIG_MAX_PCI_BUSNO directly?
yes



Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhao, Yuanyuan
Sent: Sunday, November 14, 2021 7:24 PM
To: Wang, Yu1 <yu1.wang@...>; Li, Fei1 <fei1.li@...>; Liu, Long <long.liu@...>;
acrn-dev@...
Cc: Yuanyuan Zhao <yuanyuan.zhao@...>
Subject: [acrn-dev] [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

`CONFIG_IOMMU_BUS_NUM` is not precise enough. As `struct context_table`
contains all context_table which elements in root table point to one-to-one.
So rename `CONFIG_IOMMU_BUS_NUM` to `CONFIG_IOMMU_ROOT_TABLE_CAPACITY`.

Signed-off-by: Yuanyuan Zhao <yuanyuan.zhao@...>
---
hypervisor/arch/x86/vtd.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
index db480d483..271662584 100644
--- a/hypervisor/arch/x86/vtd.c
+++ b/hypervisor/arch/x86/vtd.c
@@ -24,6 +24,9 @@
#include <pci.h>
#include <asm/platform_caps.h>

+/* temporary for compile use */
+#define CONFIG_IOMMU_ROOT_TABLE_CAPACITY 0x100
+
#define DBG_IOMMU 0

#if DBG_IOMMU
@@ -148,7 +151,7 @@ struct dmar_drhd_rt {
};

struct context_table {
- struct page buses[CONFIG_IOMMU_BUS_NUM];
+ struct page buses[CONFIG_IOMMU_ROOT_TABLE_CAPACITY];
};

struct intr_remap_table {
@@ -1000,7 +1003,7 @@ static bool is_dmar_unit_valid(const struct dmar_drhd_rt *dmar_unit, union pci_b
return valid;
}

-/* @pre bus < CONFIG_IOMMU_BUS_NUM */
+/* @pre bus < CONFIG_IOMMU_ROOT_TABLE_CAPACITY */
static int32_t iommu_attach_device(const struct iommu_domain *domain, uint8_t bus, uint8_t devfun)
{
struct dmar_drhd_rt *dmar_unit;
@@ -1079,7 +1082,7 @@ static int32_t iommu_attach_device(const struct iommu_domain *domain, uint8_t
bu
return ret;
}

-/* @pre bus < CONFIG_IOMMU_BUS_NUM */
+/* @pre bus < CONFIG_IOMMU_ROOT_TABLE_CAPACITY */
static int32_t iommu_detach_device(const struct iommu_domain *domain, uint8_t bus, uint8_t devfun)
{
struct dmar_drhd_rt *dmar_unit;
@@ -1202,7 +1205,7 @@ int32_t move_pt_device(const struct iommu_domain *from_domain, const struct
iomm

/* TODO: check if the device assigned */

- if (bus_local < CONFIG_IOMMU_BUS_NUM) {
+ if (bus_local < CONFIG_IOMMU_ROOT_TABLE_CAPACITY) {
if (from_domain != NULL) {
status = iommu_detach_device(from_domain, bus, devfun);
}
--
2.17.1











Re: [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

Xu, Anthony
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Yu Wang
Sent: Monday, November 15, 2021 5:42 PM
To: Li, Fei1 <fei1.li@...>
Cc: Xu, Anthony <anthony.xu@...>; acrn-dev@...; Liu, Long <long.liu@...>;
yuanyuan.zhao@...
Subject: Re: [acrn-dev] [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

On Tue, Nov 16, 2021 at 09:46:58AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 09:10:11AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 09:27:57AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 09:02:32AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 09:16:26AM +0800, Li Fei1 wrote:
On Tue, Nov 16, 2021 at 08:25:38AM +0800, Wang, Yu1 wrote:
On Tue, Nov 16, 2021 at 05:21:32AM +0800, Xu, Anthony wrote:
One context table corresponds to one pci bus. BUS_NUM is easy to understand to me.
BUS_NUM is used to save some memory.
Someone may confuse about the "IOMMU_BUS"..
How about CONFIG_MAX_IOMMU_ROOT_TABLE_NUM?
How about CONFIG_MAX_PCI_BUSNO ? Then define MAX_IOMMU_ROOT_TABLE_NUM to
CONFIG_MAX_PCI_BUSNO.
In this way, we could hide the VT-D from the common user.
The CONFIG_MAX_PCI_BUSNO is generated by configuration tool which is
used for describe the native PCI bus number, right?
No, it's defficult for the configuration tool to discover the VF's bus number.
Why? We can not let user to input. It is too complicated and it requires
too much knowledge.
The SRIOV is not enabled by default. So these VFs are not aviable until the user who want to use them.
The configuration tool could discover "The Secondary Bus Number" under a type 1 PCI device,
which means the configuration tool could discover the MAX_PCI_BUSNO in theory. But it could not
discover the MAX_PCI_BUSNO in real world.
Through the TotalVFs, First VF Offset, VF Stride, the number of BUS can
be cacualte directly, right?
Likely VFs is enabled in linux, can we find out how linux calculate it?
We can use the same method in configuration tools, and make it configurable just in case .


Anthony





Even it can not, we can let board inspector tool to enable the max VFs
then scan the max bus number.


For us, we could use configuration tool to define MAX_PCI_BUSNO to the theoretical value.
than would waste a little memory. We also could define MAX_PCI_BUSNO to 255 by default,
that would waster more memory, but it could simplfy the logic of configuration tool.
The value can't use a theoretical value, otherwise it may cause security
issue if the value is smaller than the real bus number.


Besides, I don't think PCI BUS Number is to difficult to understand, even for a common user.
The user could change the value if she/him how to change it, otherwise, the default value 255
is always works.
The IOMMU bus number can be removed and enforce to equal to the PCI bus
number. The open is how to decide the PCI bus number. It should not let
user to input which cause bad DX.



As all PCI devices have to go through IOMMU for security isolation, then
we can remove the CONFIG_MAX_IOMMU_ROOT_TABLE_NUM and use the
CONFIG_MAX_PCI_BUSNO directly?
yes



Anthony

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Zhao, Yuanyuan
Sent: Sunday, November 14, 2021 7:24 PM
To: Wang, Yu1 <yu1.wang@...>; Li, Fei1 <fei1.li@...>; Liu, Long <long.liu@...>;
acrn-dev@...
Cc: Yuanyuan Zhao <yuanyuan.zhao@...>
Subject: [acrn-dev] [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

`CONFIG_IOMMU_BUS_NUM` is not precise enough. As `struct context_table`
contains all context_table which elements in root table point to one-to-one.
So rename `CONFIG_IOMMU_BUS_NUM` to `CONFIG_IOMMU_ROOT_TABLE_CAPACITY`.

Signed-off-by: Yuanyuan Zhao <yuanyuan.zhao@...>
---
hypervisor/arch/x86/vtd.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
index db480d483..271662584 100644
--- a/hypervisor/arch/x86/vtd.c
+++ b/hypervisor/arch/x86/vtd.c
@@ -24,6 +24,9 @@
#include <pci.h>
#include <asm/platform_caps.h>

+/* temporary for compile use */
+#define CONFIG_IOMMU_ROOT_TABLE_CAPACITY 0x100
+
#define DBG_IOMMU 0

#if DBG_IOMMU
@@ -148,7 +151,7 @@ struct dmar_drhd_rt {
};

struct context_table {
- struct page buses[CONFIG_IOMMU_BUS_NUM];
+ struct page buses[CONFIG_IOMMU_ROOT_TABLE_CAPACITY];
};

struct intr_remap_table {
@@ -1000,7 +1003,7 @@ static bool is_dmar_unit_valid(const struct dmar_drhd_rt *dmar_unit, union pci_b
return valid;
}

-/* @pre bus < CONFIG_IOMMU_BUS_NUM */
+/* @pre bus < CONFIG_IOMMU_ROOT_TABLE_CAPACITY */
static int32_t iommu_attach_device(const struct iommu_domain *domain, uint8_t bus, uint8_t devfun)
{
struct dmar_drhd_rt *dmar_unit;
@@ -1079,7 +1082,7 @@ static int32_t iommu_attach_device(const struct iommu_domain *domain, uint8_t
bu
return ret;
}

-/* @pre bus < CONFIG_IOMMU_BUS_NUM */
+/* @pre bus < CONFIG_IOMMU_ROOT_TABLE_CAPACITY */
static int32_t iommu_detach_device(const struct iommu_domain *domain, uint8_t bus, uint8_t devfun)
{
struct dmar_drhd_rt *dmar_unit;
@@ -1202,7 +1205,7 @@ int32_t move_pt_device(const struct iommu_domain *from_domain, const struct
iomm

/* TODO: check if the device assigned */

- if (bus_local < CONFIG_IOMMU_BUS_NUM) {
+ if (bus_local < CONFIG_IOMMU_ROOT_TABLE_CAPACITY) {
if (from_domain != NULL) {
status = iommu_detach_device(from_domain, bus, devfun);
}
--
2.17.1







Re: [PATCH] hv: rename CONFIG_IOMMU_BUS_NUM

Eddie Dong
 


The CONFIG_MAX_PCI_BUSNO is generated by configuration tool which is
used for describe the native PCI bus number, right?
No, it's defficult for the configuration tool to discover the VF's bus number.
Why? We can not let user to input. It is too complicated and it requires too
much knowledge.

I think this is 2 different things: 1) The MACRO name, 2) The message the offline tool shows to developers.


#1 is completely internal stuff. We can use whatever it is. But a good name helps the developer.
CONFIG_IOMMU_BUS_NUM is the current one, and I second Anthony, we can live with it.

For #2, it is not only a word, but it is a sentence we can hint to developers. I am fine for whatever sentence to use here.




As all PCI devices have to go through IOMMU for security isolation,
then we can remove the CONFIG_MAX_IOMMU_ROOT_TABLE_NUM and
use the
CONFIG_MAX_PCI_BUSNO directly?
yes

3281 - 3300 of 37344