[PATCH V6 7/8] hv: vCAT: implementing the vCAT MSRs write handler


Dongsheng Zhang
 

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

Implement the write_vcat_msr() function to handle the MSR_IA32_PQR_ASSOC
and MSR_IA32_type_MASK_n vCAT MSRs write request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 68 ++++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 +++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c b/hypervisor/arch/x86/guest/vcat.c
index 1901ba9f3..b15413356 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,25 @@ int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm, int res)
+{
+ uint32_t max_pcbm = get_max_pcbm(vm, res);
+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64((uint64_t)max_pcbm);
+
+ /*
+ * Mask off the unwanted bits to prevent erroneous mask values,
+ * pcbm set bits should only be in the range of [low, high]
+ */
+ return (vcbm << low) & max_pcbm;
+}
+
/**
* @brief Map vMSR to pMSR
* Each vMSR or pMSR is identified by a 32-bit integer
@@ -395,19 +414,32 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint32_t pcbm)
+{
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) | pcbm;
+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit, will perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
+ pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL), res);
+ write_pcbm(pmsr, pcbm);
+
+ return 0;
}

/**
@@ -431,6 +463,28 @@ static int32_t write_vclosid(struct acrn_vcpu *vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL
+ */
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val)
+{
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L3);
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c b/hypervisor/arch/x86/guest/vmsr.c
index 8d70acf3d..eef069cb1 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1090,6 +1090,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 68c813f83..968a183ed 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ void init_vcat_msrs(struct acrn_vcpu *vcpu);
uint16_t vcat_get_num_vclosids(const struct acrn_vm *vm);
uint32_t vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint32_t pcbm, int res);
int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 6:49 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT MSRs
write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical) mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways assigned to
the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 68
++++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 +++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 1901ba9f3..b15413356 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,25 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm,
+int res) {
+ uint32_t max_pcbm = get_max_pcbm(vm, res);
This remind me: Is CBM 64 bits, or 32 bits?
Does SDM define?

+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64((uint64_t)max_pcbm);
+
+ /*
+ * Mask off the unwanted bits to prevent erroneous mask values,
+ * pcbm set bits should only be in the range of [low, high]
+ */
+ return (vcbm << low) & max_pcbm;
+}
+
/**
* @brief Map vMSR to pMSR
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19 +414,32
@@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr,
int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint32_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) |
pcbm;
What in my mind is that the guest has a value. It should be masked by "-1" per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those guest owned bits).
Or, if you are talking about the reserved bits in pCBM side, this should be a fixed pattern, and this pattern can be got at PCPU boot time.
In this case, it can be much simpler.

+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit, will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
This is address, not pmsr.

+ pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL), res);
I am not sure here. Will see the answer of previous comments.

+ write_pcbm(pmsr, pcbm);
+
After the set, we need to update the auto-save MSR list.

+ return 0;
}

/**
@@ -431,6 +463,28 @@ static int32_t write_vclosid(struct acrn_vcpu *vcpu,
uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L3);
+ }
+ }
Given that the caller side already "switch case" it, we can avoid double effort here (if..else). How about to do in caller side.

+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 8d70acf3d..eef069cb1 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1090,6 +1090,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 68c813f83..968a183ed 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ void init_vcat_msrs(struct acrn_vcpu *vcpu); uint16_t
vcat_get_num_vclosids(const struct acrn_vm *vm); uint32_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint32_t pcbm, int res);
int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
*rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
+val);

#endif /* VCAT_H_ */

--
2.25.1





Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 17, 2021 10:16 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 6:49 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways
assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 68
++++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 +++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 1901ba9f3..b15413356 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,25 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm,
+int res) {
+ uint32_t max_pcbm = get_max_pcbm(vm, res);
This remind me: Is CBM 64 bits, or 32 bits?
Does SDM define?
Don: it is 32-bit, higher 32-bit are reserved



+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64((uint64_t)max_pcbm);
+
+ /*
+ * Mask off the unwanted bits to prevent erroneous mask values,
+ * pcbm set bits should only be in the range of [low, high]
+ */
+ return (vcbm << low) & max_pcbm;
+}
+
/**
* @brief Map vMSR to pMSR
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19
+414,32 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm,
uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint32_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) |
pcbm;
What in my mind is that the guest has a value. It should be masked by "-1"
per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those guest owned
bits).
Yes, I only did that for pcbm (but did not do that for vcbm):
the pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL), res);
will do the "masked by "-1" per the bitlength of vCBM" (actually max_pcbm) action mentioned by you,
see below:
static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm, int res)
{
uint32_t max_pcbm = get_max_pcbm(vm, res);

/* Find the position low (the first bit set) in max_pcbm */
uint16_t low = ffs64((uint64_t)max_pcbm);

/*
* Mask off the unwanted bits to prevent erroneous mask values,
* pcbm set bits should only be in the range of [low, high]
*/
return (vcbm << low) & max_pcbm;
}

Or, if you are talking about the reserved bits in pCBM side, this should be a
fixed pattern, and this pattern can be got at PCPU boot time.
In this case, it can be much simpler.
Don: I will need to rethink on this, as current code for the getting/assembling reserved bits for pcbm/vcbm is already very simple.
getting at PCPU boot time may be an overkill and may need more code.

+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
This is address, not pmsr.
Don: I can change it to a better name. But in our current code, we use msr/vmsr to denote a msr/vmsr address, so I though we can do the same thing for physical
msr address and simply call it pmsr?

+ pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res);

I am not sure here. Will see the answer of previous comments.

+ write_pcbm(pmsr, pcbm);
+
After the set, we need to update the auto-save MSR list.
Don: why? seems no need to me. The pcbm/vcbm msrs are not shared, they are used exclusively by this vcat-vm only per-se.
Only the PQR is shared per pcpu, so only the PQR needs to be on the auto-save MSR list.


+ return 0;
}

/**
@@ -431,6 +463,28 @@ static int32_t write_vclosid(struct acrn_vcpu
*vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L3);
+ }
+ }
Given that the caller side already "switch case" it, we can avoid double effort
here (if..else). How about to do in caller side.
Don: No, looks we still needs to do this in vcat.c, we have the following in vmsr.c:
#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):
case MSR_IA32_PQR_ASSOC:
{
err = write_vcat_msr(vcpu, msr, v);
break;
}
#endif

But hitting one of the above case line in vmsr.c does not mean the msr is assigned to the vcat-vm:

one vm may only support the vcat for msrs ranging from MSR_IA32_L2_MASK_BASE to MSR_IA32_L2_MASK_BASE + 3
other vm may support vcat for msrs ranging from MSR_IA32_L2_MASK_BASE to MSR_IA32_L2_MASK_BASE + 6
etc, so we still need is_l2_vcbm_msr or is_l3_vcbm_msr to tell us about this


+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 8d70acf3d..eef069cb1 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1090,6 +1090,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 68c813f83..968a183ed 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ void init_vcat_msrs(struct acrn_vcpu *vcpu);
uint16_t vcat_get_num_vclosids(const struct acrn_vm *vm); uint32_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint32_t pcbm, int res);
int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1








Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 1:51 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Eddie Dong
Sent: Sunday, October 17, 2021 10:16 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 6:49 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn,
from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways
assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 68
++++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 +++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 1901ba9f3..b15413356 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,25 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t
+vcbm, int res) {
+ uint32_t max_pcbm = get_max_pcbm(vm, res);
This remind me: Is CBM 64 bits, or 32 bits?
Does SDM define?
Don: it is 32-bit, higher 32-bit are reserved
Even if it is 32 bits, we can still use 64 bits for the v-value to avoid unnecessary type casting. vcpu_set_guest_msr is doing so too.




+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64((uint64_t)max_pcbm);
+
+ /*
+ * Mask off the unwanted bits to prevent erroneous mask values,
+ * pcbm set bits should only be in the range of [low, high]
+ */
+ return (vcbm << low) & max_pcbm;
+}
+
/**
* @brief Map vMSR to pMSR
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19
+414,32 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm,
uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint32_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL)
|
pcbm;
What in my mind is that the guest has a value. It should be masked by "-1"
per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those guest
owned bits).
Yes, I only did that for pcbm (but did not do that for vcbm):
the pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL), res);
will do the "masked by "-1" per the bitlength of vCBM" (actually max_pcbm)
action mentioned by you, see below:
SDM says certain rules for the valid value of the MSR.
For CBM, SDM says:
Attempts to program a value without contiguous '1's (including zero) will result in a general protection fault (#GP(0)).

If a reserved bit is written to CBM, CPU need to raise #GP too.


static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm, int
res) {
uint32_t max_pcbm = get_max_pcbm(vm, res);

/* Find the position low (the first bit set) in max_pcbm */
uint16_t low = ffs64((uint64_t)max_pcbm);

/*
* Mask off the unwanted bits to prevent erroneous mask values,
* pcbm set bits should only be in the range of [low, high]
*/
return (vcbm << low) & max_pcbm;
}

Or, if you are talking about the reserved bits in pCBM side, this
should be a fixed pattern, and this pattern can be got at PCPU boot time.
In this case, it can be much simpler.
Don: I will need to rethink on this, as current code for the getting/assembling
reserved bits for pcbm/vcbm is already very simple.
getting at PCPU boot time may be an overkill and may need more code.
The pattern is a simple system wide global variable or VM wide field, that can be initialized at boot time or VM creation time. This way, we don't need to read the PMSR at each vCBM write time.

But given that CBM is not a frequent operation, I am fine to either solution.


+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
This is address, not pmsr.
Don: I can change it to a better name. But in our current code, we use
msr/vmsr to denote a msr/vmsr address, so I though we can do the same
thing for physical msr address and simply call it pmsr?
I see. It seems vmsr.c is using this style. Then it is fine.


+ pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res);

I am not sure here. Will see the answer of previous comments.

+ write_pcbm(pmsr, pcbm);
+
After the set, we need to update the auto-save MSR list.
Don: why? seems no need to me. The pcbm/vcbm msrs are not shared, they
are used exclusively by this vcat-vm only per-se.
Only the PQR is shared per pcpu, so only the PQR needs to be on the
auto-save MSR list.
See comments in 4/8.



+ return 0;
}

/**
@@ -431,6 +463,28 @@ static int32_t write_vclosid(struct acrn_vcpu
*vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L3);
+ }
+ }
Given that the caller side already "switch case" it, we can avoid
double effort here (if..else). How about to do in caller side.
Don: No, looks we still needs to do this in vcat.c, we have the following in
vmsr.c:
True.
We can do it in caller side to avoid one switch case, but we pay with probably more lines of code.
Up to you.


#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):
case MSR_IA32_PQR_ASSOC:
{
err = write_vcat_msr(vcpu, msr, v);
break;
}
#endif

But hitting one of the above case line in vmsr.c does not mean the msr is
assigned to the vcat-vm:

one vm may only support the vcat for msrs ranging from
MSR_IA32_L2_MASK_BASE to MSR_IA32_L2_MASK_BASE + 3
other vm may support vcat for msrs ranging from MSR_IA32_L2_MASK_BASE
to MSR_IA32_L2_MASK_BASE + 6
etc, so we still need is_l2_vcbm_msr or is_l3_vcbm_msr to tell us about this


+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 8d70acf3d..eef069cb1 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1090,6 +1090,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 68c813f83..968a183ed 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ void init_vcat_msrs(struct acrn_vcpu *vcpu);
uint16_t vcat_get_num_vclosids(const struct acrn_vm *vm); uint32_t
vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint32_t pcbm, int res);
int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1











Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 17, 2021 11:18 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 1:51 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Eddie Dong
Sent: Sunday, October 17, 2021 10:16 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the
vCAT MSRs write handler



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Dongsheng Zhang
Sent: Monday, October 18, 2021 6:49 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to
physical) mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn,
from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways
assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 68
++++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 +++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 1901ba9f3..b15413356 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,25 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t
+vcbm, int res) {
+ uint32_t max_pcbm = get_max_pcbm(vm, res);
This remind me: Is CBM 64 bits, or 32 bits?
Does SDM define?
Don: it is 32-bit, higher 32-bit are reserved
Even if it is 32 bits, we can still use 64 bits for the v-value to avoid
unnecessary type casting. vcpu_set_guest_msr is doing so too.
Don: yes, I will do this in next PATCH





+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64((uint64_t)max_pcbm);
+
+ /*
+ * Mask off the unwanted bits to prevent erroneous mask values,
+ * pcbm set bits should only be in the range of [low, high]
+ */
+ return (vcbm << low) & max_pcbm; }
+
/**
* @brief Map vMSR to pMSR
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19
+414,32 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm,
uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint32_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL)
|
pcbm;
What in my mind is that the guest has a value. It should be masked by "-
1"
per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those
guest owned bits).
Yes, I only did that for pcbm (but did not do that for vcbm):
the pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res); will do the "masked by "-1" per the bitlength of vCBM"
(actually max_pcbm) action mentioned by you, see below:
SDM says certain rules for the valid value of the MSR.
For CBM, SDM says:
Attempts to program a value without contiguous '1's (including zero) will
result in a general protection fault (#GP(0)).

If a reserved bit is written to CBM, CPU need to raise #GP too.
Don: I am assuming:
1. the guest os should have already done that checking.
2. non contiguous '1's vcbm translates to non-contiguous '1's pcbm, so the pcpu will raise a #GP, surely.
but we also need to inject a #GP to guest in this case (as vcbm has non-contiguous '1' bits),
if we want to follow the SDM.


static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t vcbm,
int
res) {
uint32_t max_pcbm = get_max_pcbm(vm, res);

/* Find the position low (the first bit set) in max_pcbm */
uint16_t low = ffs64((uint64_t)max_pcbm);

/*
* Mask off the unwanted bits to prevent erroneous mask values,
* pcbm set bits should only be in the range of [low, high]
*/
return (vcbm << low) & max_pcbm;
}

Or, if you are talking about the reserved bits in pCBM side, this
should be a fixed pattern, and this pattern can be got at PCPU boot time.
In this case, it can be much simpler.
Don: I will need to rethink on this, as current code for the
getting/assembling reserved bits for pcbm/vcbm is already very simple.
getting at PCPU boot time may be an overkill and may need more code.
The pattern is a simple system wide global variable or VM wide field, that
can be initialized at boot time or VM creation time. This way, we don't need
to read the PMSR at each vCBM write time.

But given that CBM is not a frequent operation, I am fine to either solution.
Don:
I will define a system wide global variable to save the reserved bit of pCBM.



+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu,
__unused uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
This is address, not pmsr.
Don: I can change it to a better name. But in our current code, we use
msr/vmsr to denote a msr/vmsr address, so I though we can do the same
thing for physical msr address and simply call it pmsr?
I see. It seems vmsr.c is using this style. Then it is fine.
Don:
If so, then how about the following function name? do you still want to rename it to get_backing_pmsr_address()?
uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr, int res)


+ pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res);

I am not sure here. Will see the answer of previous comments.

+ write_pcbm(pmsr, pcbm);
+
After the set, we need to update the auto-save MSR list.
Don: why? seems no need to me. The pcbm/vcbm msrs are not shared,
they
are used exclusively by this vcat-vm only per-se.
Only the PQR is shared per pcpu, so only the PQR needs to be on the
auto-save MSR list.
See comments in 4/8.
Don: will double check, but still does not understand the need to do so here, frankly.



+ return 0;
}

/**
@@ -431,6 +463,28 @@ static int32_t write_vclosid(struct acrn_vcpu
*vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L3);
+ }
+ }
Given that the caller side already "switch case" it, we can avoid
double effort here (if..else). How about to do in caller side.
Don: No, looks we still needs to do this in vcat.c, we have the
following in
vmsr.c:
True.
We can do it in caller side to avoid one switch case, but we pay with
probably more lines of code.
Up to you.
Don: yes, more code if do this on caller side, so I will stick with my current solution



#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):
case MSR_IA32_PQR_ASSOC:
{
err = write_vcat_msr(vcpu, msr, v);
break;
}
#endif

But hitting one of the above case line in vmsr.c does not mean the msr
is assigned to the vcat-vm:

one vm may only support the vcat for msrs ranging from
MSR_IA32_L2_MASK_BASE to MSR_IA32_L2_MASK_BASE + 3 other vm
may
support vcat for msrs ranging from MSR_IA32_L2_MASK_BASE
to MSR_IA32_L2_MASK_BASE + 6
etc, so we still need is_l2_vcbm_msr or is_l3_vcbm_msr to tell us
about this


+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 8d70acf3d..eef069cb1 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1090,6 +1090,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 68c813f83..968a183ed 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ void init_vcat_msrs(struct acrn_vcpu *vcpu);
uint16_t vcat_get_num_vclosids(const struct acrn_vm *vm);
uint32_t vcat_pcbm_to_vcbm(const struct acrn_vm *vm, uint32_t
pcbm, int res); int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1














Eddie Dong
 

per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those
guest owned bits).
Yes, I only did that for pcbm (but did not do that for vcbm):
the pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res); will do the "masked by "-1" per the bitlength of vCBM"
(actually max_pcbm) action mentioned by you, see below:
SDM says certain rules for the valid value of the MSR.
For CBM, SDM says:
Attempts to program a value without contiguous '1's (including zero)
will result in a general protection fault (#GP(0)).

If a reserved bit is written to CBM, CPU need to raise #GP too.
Don: I am assuming:
1. the guest os should have already done that checking.
2. non contiguous '1's vcbm translates to non-contiguous '1's pcbm, so the
pcpu will raise a #GP, surely.
but we also need to inject a #GP to guest in this case (as vcbm has
non-contiguous '1' bits), if we want to follow the SDM.
You are likely assuming the Linux behavior. This is good to get it run, but VMM needs to handle all the corner cases for sure.

Relying on p-Side #GP handler to handle gracefully and inject #GP to guest is quite wrong. I am even not sure ACRN can resume from physical #GP. Very likely a P-side #GP will crash the system.



static uint32_t vcbm_to_pcbm(const struct acrn_vm *vm, uint32_t
vcbm, int
res) {
uint32_t max_pcbm = get_max_pcbm(vm, res);

/* Find the position low (the first bit set) in max_pcbm */
uint16_t low = ffs64((uint64_t)max_pcbm);

/*
* Mask off the unwanted bits to prevent erroneous mask values,
* pcbm set bits should only be in the range of [low, high]
*/
return (vcbm << low) & max_pcbm;
}

Or, if you are talking about the reserved bits in pCBM side, this
should be a fixed pattern, and this pattern can be got at PCPU boot time.
In this case, it can be much simpler.
Don: I will need to rethink on this, as current code for the
getting/assembling reserved bits for pcbm/vcbm is already very simple.
getting at PCPU boot time may be an overkill and may need more code.
The pattern is a simple system wide global variable or VM wide field,
that can be initialized at boot time or VM creation time. This way, we
don't need to read the PMSR at each vCBM write time.

But given that CBM is not a frequent operation, I am fine to either solution.
Don:
I will define a system wide global variable to save the reserved bit of pCBM.



+
+ msr_write(pmsr, pmsr_value);
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu,
__unused uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t
+vmsr, uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
- */
- return -EFAULT;
+ uint32_t pmsr, pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, val);
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
This is address, not pmsr.
Don: I can change it to a better name. But in our current code, we
use msr/vmsr to denote a msr/vmsr address, so I though we can do the
same thing for physical msr address and simply call it pmsr?
I see. It seems vmsr.c is using this style. Then it is fine.
Don:
If so, then how about the following function name? do you still want to
rename it to get_backing_pmsr_address()?
This one is fine without address, but please add comments.

uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr, int res)
For this, we should not do, because all the other x_to_y is doing state "to". See vcbm_to_pcbm.


Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Sunday, October 17, 2021 11:59 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

per the bitlength of vCBM.
The rest bits can be from host (should be cleared to 0 for those
guest owned bits).
Yes, I only did that for pcbm (but did not do that for vcbm):
the pcbm = vcbm_to_pcbm(vcpu->vm, (uint32_t)(val & 0xFFFFFFFFUL),
res); will do the "masked by "-1" per the bitlength of vCBM"
(actually max_pcbm) action mentioned by you, see below:
SDM says certain rules for the valid value of the MSR.
For CBM, SDM says:
Attempts to program a value without contiguous '1's (including zero)
will result in a general protection fault (#GP(0)).

If a reserved bit is written to CBM, CPU need to raise #GP too.
Don: I am assuming:
1. the guest os should have already done that checking.
2. non contiguous '1's vcbm translates to non-contiguous '1's pcbm, so
the pcpu will raise a #GP, surely.
but we also need to inject a #GP to guest in this case (as vcbm has
non-contiguous '1' bits), if we want to follow the SDM.
You are likely assuming the Linux behavior. This is good to get it run, but
VMM needs to handle all the corner cases for sure.

Relying on p-Side #GP handler to handle gracefully and inject #GP to guest is
quite wrong. I am even not sure ACRN can resume from physical #GP. Very
likely a P-side #GP will crash the system.
Don: yes, I know that, will check vcbm first, and inject a #GP to guest then return from function, so no physical #GP will be raised.


Dongsheng Zhang
 

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

Implement the write_vcat_msr() function to handle the MSR_IA32_PQR_ASSOC
and MSR_IA32_type_MASK_n vCAT MSRs write request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 100 +++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 ++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c b/hypervisor/arch/x86/guest/vcat.c
index 03e9a3cc9..1dda6b61f 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,21 @@ int32_t read_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint64_t vcbm_to_pcbm(const struct acrn_vm *vm, uint64_t vcbm, int res)
+{
+ uint64_t max_pcbm = get_max_pcbm(vm, res);
+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64(max_pcbm);
+
+ return vcbm << low;
+}
+
/**
* @brief Map vMSR address (abbreviated as vmsr) to corresponding pMSR address (abbreviated as pmsr)
* Each vMSR or pMSR is identified by a 32-bit integer
@@ -395,19 +410,70 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint64_t pcbm)
+{
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) | pcbm;
+
+ msr_write(pmsr, pmsr_value);
+}
+
+/* Check if bitmask is contiguous:
+ * All (and only) contiguous '1' combinations are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.)
+ */
+static bool is_contiguous_bit_set(uint64_t bitmask)
+{
+ bool ret = false;
+ uint64_t tmp64 = bitmask;
+
+ if (tmp64 != 0UL) {
+ while ((tmp64 & 1UL) == 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ while ((tmp64 & 1UL) != 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ if (tmp64 == 0UL) {
+ ret = true;
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit, will perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len) (vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
+
+ if (is_contiguous_bit_set(masked_vcbm)) {
+ uint32_t pmsr;
+ uint64_t pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, masked_vcbm | (val & 0xFFFFFFFF00000000UL));
+
+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
+ pcbm = vcbm_to_pcbm(vcpu->vm, masked_vcbm, res);
+ write_pcbm(pmsr, pcbm);
+
+ ret = 0;
+ }
+
+ /* Return non-zero to vmexit_handler if vcbm is not contiguous, which will result in #GP injected to guest */
+ return ret;
}

/**
@@ -444,6 +510,28 @@ static int32_t write_vclosid(struct acrn_vcpu *vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL
+ */
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val)
+{
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L3);
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c b/hypervisor/arch/x86/guest/vmsr.c
index 06825d211..412388106 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1092,6 +1092,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 16ae0802b..8effa7604 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ 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_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Wednesday, October 20, 2021 6:42 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT MSRs
write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical) mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways assigned to
the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 100
+++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 ++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 03e9a3cc9..1dda6b61f 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,21 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint64_t vcbm_to_pcbm(const struct acrn_vm *vm, uint64_t vcbm,
+int res) {
+ uint64_t max_pcbm = get_max_pcbm(vm, res);
+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64(max_pcbm);
+
+ return vcbm << low;
+}
+
/**
* @brief Map vMSR address (abbreviated as vmsr) to corresponding pMSR
address (abbreviated as pmsr)
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19 +410,70
@@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm, uint32_t vmsr,
int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint64_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) |
pcbm;
+
+ msr_write(pmsr, pmsr_value);
+}
+
+/* Check if bitmask is contiguous:
+ * All (and only) contiguous '1' combinations are allowed (e.g. FFFFH,
+0FF0H, 003CH, etc.) */ static bool is_contiguous_bit_set(uint64_t
+bitmask) {
+ bool ret = false;
+ uint64_t tmp64 = bitmask;
+
+ if (tmp64 != 0UL) {
+ while ((tmp64 & 1UL) == 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ while ((tmp64 & 1UL) != 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ if (tmp64 == 0UL) {
+ ret = true;
+ }
+ }
That is too ugly.

We can use bit-scan instruction to find the highest set-bit (say high), and the lowest set bit (say low)
Then the contiguous value must be: (1<<(high+1)) -- (1<<low)


+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit, will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should inject #GP.
Not?

+
+ if (is_contiguous_bit_set(masked_vcbm)) {
+ uint32_t pmsr;
+ uint64_t pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, masked_vcbm | (val &
+0xFFFFFFFF00000000UL));
+
Didn't understand why we not directly set guest value here.

+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
+ pcbm = vcbm_to_pcbm(vcpu->vm, masked_vcbm, res);
+ write_pcbm(pmsr, pcbm);
+
+ ret = 0;
+ }
+
+ /* Return non-zero to vmexit_handler if vcbm is not contiguous, which
will result in #GP injected to guest */
+ return ret;
}

/**
@@ -444,6 +510,28 @@ static int32_t write_vclosid(struct acrn_vcpu *vcpu,
uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
Please move this to caller side to avoid double switch/if-else...
This one happens frequently at runtime.

+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val, RDT_RESOURCE_L3);
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 06825d211..412388106 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1092,6 +1092,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 16ae0802b..8effa7604 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ 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_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
*rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t
+val);

#endif /* VCAT_H_ */

--
2.25.1





Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Wednesday, October 20, 2021 5:53 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Wednesday, October 20, 2021 6:42 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn, from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways
assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 100
+++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 ++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 03e9a3cc9..1dda6b61f 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,21 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint64_t vcbm_to_pcbm(const struct acrn_vm *vm, uint64_t vcbm,
+int res) {
+ uint64_t max_pcbm = get_max_pcbm(vm, res);
+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64(max_pcbm);
+
+ return vcbm << low;
+}
+
/**
* @brief Map vMSR address (abbreviated as vmsr) to corresponding
pMSR address (abbreviated as pmsr)
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19
+410,70 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm,
uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint64_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL) |
pcbm;
+
+ msr_write(pmsr, pmsr_value);
+}
+
+/* Check if bitmask is contiguous:
+ * All (and only) contiguous '1' combinations are allowed (e.g.
+FFFFH, 0FF0H, 003CH, etc.) */ static bool
+is_contiguous_bit_set(uint64_t
+bitmask) {
+ bool ret = false;
+ uint64_t tmp64 = bitmask;
+
+ if (tmp64 != 0UL) {
+ while ((tmp64 & 1UL) == 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ while ((tmp64 & 1UL) != 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ if (tmp64 == 0UL) {
+ ret = true;
+ }
+ }
That is too ugly.

We can use bit-scan instruction to find the highest set-bit (say high), and the
lowest set bit (say low) Then the contiguous value must be: (1<<(high+1)) --
(1<<low)
Don: no, I do not think your algo is right, we need to check if the 1s are contiguous.
Can you rethink on this?


+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM
value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should inject
#GP.
Not?
Don: yes, just return non-zero value (= -EINVAL) here if not contiguous to upper caller,
then eventually in hv_main.c, it will inject a GP to guest after vmext_handler(), see below:
ret = vmexit_handler(vcpu);
if (ret < 0) {
pr_fatal("dispatch VM exit handler failed for reason"
" %d, ret = %d!", vcpu->arch.exit_reason, ret);
vcpu_inject_gp(vcpu, 0U);
continue;
}


+
+ if (is_contiguous_bit_set(masked_vcbm)) {
+ uint32_t pmsr;
+ uint64_t pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, masked_vcbm | (val &
+0xFFFFFFFF00000000UL));
+
Didn't understand why we not directly set guest value here.
Don: we can, but if the vcbm is not within range: vcbm set bits should only be in the range of [0, vcbm_len) (vcat_get_max_vcbm),
Then what should we do here, return an error code to upper caller (will #GP), or just use masked off vcbm (masked_vcbm) and set it to guest? I choose the latter one here


+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
+ pcbm = vcbm_to_pcbm(vcpu->vm, masked_vcbm, res);
+ write_pcbm(pmsr, pcbm);
+
+ ret = 0;
+ }
+
+ /* Return non-zero to vmexit_handler if vcbm is not contiguous,
+which
will result in #GP injected to guest */
+ return ret;
}

/**
@@ -444,6 +510,28 @@ static int32_t write_vclosid(struct acrn_vcpu
*vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
Please move this to caller side to avoid double switch/if-else...
This one happens frequently at runtime.
Don: yes we can, but move this to caller code (vmsr.c) will make the logic hard and requires more code.
I would try to avoid that if I have the choice.


+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L3);
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 06825d211..412388106 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1092,6 +1092,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 16ae0802b..8effa7604 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ 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_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1








Dongsheng Zhang
 

+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM
value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should
inject #GP.
Yes, we should inject #GP when the SW sets bits beyond hardware supported value, or when the vcbm is not contiguous,
I already added the code for these 2 cases.


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Thursday, October 21, 2021 1:13 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Eddie Dong
Sent: Wednesday, October 20, 2021 5:53 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@...
<acrn-dev@...> On Behalf Of Dongsheng Zhang
Sent: Wednesday, October 20, 2021 6:42 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

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

Implement the write_vcat_msr() function to handle the
MSR_IA32_PQR_ASSOC and MSR_IA32_type_MASK_n vCAT MSRs write
request.

Several vCAT P2V (physical to virtual) and V2P (virtual to physical)
mappings
exist:

struct acrn_vm_config *vm_config = get_vm_config(vm_id)

max_pcbm = vm_config->max_type_pcbm (type: l2 or l3)
mask_shift = ffs64(max_pcbm)

vclosid = vmsr - MSR_IA32_type_MASK_0
pclosid = vm_config->pclosids[vclosid]

pmsr = MSR_IA32_type_MASK_0 + pclosid
pcbm = vcbm << mask_shift
vcbm = pcbm >> mask_shift

Where
MSR_IA32_type_MASK_n: L2 or L3 mask msr address for CLOSIDn,
from
0C90H through 0D8FH (inclusive).

max_pcbm: a bitmask that selects all the physical cache ways
assigned to the VM

vclosid: virtual CLOSID, always starts from 0

pclosid: corresponding physical CLOSID for a given vclosid

vmsr: virtual msr address, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pmsr: physical msr address

vcbm: virtual CBM, passed to vCAT handlers by the
caller functions rdmsr_vmexit_handler()/wrmsr_vmexit_handler()

pcbm: physical CBM

Tracked-On: #5917
Signed-off-by: dongshen <dongsheng.x.zhang@...>
---
hypervisor/arch/x86/guest/vcat.c | 100
+++++++++++++++++--
hypervisor/arch/x86/guest/vmsr.c | 9 ++
hypervisor/include/arch/x86/asm/guest/vcat.h | 1 +
3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vcat.c
b/hypervisor/arch/x86/guest/vcat.c
index 03e9a3cc9..1dda6b61f 100644
--- a/hypervisor/arch/x86/guest/vcat.c
+++ b/hypervisor/arch/x86/guest/vcat.c
@@ -365,6 +365,21 @@ int32_t read_vcat_msr(const struct acrn_vcpu
*vcpu, uint32_t vmsr, uint64_t *rva
return ret;
}

+/**
+ * @brief Map vCBM to pCBM
+ *
+ * @pre vm != NULL
+ */
+static uint64_t vcbm_to_pcbm(const struct acrn_vm *vm, uint64_t
+vcbm, int res) {
+ uint64_t max_pcbm = get_max_pcbm(vm, res);
+
+ /* Find the position low (the first bit set) in max_pcbm */
+ uint16_t low = ffs64(max_pcbm);
+
+ return vcbm << low;
+}
+
/**
* @brief Map vMSR address (abbreviated as vmsr) to corresponding
pMSR address (abbreviated as pmsr)
* Each vMSR or pMSR is identified by a 32-bit integer @@ -395,19
+410,70 @@ static uint32_t vmsr_to_pmsr(const struct acrn_vm *vm,
uint32_t vmsr, int res)
return pmsr;
}

+static void write_pcbm(uint32_t pmsr, uint64_t pcbm) {
+ /* Preserve reserved bits, and only set the pCBM bits */
+ uint64_t pmsr_value = (msr_read(pmsr) & 0xFFFFFFFF00000000UL)
|
pcbm;
+
+ msr_write(pmsr, pmsr_value);
+}
+
+/* Check if bitmask is contiguous:
+ * All (and only) contiguous '1' combinations are allowed (e.g.
+FFFFH, 0FF0H, 003CH, etc.) */ static bool
+is_contiguous_bit_set(uint64_t
+bitmask) {
+ bool ret = false;
+ uint64_t tmp64 = bitmask;
+
+ if (tmp64 != 0UL) {
+ while ((tmp64 & 1UL) == 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ while ((tmp64 & 1UL) != 0UL) {
+ tmp64 >>= 1U;
+ }
+
+ if (tmp64 == 0UL) {
+ ret = true;
+ }
+ }
That is too ugly.

We can use bit-scan instruction to find the highest set-bit (say
high), and the lowest set bit (say low) Then the contiguous value must
be: (1<<(high+1)) --
(1<<low)
Don: no, I do not think your algo is right, we need to check if the 1s are
contiguous.
Did you try it?

Return ( tmp64 == ((1<<(high+1)) - (1 << low));


Can you rethink on this?


+
+ return ret;
+}
+
/**
* @brief vCBM MSR write handler
*
* @pre vcpu != NULL && vcpu->vm != NULL
*/
-static int32_t write_vcbm(__unused struct acrn_vcpu *vcpu, __unused
uint32_t vmsr, __unused uint64_t val, __unused int res)
+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM
value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should
inject #GP.
Not?
Don: yes, just return non-zero value (= -EINVAL) here if not contiguous to
upper caller, then eventually in hv_main.c, it will inject a GP to guest after
vmext_handler(), see below:
ret = vmexit_handler(vcpu);
if (ret < 0) {
pr_fatal("dispatch VM exit handler failed for reason"
" %d, ret = %d!", vcpu->arch.exit_reason, ret);
vcpu_inject_gp(vcpu, 0U);
continue;
}
I am saying you missed the check of case when the set-value is beyond the allowed scope, but you are talking is_contiguous_bit_set.

That is 2 different thing and we need to make sure both conditions are met.


+
+ if (is_contiguous_bit_set(masked_vcbm)) {
+ uint32_t pmsr;
+ uint64_t pcbm;
+
+ /* Write vCBM first */
+ vcpu_set_guest_msr(vcpu, vmsr, masked_vcbm | (val &
+0xFFFFFFFF00000000UL));
+
Didn't understand why we not directly set guest value here.
Don: we can, but if the vcbm is not within range: vcbm set bits should only be
in the range of [0, vcbm_len) (vcat_get_max_vcbm), Then what should we do
That is the scope check in above comments. We must do both.

here, return an error code to upper caller (will #GP), or just use masked off
vcbm (masked_vcbm) and set it to guest? I choose the latter one here
Here we are setting the V value. Please exactly follow what SDM says.
vcpu_set_guest_msr is setting V value, whatever the guest write, set it or inject #GP.



+ /* Write pCBM: */
+ pmsr = vmsr_to_pmsr(vcpu->vm, vmsr, res);
+ pcbm = vcbm_to_pcbm(vcpu->vm, masked_vcbm, res);
+ write_pcbm(pmsr, pcbm);
+
+ ret = 0;
+ }
+
+ /* Return non-zero to vmexit_handler if vcbm is not contiguous,
+which
will result in #GP injected to guest */
+ return ret;
}

/**
@@ -444,6 +510,28 @@ static int32_t write_vclosid(struct acrn_vcpu
*vcpu, uint64_t val)
return 0;
}

+/**
+ * @brief vCAT MSRs write handler
+ *
+ * @pre vcpu != NULL && vcpu->vm != NULL */ int32_t
+write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr, uint64_t val) {
+ int32_t ret = -EACCES;
+
+ if (is_vcat_configured(vcpu->vm)) {
+ if (vmsr == MSR_IA32_PQR_ASSOC) {
+ ret = write_vclosid(vcpu, val);
Please move this to caller side to avoid double switch/if-else...
This one happens frequently at runtime.
Don: yes we can, but move this to caller code (vmsr.c) will make the logic hard
and requires more code.
I didn't see any hard part. Only thing we need to do is to expose write_vclosid publically.

I would try to avoid that if I have the choice.


+ } else if (is_l2_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L2);
+ } else if (is_l3_vcbm_msr(vcpu->vm, vmsr)) {
+ ret = write_vcbm(vcpu, vmsr, val,
RDT_RESOURCE_L3);
+ }
+ }
+
+ return ret;
+}
+
/**
* @brief Initialize vCBM MSRs
*
diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 06825d211..412388106 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -1092,6 +1092,15 @@ int32_t wrmsr_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):
+ case MSR_IA32_PQR_ASSOC:
+ {
+ err = write_vcat_msr(vcpu, msr, 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 16ae0802b..8effa7604 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcat.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcat.h
@@ -16,6 +16,7 @@ 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_vcat_msr(const struct acrn_vcpu *vcpu, uint32_t vmsr,
uint64_t *rval);
+int32_t write_vcat_msr(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val);

#endif /* VCAT_H_ */

--
2.25.1











Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Dongsheng Zhang
Sent: Thursday, October 21, 2021 8:50 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM
value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should
inject #GP.
Yes, we should inject #GP when the SW sets bits beyond hardware supported
value, or when the vcbm is not contiguous, I already added the code for these
2 cases.
I don't think so.






Dongsheng Zhang
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Eddie Dong
Sent: Wednesday, October 20, 2021 9:25 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler



-----Original Message-----
From: acrn-dev@... <acrn-dev@...>
On Behalf Of Dongsheng Zhang
Sent: Thursday, October 21, 2021 8:50 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH V6 7/8] hv: vCAT: implementing the vCAT
MSRs write handler

+static int32_t write_vcbm(struct acrn_vcpu *vcpu, uint32_t vmsr,
+uint64_t val, int res)
{
- /* TODO: this is going to be implemented in a subsequent commit,
will
perform the following actions:
- * write vCBM
- * vmsr_to_pmsr and vcbm_to_pcbm
- * write pCBM
+ int32_t ret = -EINVAL;
+ /*
+ * vcbm set bits should only be in the range of [0, vcbm_len)
(vcat_get_max_vcbm),
+ * so mask with vcat_get_max_vcbm to prevent erroneous vCBM
value
*/
- return -EFAULT;
+ uint64_t masked_vcbm = val & vcat_get_max_vcbm(vcpu->vm, res);
If SW sets bits beyond hardware supported value, I thought we should
inject #GP.
Yes, we should inject #GP when the SW sets bits beyond hardware
supported value, or when the vcbm is not contiguous, I already added
the code for these
2 cases.
I don't think so.
Don: I mean I added the code to check set bits beyond h/w supported value locally on my machine today.
But not posted yet.