[PATCH V7 3/5] hv: vCAT: implementing the vCAT MSRs read handlers


Dongsheng Zhang
 

From: dongshen <dongsheng.x.zhang@...>

Implement the read_vcbm_msr() and read_vclosid() functions to handle the MSR_IA32_PQR_ASSOC
and MSR_IA32_type_MASK_n vCAT MSRs read request.

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 63 ++++++++++++++++++++
hypervisor/arch/x86/guest/vmsr.c | 13 ++++
hypervisor/include/arch/x86/asm/guest/vcat.h | 2 +
3 files changed, 78 insertions(+)

diff --git a/hypervisor/arch/x86/guest/vcat.c b/hypervisor/arch/x86/guest/vcat.c
index 388d2801d..577917e20 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -167,6 +167,50 @@ uint64_t vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int res)
return (pcbm & max_pcbm) >> low;
}

+/**
+ * @pre vm != NULL
+ */
+static bool is_l2_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr)
+{
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L2)->num_closids > 0U)
+ && (vmsr >= MSR_IA32_L2_MASK_BASE) && (vmsr < (MSR_IA32_L2_MASK_BASE + num_vcbm_msrs)));
+}
+
+/**
+ * @pre vm != NULL
+ */
+static bool is_l3_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr)
+{
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L3)->num_closids > 0U)
+ && (vmsr >= MSR_IA32_L3_MASK_BASE) && (vmsr < (MSR_IA32_L3_MASK_BASE + num_vcbm_msrs)));
+}
+
+/**
+ * @brief vCBM MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL && rval != NULL
+ */
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rval)
+{
+ int ret = -EACCES;
+ struct acrn_vm *vm = vcpu->vm;
+
+ if (is_vcat_configured(vm) && (is_l2_vcbm_msr(vm, vmsr) || is_l3_vcbm_msr(vm, vmsr))) {
+ *rval = vcpu_get_guest_msr(vcpu, vmsr);
+ ret = 0;
+ } else {
+ *rval = 0UL;
+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
@@ -183,6 +227,25 @@ static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused uint32_t vms
return -EFAULT;
}

+/**
+ * @brief vCLOSID MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL
+ */
+int32_t read_vclosid(const struct acrn_vcpu *vcpu, uint64_t *rval)
+{
+ int ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ *rval = vcpu_get_guest_msr(vcpu, MSR_IA32_PQR_ASSOC);
+ ret = 0;
+ } else {
+ *rval = 0UL;
+ }
+
+ return ret;
+}
+
/**
* @brief vCLOSID MSR write handler
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c b/hypervisor/arch/x86/guest/vmsr.c
index f5e611fcb..83eaed09e 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -703,6 +703,19 @@ int32_t rdmsr_vmexit_handler(struct acrn_vcpu *vcpu)
}
break;
}
+#ifdef CONFIG_VCAT_ENABLED
+ case MSR_IA32_L2_MASK_BASE ... (MSR_IA32_L2_MASK_BASE + NUM_VCAT_L2_MSRS - 1U):
+ case MSR_IA32_L3_MASK_BASE ... (MSR_IA32_L3_MASK_BASE + NUM_VCAT_L3_MSRS - 1U):
+ {
+ err = read_vcbm_msr(vcpu, msr, &v);
+ break;
+ }
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = read_vclosid(vcpu, &v);
+ break;
+ }
+#endif
default:
{
if (is_x2apic_msr(msr)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vcat.h b/hypervisor/include/arch/x86/asm/guest/vcat.h
index a9518bded..318d33d68 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -15,6 +15,8 @@ uint16_t vcat_get_vcbm_len(const struct acrn_vm *vm, int res);
void init_vcat_msrs(struct acrn_vcpu *vcpu);
uint16_t vcat_get_num_vclosids(const struct acrn_vm *vm);
uint64_t vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int res);
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rval);
+int32_t read_vclosid(const struct acrn_vcpu *vcpu, uint64_t *rval);

#endif /* VCAT_H_ */

--
2.25.1


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Sunday, October 24, 2021 5:39 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT MSRs
read handlers

From: dongshen <dongsheng.x.zhang@...>

Implement the read_vcbm_msr() and read_vclosid() functions to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs read
request.

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 63
++++++++++++++++++++
hypervisor/arch/x86/guest/vmsr.c | 13 ++++
hypervisor/include/arch/x86/asm/guest/vcat.h | 2 +
3 files changed, 78 insertions(+)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 388d2801d..577917e20 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -167,6 +167,50 @@ uint64_t vcat_pcbm_to_vcbm(const struct acrn_vm
*vm, uint64_t pcbm, int res)
return (pcbm & max_pcbm) >> low;
}

+/**
+ * @pre vm != NULL
+ */
+static bool is_l2_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L2)->num_closids > 0U)
+ && (vmsr >= MSR_IA32_L2_MASK_BASE) && (vmsr <
(MSR_IA32_L2_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @pre vm != NULL
+ */
+static bool is_l3_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L3)->num_closids > 0U)
+ && (vmsr >= MSR_IA32_L3_MASK_BASE) && (vmsr <
(MSR_IA32_L3_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @brief vCBM MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL && rval != NULL */ int32_t
+read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
+*rval) {
+ int ret = -EACCES;
+ struct acrn_vm *vm = vcpu->vm;
+
+ if (is_vcat_configured(vm) && (is_l2_vcbm_msr(vm, vmsr) ||
is_l3_vcbm_msr(vm, vmsr))) {
+ *rval = vcpu_get_guest_msr(vcpu, vmsr);
+ ret = 0;
+ } else {
+ *rval = 0UL;
What is the purpose of this?

+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
@@ -183,6 +227,25 @@ static int32_t write_vcbm(__unused struct
acrn_vcpu *vcpu, __unused uint32_t vms
return -EFAULT;
}

+/**
+ * @brief vCLOSID MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t read_vclosid(const
+struct acrn_vcpu *vcpu, uint64_t *rval) {
+ int ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ *rval = vcpu_get_guest_msr(vcpu, MSR_IA32_PQR_ASSOC);
+ ret = 0;
+ } else {
+ *rval = 0UL;
Ditto


The rest is fine.

+ }
+
+ return ret;
+}
+
/**
* @brief vCLOSID MSR write handler
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index f5e611fcb..83eaed09e 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -703,6 +703,19 @@ int32_t rdmsr_vmexit_handler(struct acrn_vcpu
*vcpu)
}
break;
}
+#ifdef CONFIG_VCAT_ENABLED
+ case MSR_IA32_L2_MASK_BASE ... (MSR_IA32_L2_MASK_BASE +
NUM_VCAT_L2_MSRS - 1U):
+ case MSR_IA32_L3_MASK_BASE ... (MSR_IA32_L3_MASK_BASE +
NUM_VCAT_L3_MSRS - 1U):
+ {
+ err = read_vcbm_msr(vcpu, msr, &v);
+ break;
+ }
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = read_vclosid(vcpu, &v);
+ break;
+ }
+#endif
default:
{
if (is_x2apic_msr(msr)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vcat.h
b/hypervisor/include/arch/x86/asm/guest/vcat.h
index a9518bded..318d33d68 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -15,6 +15,8 @@ uint16_t vcat_get_vcbm_len(const struct acrn_vm *vm,
int res); void init_vcat_msrs(struct acrn_vcpu *vcpu); uint16_t
vcat_get_num_vclosids(const struct acrn_vm *vm); uint64_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int res);
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t *rval); int32_t read_vclosid(const struct acrn_vcpu *vcpu,
+uint64_t *rval);

#endif /* VCAT_H_ */

--
2.25.1





Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 8:11 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Sunday, October 24, 2021 5:39 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers

From: dongshen <dongsheng.x.zhang@...>

Implement the read_vcbm_msr() and read_vclosid() functions to handle
the MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs read
request.

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 63
++++++++++++++++++++
hypervisor/arch/x86/guest/vmsr.c | 13 ++++
hypervisor/include/arch/x86/asm/guest/vcat.h | 2 +
3 files changed, 78 insertions(+)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 388d2801d..577917e20 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -167,6 +167,50 @@ uint64_t vcat_pcbm_to_vcbm(const struct
acrn_vm
*vm, uint64_t pcbm, int res)
return (pcbm & max_pcbm) >> low;
}

+/**
+ * @pre vm != NULL
+ */
+static bool is_l2_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L2)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L2_MASK_BASE) && (vmsr <
(MSR_IA32_L2_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @pre vm != NULL
+ */
+static bool is_l3_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L3)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L3_MASK_BASE) && (vmsr <
(MSR_IA32_L3_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @brief vCBM MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL && rval != NULL */ int32_t
+read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
+*rval) {
+ int ret = -EACCES;
+ struct acrn_vm *vm = vcpu->vm;
+
+ if (is_vcat_configured(vm) && (is_l2_vcbm_msr(vm, vmsr) ||
is_l3_vcbm_msr(vm, vmsr))) {
+ *rval = vcpu_get_guest_msr(vcpu, vmsr);
+ ret = 0;
+ } else {
+ *rval = 0UL;
What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better to also set the rval to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;" in rdmsr_vmexit_handler():
default:
{
if (is_x2apic_msr(msr)) {
err = vlapic_x2apic_read(vcpu, msr, &v);
} else if (is_vmx_msr(msr)) {
/*
* TODO: after the switch statement in this function, there is another
* switch statement inside read_vmx_msr(). Is it possible to reduce it
* to just one switch to improvement performance?
*/
err = read_vmx_msr(vcpu, msr, &v);
} else {
pr_warn("%s(): vm%d vcpu%d reading MSR %lx not supported",
__func__, vcpu->vm->vm_id, vcpu->vcpu_id, msr);
err = -EACCES;
v = 0UL;
}
break;
}


+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
@@ -183,6 +227,25 @@ static int32_t write_vcbm(__unused struct
acrn_vcpu *vcpu, __unused uint32_t vms
return -EFAULT;
}

+/**
+ * @brief vCLOSID MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+read_vclosid(const struct acrn_vcpu *vcpu, uint64_t *rval) {
+ int ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ *rval = vcpu_get_guest_msr(vcpu, MSR_IA32_PQR_ASSOC);
+ ret = 0;
+ } else {
+ *rval = 0UL;
Ditto
Don : see above comment, same logic.



The rest is fine.

+ }
+
+ return ret;
+}
+
/**
* @brief vCLOSID MSR write handler
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index f5e611fcb..83eaed09e 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -703,6 +703,19 @@ int32_t rdmsr_vmexit_handler(struct acrn_vcpu
*vcpu)
}
break;
}
+#ifdef CONFIG_VCAT_ENABLED
+ case MSR_IA32_L2_MASK_BASE ... (MSR_IA32_L2_MASK_BASE +
NUM_VCAT_L2_MSRS - 1U):
+ case MSR_IA32_L3_MASK_BASE ... (MSR_IA32_L3_MASK_BASE +
NUM_VCAT_L3_MSRS - 1U):
+ {
+ err = read_vcbm_msr(vcpu, msr, &v);
+ break;
+ }
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = read_vclosid(vcpu, &v);
+ break;
+ }
+#endif
default:
{
if (is_x2apic_msr(msr)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vcat.h
b/hypervisor/include/arch/x86/asm/guest/vcat.h
index a9518bded..318d33d68 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -15,6 +15,8 @@ uint16_t vcat_get_vcbm_len(const struct acrn_vm
*vm,
int res); void init_vcat_msrs(struct acrn_vcpu *vcpu); uint16_t
vcat_get_num_vclosids(const struct acrn_vm *vm); uint64_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int res);
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t *rval); int32_t read_vclosid(const struct acrn_vcpu *vcpu,
+uint64_t *rval);

#endif /* VCAT_H_ */

--
2.25.1








Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Monday, October 25, 2021 11:19 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 8:11 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Dongsheng Zhang
Sent: Sunday, October 24, 2021 5:39 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers

From: dongshen <dongsheng.x.zhang@...>

Implement the read_vcbm_msr() and read_vclosid() functions to handle
the MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs
read
request.

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 63
++++++++++++++++++++
hypervisor/arch/x86/guest/vmsr.c | 13 ++++
hypervisor/include/arch/x86/asm/guest/vcat.h | 2 +
3 files changed, 78 insertions(+)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 388d2801d..577917e20 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -167,6 +167,50 @@ uint64_t vcat_pcbm_to_vcbm(const struct
acrn_vm
*vm, uint64_t pcbm, int res)
return (pcbm & max_pcbm) >> low;
}

+/**
+ * @pre vm != NULL
+ */
+static bool is_l2_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L2)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L2_MASK_BASE) && (vmsr <
(MSR_IA32_L2_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @pre vm != NULL
+ */
+static bool is_l3_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr) {
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L3)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L3_MASK_BASE) && (vmsr <
(MSR_IA32_L3_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @brief vCBM MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL && rval != NULL */
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t
+*rval) {
+ int ret = -EACCES;
+ struct acrn_vm *vm = vcpu->vm;
+
+ if (is_vcat_configured(vm) && (is_l2_vcbm_msr(vm, vmsr) ||
is_l3_vcbm_msr(vm, vmsr))) {
+ *rval = vcpu_get_guest_msr(vcpu, vmsr);
+ ret = 0;
+ } else {
+ *rval = 0UL;
What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better to also set the rval
to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;" in
rdmsr_vmexit_handler():
Please response with evidence from SDM.


default:
{
if (is_x2apic_msr(msr)) {
err = vlapic_x2apic_read(vcpu, msr, &v);
} else if (is_vmx_msr(msr)) {
/*
* TODO: after the switch statement in this function, there is
another
* switch statement inside read_vmx_msr(). Is it possible to
reduce it
* to just one switch to improvement performance?
*/
err = read_vmx_msr(vcpu, msr, &v);
} else {
pr_warn("%s(): vm%d vcpu%d reading MSR %lx not supported",
__func__, vcpu->vm->vm_id, vcpu->vcpu_id, msr);
err = -EACCES;
v = 0UL;
}
break;
}


+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
@@ -183,6 +227,25 @@ static int32_t write_vcbm(__unused struct
acrn_vcpu *vcpu, __unused uint32_t vms
return -EFAULT;
}

+/**
+ * @brief vCLOSID MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+read_vclosid(const struct acrn_vcpu *vcpu, uint64_t *rval) {
+ int ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ *rval = vcpu_get_guest_msr(vcpu, MSR_IA32_PQR_ASSOC);
+ ret = 0;
+ } else {
+ *rval = 0UL;
Ditto
Don : see above comment, same logic.



The rest is fine.

+ }
+
+ return ret;
+}
+
/**
* @brief vCLOSID MSR write handler
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index f5e611fcb..83eaed09e 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -703,6 +703,19 @@ int32_t rdmsr_vmexit_handler(struct acrn_vcpu
*vcpu)
}
break;
}
+#ifdef CONFIG_VCAT_ENABLED
+ case MSR_IA32_L2_MASK_BASE ... (MSR_IA32_L2_MASK_BASE +
NUM_VCAT_L2_MSRS - 1U):
+ case MSR_IA32_L3_MASK_BASE ... (MSR_IA32_L3_MASK_BASE +
NUM_VCAT_L3_MSRS - 1U):
+ {
+ err = read_vcbm_msr(vcpu, msr, &v);
+ break;
+ }
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = read_vclosid(vcpu, &v);
+ break;
+ }
+#endif
default:
{
if (is_x2apic_msr(msr)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vcat.h
b/hypervisor/include/arch/x86/asm/guest/vcat.h
index a9518bded..318d33d68 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -15,6 +15,8 @@ uint16_t vcat_get_vcbm_len(const struct acrn_vm
*vm,
int res); void init_vcat_msrs(struct acrn_vcpu *vcpu); uint16_t
vcat_get_num_vclosids(const struct acrn_vm *vm); uint64_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int res);
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t *rval); int32_t read_vclosid(const struct acrn_vcpu *vcpu,
+uint64_t *rval);

#endif /* VCAT_H_ */

--
2.25.1











Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 8:42 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Monday, October 25, 2021 11:19 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 8:11 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the
vCAT MSRs read handlers



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Dongsheng Zhang
Sent: Sunday, October 24, 2021 5:39 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers

From: dongshen <dongsheng.x.zhang@...>

Implement the read_vcbm_msr() and read_vclosid() functions to
handle the MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT
MSRs
read
request.

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 63
++++++++++++++++++++
hypervisor/arch/x86/guest/vmsr.c | 13 ++++
hypervisor/include/arch/x86/asm/guest/vcat.h | 2 +
3 files changed, 78 insertions(+)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 388d2801d..577917e20 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -167,6 +167,50 @@ uint64_t vcat_pcbm_to_vcbm(const struct
acrn_vm
*vm, uint64_t pcbm, int res)
return (pcbm & max_pcbm) >> low; }

+/**
+ * @pre vm != NULL
+ */
+static bool is_l2_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr)
{
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L2)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L2_MASK_BASE) && (vmsr <
(MSR_IA32_L2_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @pre vm != NULL
+ */
+static bool is_l3_vcbm_msr(const struct acrn_vm *vm, uint32_t vmsr)
{
+ /* num_vcbm_msrs = num_vclosids */
+ uint16_t num_vcbm_msrs = vcat_get_num_vclosids(vm);
+
+ return ((get_rdt_res_cap_info(RDT_RESOURCE_L3)->num_closids >
0U)
+ && (vmsr >= MSR_IA32_L3_MASK_BASE) && (vmsr <
(MSR_IA32_L3_MASK_BASE
++ num_vcbm_msrs))); }
+
+/**
+ * @brief vCBM MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL && rval != NULL */
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t
+vmsr, uint64_t
+*rval) {
+ int ret = -EACCES;
+ struct acrn_vm *vm = vcpu->vm;
+
+ if (is_vcat_configured(vm) && (is_l2_vcbm_msr(vm, vmsr) ||
is_l3_vcbm_msr(vm, vmsr))) {
+ *rval = vcpu_get_guest_msr(vcpu, vmsr);
+ ret = 0;
+ } else {
+ *rval = 0UL;
What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better to also
set the rval to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;" in
rdmsr_vmexit_handler():
Please response with evidence from SDM.
Don: sorry, I did not see that from SDM so I cannot justify for myself, will just return -EACCES (and do not set *rval to 0UL).



default:
{
if (is_x2apic_msr(msr)) {
err = vlapic_x2apic_read(vcpu, msr, &v);
} else if (is_vmx_msr(msr)) {
/*
* TODO: after the switch statement in this function,
there is
another
* switch statement inside read_vmx_msr(). Is it
possible to reduce
it
* to just one switch to improvement performance?
*/
err = read_vmx_msr(vcpu, msr, &v);
} else {
pr_warn("%s(): vm%d vcpu%d reading MSR %lx not
supported",
__func__, vcpu->vm->vm_id, vcpu->vcpu_id,
msr);
err = -EACCES;
v = 0UL;
}
break;
}


+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
@@ -183,6 +227,25 @@ static int32_t write_vcbm(__unused struct
acrn_vcpu *vcpu, __unused uint32_t vms
return -EFAULT;
}

+/**
+ * @brief vCLOSID MSR read handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+read_vclosid(const struct acrn_vcpu *vcpu, uint64_t *rval) {
+ int ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ *rval = vcpu_get_guest_msr(vcpu, MSR_IA32_PQR_ASSOC);
+ ret = 0;
+ } else {
+ *rval = 0UL;
Ditto
Don : see above comment, same logic.



The rest is fine.

+ }
+
+ return ret;
+}
+
/**
* @brief vCLOSID MSR write handler
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index f5e611fcb..83eaed09e 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -703,6 +703,19 @@ int32_t rdmsr_vmexit_handler(struct
acrn_vcpu
*vcpu)
}
break;
}
+#ifdef CONFIG_VCAT_ENABLED
+ case MSR_IA32_L2_MASK_BASE ... (MSR_IA32_L2_MASK_BASE +
NUM_VCAT_L2_MSRS - 1U):
+ case MSR_IA32_L3_MASK_BASE ... (MSR_IA32_L3_MASK_BASE +
NUM_VCAT_L3_MSRS - 1U):
+ {
+ err = read_vcbm_msr(vcpu, msr, &v);
+ break;
+ }
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = read_vclosid(vcpu, &v);
+ break;
+ }
+#endif
default:
{
if (is_x2apic_msr(msr)) {
diff --git a/hypervisor/include/arch/x86/asm/guest/vcat.h
b/hypervisor/include/arch/x86/asm/guest/vcat.h
index a9518bded..318d33d68 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -15,6 +15,8 @@ uint16_t vcat_get_vcbm_len(const struct acrn_vm
*vm,
int res); void init_vcat_msrs(struct acrn_vcpu *vcpu); uint16_t
vcat_get_num_vclosids(const struct acrn_vm *vm); uint64_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint64_t pcbm, int
res);
+int32_t read_vcbm_msr(const struct acrn_vcpu *vcpu, uint32_t
+vmsr, uint64_t *rval); int32_t read_vclosid(const struct
+acrn_vcpu *vcpu, uint64_t *rval);

#endif /* VCAT_H_ */

--
2.25.1














Eddie Dong
 

What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better to
also set the rval to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;"
in
rdmsr_vmexit_handler():
Please response with evidence from SDM.
Don: sorry, I did not see that from SDM so I cannot justify for myself, will just
return -EACCES (and do not set *rval to 0UL).
Don't do arbitrary changes. All what we do must have reason to do...


Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 10:15 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers

What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better to
also set the rval to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;"
in
rdmsr_vmexit_handler():
Please response with evidence from SDM.
Don: sorry, I did not see that from SDM so I cannot justify for
myself, will just return -EACCES (and do not set *rval to 0UL).
Don't do arbitrary changes. All what we do must have reason to do...
Don: I am not an expert on that, so just follow the norm. Do you have any suggestion on this change (or just keep my current code if not sure)?








Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Monday, October 25, 2021 1:37 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Eddie Dong
Sent: Sunday, October 24, 2021 10:15 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V7 3/5] hv: vCAT: implementing the vCAT
MSRs read handlers

What is the purpose of this?
Don:
I think for read access, when ret is not zero (failed), better
to also set the rval to 0 and return this to guest vm?
This is in sync with the following lines "err = -EACCES; v = 0UL;"
in
rdmsr_vmexit_handler():
Please response with evidence from SDM.
Don: sorry, I did not see that from SDM so I cannot justify for
myself, will just return -EACCES (and do not set *rval to 0UL).
Don't do arbitrary changes. All what we do must have reason to do...
Don: I am not an expert on that, so just follow the norm. Do you have any
suggestion on this change (or just keep my current code if not sure)?
Don't do arbitrary code. Remove them if there is no reason to keep them.