1. Patchew
  2. QEMU
  3. [PATCH v1 0/3] hvf: Support AVX512 guests and cleanup
  4. View patch

[PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.

Cameron Esfahani via posted 3 patches 5 years, 10 months ago
Maintainers: Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>, Roman Bolshakov <r.bolshakov@yadro.com>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.
Posted by Cameron Esfahani via 5 years, 10 months ago 
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
Re: [PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.
Posted by Roman Bolshakov 5 years, 10 months ago 
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
Re: [PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.
Posted by Cameron Esfahani via 5 years, 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.