| Message ID | 17777cc82122d29903bad7268b4c33e83b27d9a6.1585607927.git.dirty@apple.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hvf: Support AVX512 guests and cleanup | expand |
On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote: > Signed-off-by: Cameron Esfahani <dirty@apple.com> > --- > target/i386/hvf/vmx.h | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h > index 8ec2e6414e..1a1b150c97 100644 > --- a/target/i386/hvf/vmx.h > +++ b/target/i386/hvf/vmx.h > @@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) > uint64_t pdpte[4] = {0, 0, 0, 0}; > uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER); > uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0); > + uint64_t changed_cr0 = old_cr0 ^ cr0; > uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK | > CR0_NE_MASK | CR0_ET_MASK; > > @@ -139,11 +140,12 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) > wvmcs(vcpu, VMCS_CR0_SHADOW, cr0); > > if (efer & MSR_EFER_LME) { > - if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) { > - enter_long_mode(vcpu, cr0, efer); > - } > - if (!(cr0 & CR0_PG_MASK)) { > - exit_long_mode(vcpu, cr0, efer); > + if (changed_cr0 & CR0_PG_MASK) { > + if (cr0 & CR0_PG_MASK) { > + enter_long_mode(vcpu, cr0, efer); > + } else { > + exit_long_mode(vcpu, cr0, efer); > + } > } > } > > -- > 2.24.0 > The changes look good but I have a few nitpicks. The summary line should not have "." at the end, please see (https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message): > Whether the "single line summary of change" starts with a capital is a > matter of taste, but we prefer that the summary does not end in "." Also, it would be good to mention in the title/commit message that with the change QEMU is skipping unconditional writes to the guest IA32_EFER.LMA and VMCS Entry Controls in compatibility mode, instead it does so only when the actual switch out of long mode happens. (It's worth to mention any other issues the patch helps to address, if any). The comment in the previous patch may be dropped here IMO. Besides that, Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> Thanks, Roman
I'll update with your feedback. Cameron Esfahani dirty@apple.com "We do what we must because we can." Aperture Science > On Apr 5, 2020, at 11:51 AM, Roman Bolshakov <r.bolshakov@yadro.com> wrote: > > On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote: >> Signed-off-by: Cameron Esfahani <dirty@apple.com> >> --- >> target/i386/hvf/vmx.h | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h >> index 8ec2e6414e..1a1b150c97 100644 >> --- a/target/i386/hvf/vmx.h >> +++ b/target/i386/hvf/vmx.h >> @@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) >> uint64_t pdpte[4] = {0, 0, 0, 0}; >> uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER); >> uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0); >> + uint64_t changed_cr0 = old_cr0 ^ cr0; >> uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK | >> CR0_NE_MASK | CR0_ET_MASK; >> >> @@ -139,11 +140,12 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) >> wvmcs(vcpu, VMCS_CR0_SHADOW, cr0); >> >> if (efer & MSR_EFER_LME) { >> - if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) { >> - enter_long_mode(vcpu, cr0, efer); >> - } >> - if (!(cr0 & CR0_PG_MASK)) { >> - exit_long_mode(vcpu, cr0, efer); >> + if (changed_cr0 & CR0_PG_MASK) { >> + if (cr0 & CR0_PG_MASK) { >> + enter_long_mode(vcpu, cr0, efer); >> + } else { >> + exit_long_mode(vcpu, cr0, efer); >> + } >> } >> } >> >> -- >> 2.24.0 >> > > The changes look good but I have a few nitpicks. > > The summary line should not have "." at the end, please see > (https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message): >> Whether the "single line summary of change" starts with a capital is a >> matter of taste, but we prefer that the summary does not end in "." > > Also, it would be good to mention in the title/commit message that with the > change QEMU is skipping unconditional writes to the guest IA32_EFER.LMA > and VMCS Entry Controls in compatibility mode, instead it does so only > when the actual switch out of long mode happens. (It's worth to mention > any other issues the patch helps to address, if any). > > The comment in the previous patch may be dropped here IMO. > > Besides that, > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > > Thanks, > Roman
diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h index 8ec2e6414e..1a1b150c97 100644 --- a/target/i386/hvf/vmx.h +++ b/target/i386/hvf/vmx.h @@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) uint64_t pdpte[4] = {0, 0, 0, 0}; uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER); uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0); + uint64_t changed_cr0 = old_cr0 ^ cr0; uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK | CR0_NE_MASK | CR0_ET_MASK; @@ -139,11 +140,12 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0) wvmcs(vcpu, VMCS_CR0_SHADOW, cr0); if (efer & MSR_EFER_LME) { - if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) { - enter_long_mode(vcpu, cr0, efer); - } - if (!(cr0 & CR0_PG_MASK)) { - exit_long_mode(vcpu, cr0, efer); + if (changed_cr0 & CR0_PG_MASK) { + if (cr0 & CR0_PG_MASK) { + enter_long_mode(vcpu, cr0, efer); + } else { + exit_long_mode(vcpu, cr0, efer); + } } }
Signed-off-by: Cameron Esfahani <dirty@apple.com> --- target/i386/hvf/vmx.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)