Date   

Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/11/2019 11:08 PM, Kaige Fu wrote:
On 04-11 Thu 14:49, Yin, Fengwei wrote:
On 4/11/2019 2:36 PM, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}
+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
If so, let's move vmx_off to just before hlt_cpu in cpu_dead.
And add comments about this case?
I am afraid we cann't do like this. Considering the following condition, the INIT/SIPI signal will arrive before VMX off.
+-----------+ +----------+
| SOS CPU | | VM CPU |
+-----------+ +----------+
|make_pcpu_offline|
+---------------> +--+
| | |state
| | |dead
|1.Send INIT/SIPI +<-+
+---------------->+
| +--+
| | | vmx
| | | off
|2.Send INIT/SIPI +<-+
+---------------->+
| |
| |
The SOS CPU may send INIT/SIPI in step 1 or step 2.
If SOS CPU send INIT/SIPI in step 1, the similar problem like v2 patch occurs. Right?
No. It's different. In this scenario, we actually know the target cpu is
off (state is changed to offline). But for v2 patch, we don't know where
the cpu is.

Regards
Yin, Fengwei

Anyway, if we can move vmx_off after set cpu state, we make it to check the pcpu_active_bitmap without modifing cpu_dead
function. This is why I check cpu state in my patch.

Regards
Yin, Fengwei

+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0






Re: [RFC PATCH 10/11] hv: vlapic: minor fix about vlapic write

Grandhi, Sainath
 

Yes, except for few registers, APIC write accesses will cause vmexit.

-----Original Message-----
From: Li, Fei1
Sent: Thursday, April 11, 2019 8:06 AM
To: Xu, Anthony <anthony.xu@...>
Cc: acrn-dev@...; Dong, Eddie <eddie.dong@...>;
Chen, Jason CJ <jason.cj.chen@...>; Grandhi, Sainath
<sainath.grandhi@...>
Subject: Re: [RFC PATCH 10/11] hv: vlapic: minor fix about vlapic write

On Thu, Apr 11, 2019 at 01:19:57PM +0800, Xu, Anthony wrote:
Acked-by: Anthony Xu <anthony.xu@...>

Please submit a separate PR.

apic_write_vmexit_handler is not correct, only limited apic access will cause
apic write vmexit.
Yes

For example , I don't think write to APIC_OFFSET_ID would cause apic write
vmexit.
In our case, EOI will not trigget an APIC-write exit which will trigger veoi vmexit.
Please help clean up the function.
Section 29.4.3.1, Vol 3, SDM

If “APIC-register virtualization is 1, a write access is virtualized if it is entirely
within one the following ranges of offsets:
— 020H–023H (local APIC ID);
— 080H–083H (task priority);
— 0B0H–0B3H (end of interrupt);
— 0D0H–0D3H (logical destination);
— 0E0H–0E3H (destination format);
— 0F0H–0F3H (spurious-interrupt vector); — 280H–283H (error status); —
300H–303H or 310H–313H (interrupt command); — 320H–323H, 330H–333H,
340H–343H, 350H–353H, 360H–363H, or 370H–373H (LVT entries); — 380H–
383H (initial count); or — 3E0H–3E3H (divide configuration).
Following this, the processor performs certain actions after completion of the
operation of which the access was a part. APIC-write emulation is described in
Section 29.4.3.2

Section 29.4.3.2, Vol 3, SDM
080H (task priority) - do TPR virtualization 0B0H (end of interrupt) - EOI
virtualization (apicv advanced used, so VID is enabled) 300H (interrupt
command — low) - may causes an APIC-write VM exit 310H–313H (interrupt
command — high). - No other virtualization or VM exit occurs.
Any other page offset. The processor causes an APIC-write VM exit.

So
— 020H–023H (local APIC ID);
— 080H–083H (task priority);
— 0D0H–0D3H (logical destination);
— 0E0H–0E3H (destination format);
— 0F0H–0F3H (spurious-interrupt vector); — 280H–283H (error status); —
300H–303H (interrupt command - low); — 320H–323H, 330H–333H, 340H–
343H, 350H–353H, 360H–363H, or 370H–373H (LVT entries); — 380H–383H
(initial count); or — 3E0H–3E3H (divide configuration).
will trigger an APIC-write VM exit.



Anthony


-----Original Message-----
From: Li, Fei1
Sent: Thursday, March 21, 2019 6:04 PM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>; Xu, Anthony
<anthony.xu@...>; Chen, Jason CJ <jason.cj.chen@...>;
Grandhi, Sainath <sainath.grandhi@...>
Subject: [RFC PATCH 10/11] hv: vlapic: minor fix about vlapic write

1) In x2apic, when read ICR, we want to read a 64-bits value.
2) In xapic mode, we would not write self-IPI since it's only aviable for x2apic
mode.

Signed-off-by: Li, Fei1 <fei1.li@...>
---
hypervisor/arch/x86/guest/vlapic.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c
b/hypervisor/arch/x86/guest/vlapic.c
index 18c7a3de0a07..b4bb68587bdb 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -94,6 +94,7 @@ apicv_set_intr_ready(struct acrn_vlapic *vlapic,
uint32_t vector);

static void apicv_post_intr(uint16_t dest_pcpu_id);

+static void vlapic_x2apic_self_ipi_handler(struct acrn_vlapic
+*vlapic);
/*
* Post an interrupt to the vcpu running on 'hostcpu'. This will use a
* hardware assist if available (e.g. Posted Interrupt) or fall
back to @@ -1526,7 +1527,11 @@ vlapic_read(struct acrn_vlapic *vlapic,
uint32_t offset_arg, uint64_t *data)
*data = lapic->esr.v;
break;
case APIC_OFFSET_ICR_LOW:
- *data = lapic->icr_lo.v;
+ if (is_x2apic_enabled(vlapic)) {
+ *data = (((uint64_t)lapic->icr_hi.v) << 32U) |
lapic->icr_lo.v;
+ } else {
+ *data = lapic->icr_lo.v;
+ }
break;
case APIC_OFFSET_ICR_HI:
*data = lapic->icr_hi.v;
@@ -1564,7 +1569,7 @@ vlapic_read(struct acrn_vlapic *vlapic, uint32_t
offset_arg, uint64_t *data)
}
}

- dev_dbg(ACRN_DBG_LAPIC, "vlapic read offset %#x, data %#lx", offset,
*data);
+ dev_dbg(ACRN_DBG_LAPIC, "vlapic read offset %#x, data %#llx",
+offset, *data);
return ret;
}

@@ -1642,6 +1647,13 @@ vlapic_write(struct acrn_vlapic *vlapic, uint32_t
offset, uint64_t data)
vlapic_esr_write_handler(vlapic);
break;

+ case APIC_OFFSET_SELF_IPI:
+ if (is_x2apic_enabled(vlapic)) {
+ lapic->self_ipi.v = data32;
+ vlapic_x2apic_self_ipi_handler(vlapic);
+ }
+ break;
+
default:
/* Read only */
ret = -EACCES;
@@ -2434,11 +2446,6 @@ int32_t apic_write_vmexit_handler(struct
acrn_vcpu *vcpu)
case APIC_OFFSET_TIMER_DCR:
vlapic_dcr_write_handler(vlapic);
break;
- case APIC_OFFSET_SELF_IPI:
- if (is_x2apic_enabled(vlapic)) {
- vlapic_x2apic_self_ipi_handler(vlapic);
- }
- break;
default:
err = -EACCES;
pr_err("Unhandled APIC-Write, offset:0x%x", offset);
--
2.17.1


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Chen, Jason CJ
 

On Thu, Apr 11, 2019 at 07:18:12AM +0000, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 12:07 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 06:59:36AM +0000, Kaige Fu wrote:

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 2:54 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of
lapic_pt vm when shutdown

On Thu, Apr 11, 2019 at 10:48:24PM +0800, Kaige Fu wrote:
On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security
and correctness when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c
b/hypervisor/arch/x86/cpu.c index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id) {
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD)
&&
(timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked,
will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there
is no chance for this pcpu to set boot_state...

Yes. Once the VMX is turn off, the INIT signal blocked in VMX
root-mode will
be triggered.
But, we didn't send INIT to this cpu before.
We pause_vcpu through sending INIT, not?
No. You may miss something.
When pause_vcpu, the target cpu stays at VMX non-root mode. So, this
signal just trigger vm-exit and be discarded. It is not blocked in this case.
ok, make sense.


BTW, fengwei's suggestion looks good to me :)
Jason/Fengwei
Is there a plan to move to halt/mwait for cpu_idle?
If that is the case, this technique will not work either.
Because this technique relies on the fact that the remote cpu keeps coming out of pause periodically.
If Remote cpu is in halt, it can be woken only by an external interrupt. But since the remote cpu
has LAPIC disabled, external interrupt cannot be delivered.
If it is mwait, then we might need to use monitor on some address.
agree, if move to mwait, we still have solution for it. but not for halt.. we need think about it in the future when
enable c-state in hypervisor, at that time, we should avoid to use halt..



+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09b95e56..82a11f1c
100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for
security
+and
correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm); diff --git
a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void); void
load_cpu_state_data(void); void init_cpu_pre(uint16_t
pcpu_id_args); void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



--

Thanks

Jason



--

Thanks

Jason



--

Thanks

Jason


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Grandhi, Sainath
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 12:07 AM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 06:59:36AM +0000, Kaige Fu wrote:

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 2:54 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of
lapic_pt vm when shutdown

On Thu, Apr 11, 2019 at 10:48:24PM +0800, Kaige Fu wrote:
On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security
and correctness when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c
b/hypervisor/arch/x86/cpu.c index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id) {
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD)
&&
(timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked,
will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there
is no chance for this pcpu to set boot_state...

Yes. Once the VMX is turn off, the INIT signal blocked in VMX
root-mode will
be triggered.
But, we didn't send INIT to this cpu before.
We pause_vcpu through sending INIT, not?
No. You may miss something.
When pause_vcpu, the target cpu stays at VMX non-root mode. So, this
signal just trigger vm-exit and be discarded. It is not blocked in this case.
ok, make sense.


BTW, fengwei's suggestion looks good to me :)
Jason/Fengwei
Is there a plan to move to halt/mwait for cpu_idle?
If that is the case, this technique will not work either.
Because this technique relies on the fact that the remote cpu keeps coming out of pause periodically.
If Remote cpu is in halt, it can be woken only by an external interrupt. But since the remote cpu
has LAPIC disabled, external interrupt cannot be delivered.
If it is mwait, then we might need to use monitor on some address.


+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09b95e56..82a11f1c
100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for
security
+and
correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm); diff --git
a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void); void
load_cpu_state_data(void); void init_cpu_pre(uint16_t
pcpu_id_args); void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



--

Thanks

Jason



--

Thanks

Jason


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-11 Thu 23:08, Kaige Fu wrote:
On 04-11 Thu 14:49, Yin, Fengwei wrote:
On 4/11/2019 2:36 PM, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}
+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
If so, let's move vmx_off to just before hlt_cpu in cpu_dead.
And add comments about this case?
I am afraid we cann't do like this. Considering the following condition, the INIT/SIPI signal will arrive before VMX off.

+-----------+ +----------+
| SOS CPU | | VM CPU |
+-----------+ +----------+
|make_pcpu_offline|
+---------------> +--+
| | |state
| | |dead
|1.Send INIT/SIPI +<-+
+---------------->+
| +--+
| | | vmx
| | | off
|2.Send INIT/SIPI +<-+
+---------------->+
| |
| |

The SOS CPU may send INIT/SIPI in step 1 or step 2.
If SOS CPU send INIT/SIPI in step 1, the similar problem like v2 patch occurs. Right?

Anyway, if we can move vmx_off after set cpu state, we make it to check the pcpu_active_bitmap without modifing cpu_dead
function. This is why I check cpu state in my patch.
Sorry if there is confusing. "we make it to check" should be "we can make it also to check".

Regards
Yin, Fengwei

+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0







Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-11 Thu 14:49, Yin, Fengwei wrote:
On 4/11/2019 2:36 PM, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}
+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
If so, let's move vmx_off to just before hlt_cpu in cpu_dead.
And add comments about this case?
I am afraid we cann't do like this. Considering the following condition, the INIT/SIPI signal will arrive before VMX off.

+-----------+ +----------+
| SOS CPU | | VM CPU |
+-----------+ +----------+
|make_pcpu_offline|
+---------------> +--+
| | |state
| | |dead
|1.Send INIT/SIPI +<-+
+---------------->+
| +--+
| | | vmx
| | | off
|2.Send INIT/SIPI +<-+
+---------------->+
| |
| |

The SOS CPU may send INIT/SIPI in step 1 or step 2.
If SOS CPU send INIT/SIPI in step 1, the similar problem like v2 patch occurs. Right?

Anyway, if we can move vmx_off after set cpu state, we make it to check the pcpu_active_bitmap without modifing cpu_dead
function. This is why I check cpu state in my patch.

Regards
Yin, Fengwei

+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0






Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Chen, Jason CJ
 

On Thu, Apr 11, 2019 at 06:59:36AM +0000, Kaige Fu wrote:

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 2:54 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 10:48:24PM +0800, Kaige Fu wrote:
On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and
correctness when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) &&
(timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will
this INIT signal trigger at the moment this cpu doing vmx_off? If yes, there
is no chance for this pcpu to set boot_state...

Yes. Once the VMX is turn off, the INIT signal blocked in VMX root-mode will
be triggered.
But, we didn't send INIT to this cpu before.
We pause_vcpu through sending INIT, not?
No. You may miss something.
When pause_vcpu, the target cpu stays at VMX non-root mode. So, this
signal just trigger vm-exit and be discarded. It is not blocked in this case.
ok, make sense.


BTW, fengwei's suggestion looks good to me :)


+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and
correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void); void
load_cpu_state_data(void); void init_cpu_pre(uint16_t
pcpu_id_args); void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



--

Thanks

Jason



--

Thanks

Jason


Re: [RFC PATCH 10/11] hv: vlapic: minor fix about vlapic write

Li, Fei1
 

On Thu, Apr 11, 2019 at 01:19:57PM +0800, Xu, Anthony wrote:
Acked-by: Anthony Xu <anthony.xu@...>

Please submit a separate PR.

apic_write_vmexit_handler is not correct, only limited apic access will cause apic write vmexit.
Yes

For example , I don't think write to APIC_OFFSET_ID would cause apic write vmexit.
In our case, EOI will not trigget an APIC-write exit which will trigger veoi vmexit.
Please help clean up the function.
Section 29.4.3.1, Vol 3, SDM

If “APIC-register virtualization is 1, a write access is virtualized if it is entirely within one the following ranges of offsets:
— 020H–023H (local APIC ID);
— 080H–083H (task priority);
— 0B0H–0B3H (end of interrupt);
— 0D0H–0D3H (logical destination);
— 0E0H–0E3H (destination format);
— 0F0H–0F3H (spurious-interrupt vector);
— 280H–283H (error status);
— 300H–303H or 310H–313H (interrupt command);
— 320H–323H, 330H–333H, 340H–343H, 350H–353H, 360H–363H, or 370H–373H (LVT entries);
— 380H–383H (initial count); or
— 3E0H–3E3H (divide configuration).
Following this, the processor performs certain actions after completion of the operation
of which the access was a part. APIC-write emulation is described in Section 29.4.3.2

Section 29.4.3.2, Vol 3, SDM
080H (task priority) - do TPR virtualization
0B0H (end of interrupt) - EOI virtualization (apicv advanced used, so VID is enabled)
300H (interrupt command — low) - may causes an APIC-write VM exit
310H–313H (interrupt command — high). - No other virtualization or VM exit occurs.
Any other page offset. The processor causes an APIC-write VM exit.

So
— 020H–023H (local APIC ID);
— 080H–083H (task priority);
— 0D0H–0D3H (logical destination);
— 0E0H–0E3H (destination format);
— 0F0H–0F3H (spurious-interrupt vector);
— 280H–283H (error status);
— 300H–303H (interrupt command - low);
— 320H–323H, 330H–333H, 340H–343H, 350H–353H, 360H–363H, or 370H–373H (LVT entries);
— 380H–383H (initial count); or
— 3E0H–3E3H (divide configuration).
will trigger an APIC-write VM exit.



Anthony


-----Original Message-----
From: Li, Fei1
Sent: Thursday, March 21, 2019 6:04 PM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>; Xu, Anthony <anthony.xu@...>; Chen, Jason CJ
<jason.cj.chen@...>; Grandhi, Sainath <sainath.grandhi@...>
Subject: [RFC PATCH 10/11] hv: vlapic: minor fix about vlapic write

1) In x2apic, when read ICR, we want to read a 64-bits value.
2) In xapic mode, we would not write self-IPI since it's only aviable for x2apic mode.

Signed-off-by: Li, Fei1 <fei1.li@...>
---
hypervisor/arch/x86/guest/vlapic.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c b/hypervisor/arch/x86/guest/vlapic.c
index 18c7a3de0a07..b4bb68587bdb 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -94,6 +94,7 @@ apicv_set_intr_ready(struct acrn_vlapic *vlapic, uint32_t vector);

static void apicv_post_intr(uint16_t dest_pcpu_id);

+static void vlapic_x2apic_self_ipi_handler(struct acrn_vlapic *vlapic);
/*
* Post an interrupt to the vcpu running on 'hostcpu'. This will use a
* hardware assist if available (e.g. Posted Interrupt) or fall back to
@@ -1526,7 +1527,11 @@ vlapic_read(struct acrn_vlapic *vlapic, uint32_t offset_arg, uint64_t *data)
*data = lapic->esr.v;
break;
case APIC_OFFSET_ICR_LOW:
- *data = lapic->icr_lo.v;
+ if (is_x2apic_enabled(vlapic)) {
+ *data = (((uint64_t)lapic->icr_hi.v) << 32U) | lapic->icr_lo.v;
+ } else {
+ *data = lapic->icr_lo.v;
+ }
break;
case APIC_OFFSET_ICR_HI:
*data = lapic->icr_hi.v;
@@ -1564,7 +1569,7 @@ vlapic_read(struct acrn_vlapic *vlapic, uint32_t offset_arg, uint64_t *data)
}
}

- dev_dbg(ACRN_DBG_LAPIC, "vlapic read offset %#x, data %#lx", offset, *data);
+ dev_dbg(ACRN_DBG_LAPIC, "vlapic read offset %#x, data %#llx", offset, *data);
return ret;
}

@@ -1642,6 +1647,13 @@ vlapic_write(struct acrn_vlapic *vlapic, uint32_t offset, uint64_t data)
vlapic_esr_write_handler(vlapic);
break;

+ case APIC_OFFSET_SELF_IPI:
+ if (is_x2apic_enabled(vlapic)) {
+ lapic->self_ipi.v = data32;
+ vlapic_x2apic_self_ipi_handler(vlapic);
+ }
+ break;
+
default:
/* Read only */
ret = -EACCES;
@@ -2434,11 +2446,6 @@ int32_t apic_write_vmexit_handler(struct acrn_vcpu *vcpu)
case APIC_OFFSET_TIMER_DCR:
vlapic_dcr_write_handler(vlapic);
break;
- case APIC_OFFSET_SELF_IPI:
- if (is_x2apic_enabled(vlapic)) {
- vlapic_x2apic_self_ipi_handler(vlapic);
- }
- break;
default:
err = -EACCES;
pr_err("Unhandled APIC-Write, offset:0x%x", offset);
--
2.17.1


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Thursday, April 11, 2019 2:54 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 10:48:24PM +0800, Kaige Fu wrote:
On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and
correctness when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) &&
(timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will
this INIT signal trigger at the moment this cpu doing vmx_off? If yes, there
is no chance for this pcpu to set boot_state...

Yes. Once the VMX is turn off, the INIT signal blocked in VMX root-mode will
be triggered.
But, we didn't send INIT to this cpu before.
We pause_vcpu through sending INIT, not?
No. You may miss something.
When pause_vcpu, the target cpu stays at VMX non-root mode. So, this
signal just trigger vm-exit and be discarded. It is not blocked in this case.

BTW, fengwei's suggestion looks good to me :)


+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c
b/hypervisor/arch/x86/guest/vm.c index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and
correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void); void
load_cpu_state_data(void); void init_cpu_pre(uint16_t
pcpu_id_args); void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



--

Thanks

Jason


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Chen, Jason CJ
 

On Thu, Apr 11, 2019 at 10:48:24PM +0800, Kaige Fu wrote:
On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
Yes. Once the VMX is turn off, the INIT signal blocked in VMX root-mode will be triggered.
But, we didn't send INIT to this cpu before.
We pause_vcpu through sending INIT, not?

BTW, fengwei's suggestion looks good to me :)


+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



--

Thanks

Jason


Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-11 Thu 14:36, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
Yes. Once the VMX is turn off, the INIT signal blocked in VMX root-mode will be triggered.
But, we didn't send INIT to this cpu before.

+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason



Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/11/2019 2:36 PM, Chen, Jason CJ wrote:
On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}
+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
If so, let's move vmx_off to just before hlt_cpu in cpu_dead.
And add comments about this case?

Regards
Yin, Fengwei

+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0




Re: [PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Chen, Jason CJ
 

On Thu, Apr 11, 2019 at 02:24:08PM +0000, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
one question here, if a INIT under vmx_on state is unblocked, will this INIT signal trigger at the moment this cpu doing
vmx_off? If yes, there is no chance for this pcpu to set boot_state...
+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



--

Thanks

Jason


Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/11/2019 2:11 PM, Grandhi, Sainath wrote:

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Wednesday, April 10, 2019 10:12 PM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: Re: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 05:05:39AM +0000, Kaige Fu wrote:

-----Original Message-----
From: Dong, Eddie
Sent: Thursday, April 11, 2019 11:35 AM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of
lapic_pt vm when shutdown


For INIT signal, it will unblocked when turn VMX off:
Intel SDM Vol3 Chapter 33.2:
INIT assertions are always blocked in VMX root operation and
while in SMM, and unblocked otherwise.

Intel SDM Vol3 Chapter 30.3:
Takes the logical processor out of VMX operation, unblocks
INIT signals, conditionally re-enables A20M, and clears any
address-range monitoring
This is good to prove the blocked signal will be accepted after VMX OFF.

But, I have another question: Since the SOS VM/PCPU will send INIT
and SIPI, will the latterly sent SIPI signal overwrite INIT signal?
I.e. can the target CPU buffer the blocked INIT, and SIPI signal? I doubt...
I tried to find some statement about it but failed.

BTW, It is still better to send INIT/SIPI till VMX OFF is really
done in target PCPU side. Let us think of easy way to do so... For
example, can we do cpu_dead
Sure. May be we I do per Sainath's suggestion: Offline pcpu when pause_vm
and start it in shutdown_vm.
Kaige,
In the approach I suggested earlier, we cannot guarantee cpu_dead is called on
remote cpu, by the time shutdown_vm is called from SOS DM.
We get more time than your current approach, but no guarantee.
Can we use some of bitmap in memory to know whether cpu_dead is done on remote cpu?
Yes. We should change the stop_cpus a little bit to like
start_cpus/start_cpu. To check the bitmap. We didn't do that because
we have no requirement to offline one specific pCPU. We have such
requirement now.

Regards
Yin, Fengwei

why we cannot do all the thing in shutdown_vm? Like, if it's a lapit_pt cpu, then
we make it offline(cpu_dead) & start_cpu , we can even wrap a reset_cpu
function for it.
shutdown_vm is executing on SOS CPU and offline needs to be done on remote cpu before we send startup IPIs.


For SIPI signal, it says the SIPI will be blocked in VMX root
operation but doesn't say when to unblock it. From my test, it
will be unblocked after VMX off.
Intel SDM Vol3 Chapter 33.2:
SIPI events are always blocked in VMX root operation.

BTW, I see the following statement in SDM Vol3 Chapter 25.2:
Start-up IPIs (SIPIs). SIPIs cause VM exits. If a logical
processor is not in the wait-for-SIPI activity state when a SIPI
arrives, no VM exit occurs and the SIPI is discarded.

IMHO, if saying "block SIPI", it means the SIPI signal will be
unblocked. If saying "SIPI is discarded", it means the SIPI signal
will be
ignored.

What's your opinion here? Should we add some code the wait pcpu
offline or keep it as it is?



--

Thanks

Jason


Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-11 Thu 06:11, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Wednesday, April 10, 2019 10:12 PM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: Re: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 05:05:39AM +0000, Kaige Fu wrote:

-----Original Message-----
From: Dong, Eddie
Sent: Thursday, April 11, 2019 11:35 AM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of
lapic_pt vm when shutdown


For INIT signal, it will unblocked when turn VMX off:
Intel SDM Vol3 Chapter 33.2:
INIT assertions are always blocked in VMX root operation and
while in SMM, and unblocked otherwise.

Intel SDM Vol3 Chapter 30.3:
Takes the logical processor out of VMX operation, unblocks
INIT signals, conditionally re-enables A20M, and clears any
address-range monitoring
This is good to prove the blocked signal will be accepted after VMX OFF.

But, I have another question: Since the SOS VM/PCPU will send INIT
and SIPI, will the latterly sent SIPI signal overwrite INIT signal?
I.e. can the target CPU buffer the blocked INIT, and SIPI signal? I doubt...
I tried to find some statement about it but failed.

BTW, It is still better to send INIT/SIPI till VMX OFF is really
done in target PCPU side. Let us think of easy way to do so... For
example, can we do cpu_dead
Sure. May be we I do per Sainath's suggestion: Offline pcpu when pause_vm
and start it in shutdown_vm.
Kaige,
In the approach I suggested earlier, we cannot guarantee cpu_dead is called on
remote cpu, by the time shutdown_vm is called from SOS DM.
We get more time than your current approach, but no guarantee.
Can we use some of bitmap in memory to know whether cpu_dead is done on remote cpu?
We can check per-cpu boot_state. BTW, I have sent v3. Could you help review?

why we cannot do all the thing in shutdown_vm? Like, if it's a lapit_pt cpu, then
we make it offline(cpu_dead) & start_cpu , we can even wrap a reset_cpu
function for it.
shutdown_vm is executing on SOS CPU and offline needs to be done on remote cpu before we send startup IPIs.


For SIPI signal, it says the SIPI will be blocked in VMX root
operation but doesn't say when to unblock it. From my test, it
will be unblocked after VMX off.
Intel SDM Vol3 Chapter 33.2:
SIPI events are always blocked in VMX root operation.

BTW, I see the following statement in SDM Vol3 Chapter 25.2:
Start-up IPIs (SIPIs). SIPIs cause VM exits. If a logical
processor is not in the wait-for-SIPI activity state when a SIPI
arrives, no VM exit occurs and the SIPI is discarded.

IMHO, if saying "block SIPI", it means the SIPI signal will be
unblocked. If saying "SIPI is discarded", it means the SIPI signal
will be
ignored.

What's your opinion here? Should we add some code the wait pcpu
offline or keep it as it is?



--

Thanks

Jason




Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/11/2019 11:19 AM, Grandhi, Sainath wrote:
This solution of "waiting" can work because today ACRN uses "pause" instead of "halt" in cpu_do_idle
Any reason we chose pause over halt?
This is from performance perspective. halt will enter C state and back
to run state takes more time.

Regards
Yin, Fengwei


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Kaige Fu
Sent: Wednesday, April 10, 2019 12:10 AM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: Re: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

Hi Eddie,

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Eddie Dong
Sent: Wednesday, April 10, 2019 11:59 AM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: Re: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of
lapic_pt vm when shutdown

Not sure how you address the following issues:
make_pcpu_offline relies on send_single_ipi(vcpu->pcpu_id,
VECTOR_NOTIFY_VCPU), which further relies on the scheduler to tick
to execute cpu_dead()... In this case, the caller PCPU needs to wait
till the target PCPU is VMX off. I didn't see this waiting
mechanism... Did I miss
something?

Here is what I see from my test: INIT/SIPI signal will be blocked in
VMX root-mode, but it will be delivered the signals to the target
cpu once we turn the VMX off. So the above code works.

But it is better to wait here per suggestion.
Do you read "blocked" as "deferred till switching back to VMX off.

If this is clearly stated in SDM. That is fine.
Try to find data.

For INIT signal, it will unblocked when turn VMX off:
Intel SDM Vol3 Chapter 33.2:
INIT assertions are always blocked in VMX root operation and while in SMM,
and unblocked otherwise.

Intel SDM Vol3 Chapter 30.3:
Takes the logical processor out of VMX operation, unblocks INIT signals,
conditionally re-enables A20M, and clears any address-range monitoring

For SIPI signal, it says the SIPI will be blocked in VMX root operation but doesn't
say when to unblock it. From my test, it will be unblocked after VMX off.
Intel SDM Vol3 Chapter 33.2:
SIPI events are always blocked in VMX root operation.

BTW, I see the following statement in SDM Vol3 Chapter 25.2:
Start-up IPIs (SIPIs). SIPIs cause VM exits. If a logical processor is not in the
wait-for-SIPI activity state when a SIPI arrives, no VM exit occurs and the SIPI is
discarded.

IMHO, if saying "block SIPI", it means the SIPI signal will be unblocked. If saying
"SIPI is discarded", it means the SIPI signal will be ignored.

What's your opinion here? Should we add some code the wait pcpu offline or
keep it as it is?



[PATCH v3 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 30 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..32fb8893 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -293,6 +293,30 @@ static void start_cpu(uint16_t pcpu_id)
}
}

+void reset_cpu(uint16_t pcpu_id)
+{
+ uint32_t timeout;
+
+ make_pcpu_offline(pcpu_id);
+
+ timeout = (uint32_t)CONFIG_CPU_UP_TIMEOUT * 1000U;
+ while ((per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) && (timeout != 0U)) {
+ udelay(10U);
+ timeout -= 10U;
+ }
+
+ /* Check to see if expected CPU is actually in dead state */
+ if (per_cpu(boot_state, pcpu_id) != PCPU_STATE_DEAD) {
+ pr_fatal("Failed to offline pcpu%hu", pcpu_id);
+
+ /* Error condition - loop endlessly for now */
+ do {
+ } while (1);
+ }
+
+ start_cpu(pcpu_id);
+}
+
void start_cpus(void)
{
uint16_t i;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..82a11f1c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -492,6 +492,11 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ reset_cpu(vcpu->pcpu_id);
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..004276ed 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void reset_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0


[PATCH v3 1/2] HV: Assign the guest_flags with cv.vm_flag

Kaige Fu
 

We should assign the guest_flags with cv.vm_flag using '=' instead of '|='.
Because the later one will keep the previous configurations.

For example, if we create one vm with LAPIC_PASSTHROUGH flag and shutdown it.
The the following vm to be bootup will has the LAPIC_PASSTHROUGH flag set no
matter whether we set it in DM.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/common/hypercall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 96b2fab1..a871755a 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -133,7 +133,7 @@ int32_t hcall_create_vm(struct acrn_vm *vm, uint64_t param)
vm_config = get_vm_config(vm_id);
/* TODO: set by DM */
vm_config->type = NORMAL_VM;
- vm_config->guest_flags |= cv.vm_flag;
+ vm_config->guest_flags = cv.vm_flag;
(void)memcpy_s(&vm_config->GUID[0], 16U, &cv.GUID[0], 16U);

/* GUEST_FLAG_RT must be set if we have GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
--
2.20.0


[PATCH v3 0/2] Minor fixes about vm and vm_config and lapic_pt vm

Kaige Fu
 

Patch 1/2 reset vm and vm_config structure to initial value when shutdown the vm.

Patch 2/2 fixes the following issue.
The normal vm can't boot up on the pcpu which ever run lapic_pt vm. The reason
is that the guest will directly write to the passthroughed physical lapic msrs.
The lapic will stay at disabled state or other invalid state. For example, the
lapic will stay at software disabled state when shutdown the linux guest with LAPIC_PT.

So the 2/2 patch reset the physical core of lapic_pt vm when shutdown the vm for security
and correctness. Physical core and lapic will return to initial state after reset.

---
v2 -> v3:
- Just reset vm_config->guest_flags instead of whole structure.
- Wait the target cpu really enter into dead state before start_cpu and wrap it as reset_cpu.

v1 -> v2:
- Reset physical core of lapic_pt vm instead of save/restore lapic msrs.
- Change the cover letter title "Save/restore LAPIC MSRs for LAPIC_PT vm".

Kaige Fu (2):
HV: Assign the guest_flags with cv.vm_flag
HV: Reset physical core of lapic_pt vm when shutdown

hypervisor/arch/x86/cpu.c | 24 ++++++++++++++++++++++++
hypervisor/arch/x86/guest/vm.c | 5 +++++
hypervisor/common/hypercall.c | 2 +-
hypervisor/include/arch/x86/cpu.h | 1 +
4 files changed, 31 insertions(+), 1 deletion(-)

--
2.20.0


Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Chen, Jason CJ
 

On Thu, Apr 11, 2019 at 06:11:58AM +0000, Grandhi, Sainath wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Chen, Jason CJ
Sent: Wednesday, April 10, 2019 10:12 PM
To: acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: Re: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

On Thu, Apr 11, 2019 at 05:05:39AM +0000, Kaige Fu wrote:

-----Original Message-----
From: Dong, Eddie
Sent: Thursday, April 11, 2019 11:35 AM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of
lapic_pt vm when shutdown


For INIT signal, it will unblocked when turn VMX off:
Intel SDM Vol3 Chapter 33.2:
INIT assertions are always blocked in VMX root operation and
while in SMM, and unblocked otherwise.

Intel SDM Vol3 Chapter 30.3:
Takes the logical processor out of VMX operation, unblocks
INIT signals, conditionally re-enables A20M, and clears any
address-range monitoring
This is good to prove the blocked signal will be accepted after VMX OFF.

But, I have another question: Since the SOS VM/PCPU will send INIT
and SIPI, will the latterly sent SIPI signal overwrite INIT signal?
I.e. can the target CPU buffer the blocked INIT, and SIPI signal? I doubt...
I tried to find some statement about it but failed.

BTW, It is still better to send INIT/SIPI till VMX OFF is really
done in target PCPU side. Let us think of easy way to do so... For
example, can we do cpu_dead
Sure. May be we I do per Sainath's suggestion: Offline pcpu when pause_vm
and start it in shutdown_vm.
Kaige,
In the approach I suggested earlier, we cannot guarantee cpu_dead is called on
remote cpu, by the time shutdown_vm is called from SOS DM.
We get more time than your current approach, but no guarantee.
Can we use some of bitmap in memory to know whether cpu_dead is done on remote cpu?
why we cannot do all the thing in shutdown_vm? Like, if it's a lapit_pt cpu, then
we make it offline(cpu_dead) & start_cpu , we can even wrap a reset_cpu
function for it.
shutdown_vm is executing on SOS CPU and offline needs to be done on remote cpu before we send startup IPIs.
yes, here we should use a sync method for reset_cpu called from shutdown_vm:
1. make offline request to target cpu
I have a talk with Kaige about this, there is a concern about if cpu is idle in "hlt", then we may not be able to
wake up target cpu when it's in vmx_on state; but currently, we are using "pause" for idle, and in the future, we
suppose to use mwait. So it seemed no problem for me.
2. wait target cpu offline
3. start_cpu by sending INIT-SIPI



For SIPI signal, it says the SIPI will be blocked in VMX root
operation but doesn't say when to unblock it. From my test, it
will be unblocked after VMX off.
Intel SDM Vol3 Chapter 33.2:
SIPI events are always blocked in VMX root operation.

BTW, I see the following statement in SDM Vol3 Chapter 25.2:
Start-up IPIs (SIPIs). SIPIs cause VM exits. If a logical
processor is not in the wait-for-SIPI activity state when a SIPI
arrives, no VM exit occurs and the SIPI is discarded.

IMHO, if saying "block SIPI", it means the SIPI signal will be
unblocked. If saying "SIPI is discarded", it means the SIPI signal
will be
ignored.

What's your opinion here? Should we add some code the wait pcpu
offline or keep it as it is?



--

Thanks

Jason



--

Thanks

Jason

16681 - 16700 of 37344