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

[Cameron Esfahani]

I'll update with your feedback.

Cameron Esfahani
address@hidden

"We do what we must because we can."

Aperture Science



> On Apr 5, 2020, at 11:51 AM, Roman Bolshakov <address@hidden> wrote:
> 
> 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