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

[Roman Bolshakov]

On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote:
> Signed-off-by: Cameron Esfahani <address@hidden>
> ---
>  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 <address@hidden>

Thanks,
Roman