| Message ID | 20200624225850.16982-7-r.bolshakov@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve synchronization between QEMU and HVF | expand |
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
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
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
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
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
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 --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);
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(-)