diff mbox series

[6/8] i386: hvf: Drop hvf_reset_vcpu()

Message ID 20200624225850.16982-7-r.bolshakov@yadro.com (mailing list archive)
State New, archived
Headers show
Series Improve synchronization between QEMU and HVF | expand

Commit Message

Roman Bolshakov June 24, 2020, 10:58 p.m. UTC
It's worth to have a custom accel-specific reset in x86_cpu_reset() only
if something related to CPUState has to be reset and that can't be done
in post-init or post-reset.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 include/sysemu/hvf.h  |  1 -
 target/i386/cpu.c     |  3 ---
 target/i386/hvf/hvf.c | 23 +++++++++++------------
 3 files changed, 11 insertions(+), 16 deletions(-)

Comments

Paolo Bonzini June 25, 2020, 10:31 a.m. UTC | #1
On 25/06/20 00:58, Roman Bolshakov wrote:
> +    uint64_t pdpte[4] = {0, 0, 0, 0};
> +    int i;
> +
> +    /* Reset IA-32e mode guest (LMA) */
> +    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
> +

Where is the place (if any...) that calls macvm_set_cr0 and
macvm_set_cr4 from cpu_synchronize_*?  If you have such a place it
should take care of resetting LMA as well.  Assuming that no entry
controls are ever set is quite fragile.

Paolo
Roman Bolshakov June 25, 2020, 12:36 p.m. UTC | #2
On Thu, Jun 25, 2020 at 12:31:49PM +0200, Paolo Bonzini wrote:
> On 25/06/20 00:58, Roman Bolshakov wrote:
> > +    uint64_t pdpte[4] = {0, 0, 0, 0};
> > +    int i;
> > +
> > +    /* Reset IA-32e mode guest (LMA) */
> > +    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
> > +
> 
> Where is the place (if any...) that calls macvm_set_cr0 and
> macvm_set_cr4 from cpu_synchronize_*?  If you have such a place it
> should take care of resetting LMA as well.  Assuming that no entry
> controls are ever set is quite fragile.
> 

Hi Paolo,

Yes, there's such a place. post-init and post-reset invoke
hvf_put_registers() and the latter one calls hvf_put_segments().
hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
using the CR0/CR4 from env. So, the reset is relying on generic QEMU
CPUX86State now. LMA in EFER is reset there as well.

I don't know any alternative for PDPTE and VMCS Entry Controls in
CPUX86State, that's why I left explicit reset of the VMCS fields in
post-reset.

Is there an outstanding issue I'm missing?

Regards,
Roman
Paolo Bonzini June 25, 2020, 1:30 p.m. UTC | #3
On 25/06/20 14:36, Roman Bolshakov wrote:
> 
> Yes, there's such a place. post-init and post-reset invoke
> hvf_put_registers() and the latter one calls hvf_put_segments().
> hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
> using the CR0/CR4 from env. So, the reset is relying on generic QEMU
> CPUX86State now. LMA in EFER is reset there as well.

Ok, do you want to send a follow-up or a v2 of this?

> I don't know any alternative for PDPTE and VMCS Entry Controls in
> CPUX86State, that's why I left explicit reset of the VMCS fields in
> post-reset.

VMCS entry controls should be handled by macvm_set_cr0 as well, because
QEMU does not use any except for the LMA bit.  They are initialized zero

    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));

but in practice the last argument ends up being zero all the time.

PDPTEs are not a problem, because they are not used after reset (only if
CR4.PAE=CR4.PG=1 and EFER.LME=0).

Thanks,

Paolo
Roman Bolshakov June 25, 2020, 3:02 p.m. UTC | #4
On Thu, Jun 25, 2020 at 03:30:38PM +0200, Paolo Bonzini wrote:
> On 25/06/20 14:36, Roman Bolshakov wrote:
> > 
> > Yes, there's such a place. post-init and post-reset invoke
> > hvf_put_registers() and the latter one calls hvf_put_segments().
> > hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
> > using the CR0/CR4 from env. So, the reset is relying on generic QEMU
> > CPUX86State now. LMA in EFER is reset there as well.
> 
> Ok, do you want to send a follow-up or a v2 of this?
> 

I'm still trying to understand what I should do :)

> > I don't know any alternative for PDPTE and VMCS Entry Controls in
> > CPUX86State, that's why I left explicit reset of the VMCS fields in
> > post-reset.
> 
> VMCS entry controls should be handled by macvm_set_cr0 as well, because
> QEMU does not use any except for the LMA bit.  They are initialized zero
> 
>     wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
> 	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));
> 
> but in practice the last argument ends up being zero all the time.
> 

macvm_set_cr0() sets/clears LMA in entry controls only in case of
transitions into/out of long mode in enter_long_mode() in
exit_long_mode(), respectively. But macvm_set_cr0() doesn't load
EFER.LMA from CPUX86State into VMCS entry controls during reset and
that's where hvf_put_registers() might not behave properly.

As far as I understand you propose to drop explicit LMA reset in
post-reset and rather impove synchronization between efer and entry
controls in macvm_set_cr0(), right? In that case I don't see a
regression in the series, and if possible I'd prefer a follow up patch
for the issue.

> PDPTEs are not a problem, because they are not used after reset (only if
> CR4.PAE=CR4.PG=1 and EFER.LME=0).
> 

Ok, good, then we're leaving PDPTE initialization as is in post-reset.

Thanks,
Roman
Paolo Bonzini June 25, 2020, 6:26 p.m. UTC | #5
On 25/06/20 17:02, Roman Bolshakov wrote:
> macvm_set_cr0() sets/clears LMA in entry controls only in case of
> transitions into/out of long mode in enter_long_mode() in
> exit_long_mode(), respectively. But macvm_set_cr0() doesn't load
> EFER.LMA from CPUX86State into VMCS entry controls during reset and
> that's where hvf_put_registers() might not behave properly.
> 
> As far as I understand you propose to drop explicit LMA reset in
> post-reset and rather impove synchronization between efer and entry
> controls in macvm_set_cr0(), right? In that case I don't see a
> regression in the series, and if possible I'd prefer a follow up patch
> for the issue.

Indeed it's not a regression.  Thanks!

Paolo
Roman Bolshakov June 29, 2020, 12:58 p.m. UTC | #6
On Thu, Jun 25, 2020 at 03:30:38PM +0200, Paolo Bonzini wrote:
> On 25/06/20 14:36, Roman Bolshakov wrote:
> > I don't know any alternative for PDPTE and VMCS Entry Controls in
> > CPUX86State, that's why I left explicit reset of the VMCS fields in
> > post-reset.
> 
> VMCS entry controls should be handled by macvm_set_cr0 as well, because
> QEMU does not use any except for the LMA bit.  They are initialized zero
> 
>     wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
> 	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));
> 
> but in practice the last argument ends up being zero all the time.
> 
> PDPTEs are not a problem, because they are not used after reset (only if
> CR4.PAE=CR4.PG=1 and EFER.LME=0).
> 

Paolo, I'm not sure if I properly understand it yet but I don't see any
specific requirements on PDPTE's reset. SDM says that it should be valid
only if PAE is used as documented in 26.3.1.6 Checks on Guest
Page-Directory-Pointer-Table Entries:
 "A VM entry to a guest that does not use PAE paging does not check the
 validity of any PDPTEs.

 A VM entry that checks the validity of the PDPTEs uses the same checks
 that are used when CR3 is loaded with MOV to CR3 when PAE paging is in
 use. If MOV to CR3 would cause a general-protection exception due to
 the PDPTEs that would be loaded (e.g., because a reserved bit is set),
 the VM entry fails."

That means we can drop PDPTE initialization in hv_vcpu_reset() as well.
Perhaps, I can try that and check if Windows XP with PAE support works
well alright across resets. macvm_set_cr0() takes care of setting PDPTEs
upon the entry into PAE mode.

Regards,
Roman
diff mbox series

Patch

diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
index aaa00cbf05..a1ab61403f 100644
--- a/include/sysemu/hvf.h
+++ b/include/sysemu/hvf.h
@@ -31,7 +31,6 @@  void hvf_cpu_synchronize_post_reset(CPUState *);
 void hvf_cpu_synchronize_post_init(CPUState *);
 void hvf_cpu_synchronize_pre_loadvm(CPUState *);
 void hvf_vcpu_destroy(CPUState *);
-void hvf_reset_vcpu(CPUState *);
 
 #define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b1b311baa2..bfa3eed9b6 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6089,9 +6089,6 @@  static void x86_cpu_reset(DeviceState *dev)
     if (kvm_enabled()) {
         kvm_arch_reset_vcpu(cpu);
     }
-    else if (hvf_enabled()) {
-        hvf_reset_vcpu(s);
-    }
 #endif
 }
 
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 2c4028d08c..0b2be8de47 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -303,6 +303,17 @@  void hvf_cpu_synchronize_state(CPUState *cpu_state)
 static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
 {
     CPUState *cpu_state = cpu;
+    uint64_t pdpte[4] = {0, 0, 0, 0};
+    int i;
+
+    /* Reset IA-32e mode guest (LMA) */
+    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
+
+    /* Initialize PDPTE */
+    for (i = 0; i < 4; i++) {
+        wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
+    }
+
     hvf_put_registers(cpu_state);
     cpu_state->vcpu_dirty = false;
 }
@@ -452,18 +463,6 @@  static MemoryListener hvf_memory_listener = {
     .log_sync = hvf_log_sync,
 };
 
-void hvf_reset_vcpu(CPUState *cpu) {
-    uint64_t pdpte[4] = {0, 0, 0, 0};
-    int i;
-
-    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
-
-    /* Initialize PDPTE */
-    for (i = 0; i < 4; i++) {
-        wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
-    }
-}
-
 void hvf_vcpu_destroy(CPUState *cpu)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);