I'll update with your feedback.
Cameron Esfahani
di...@apple.com
"We do what we must because we can."
Aperture Science
> On Apr 5, 2020, at 11:51 AM, Roman Bolshakov <r.bolsha...@yadro.com> wrote:
>
> On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote:
>> Signed-off-by: Cameron Esfahani <di...@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.bolsha...@yadro.com>
>
> Thanks,
> Roman
