Re: [PATCH v1 1/3] hvf: use standard CR0 and CR4 register definitions

[Cameron Esfahani]

Responses inline

Cameron Esfahani
address@hidden

"We do what we must because we can."

Aperture Science



> On Apr 5, 2020, at 10:58 AM, Roman Bolshakov <address@hidden> wrote:
> 
> On Mon, Mar 30, 2020 at 05:16:04PM -0700, Cameron Esfahani wrote:
>> Signed-off-by: Cameron Esfahani <address@hidden>
>> ---
>> target/i386/cpu.h          |  2 ++
>> target/i386/hvf/hvf.c      |  1 +
>> target/i386/hvf/vmx.h      | 15 ++++++++-------
>> target/i386/hvf/x86.c      |  6 +++---
>> target/i386/hvf/x86.h      | 34 ----------------------------------
>> target/i386/hvf/x86_mmu.c  |  2 +-
>> target/i386/hvf/x86_task.c |  3 ++-
>> 7 files changed, 17 insertions(+), 46 deletions(-)
>> 
> 
> Hi Cameron,
> 
> I'm sorry for delay.
> 
> This is fun, I had very similar changeset I forgot to send quite a while
> ago:
> https://github.com/roolebo/qemu/commits/hvf-common-cr-constants
> 
>> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>> index d72543dc31..fef1ee7d70 100644
>> --- a/target/i386/hvf/hvf.c
>> +++ b/target/i386/hvf/hvf.c
>> @@ -455,6 +455,7 @@ void hvf_reset_vcpu(CPUState *cpu) {
>>         wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
>>     }
>> 
>> +    macvm_set_cr0(cpu->hvf_fd, CR0_CD_MASK | CR0_NW_MASK | CR0_ET_MASK);
>>     macvm_set_cr0(cpu->hvf_fd, 0x60000010);
> 
> The second macvm_set_cr0() is a duplicate of the first one and should be
> dropped.
> 

I'll fix it in next patch update, pending feedback from next issue.

>> 
>>     wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK);
>> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
>> index 03d2c79b9c..8ec2e6414e 100644
>> --- a/target/i386/hvf/vmx.h
>> +++ b/target/i386/hvf/vmx.h
>> @@ -138,17 +139,17 @@ 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) && (cr0 & CR0_PG)) {
>> +        if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) {
>>             enter_long_mode(vcpu, cr0, efer);
>>         }
>> -        if (/*(old_cr0 & CR0_PG) &&*/ !(cr0 & CR0_PG)) {
>> +        if (!(cr0 & CR0_PG_MASK)) {
> 
> IMO the patch should only change CR0_PG to CR0_PG_MASK without removal
> of the commented condition.
> 
> In the next patch you're improving how long mode exit is done and
> replacement of the comment with an implementation fits better there.
> 

The reason I removed that code was because checkpatch.pl scolded me for a patch 
with code commented out.

I assumed that I'd get a similar warning from patchew.org about some erroneous 
coding styles.

So I thought the easiest thing would be to remove that code as well.

But I'll defer to you or Paolo: should I remove that commented code with this 
patch?

>>             exit_long_mode(vcpu, cr0, efer);
>>         }
>>     }
>> 
>>     /* Filter new CR0 after we are finished examining it above. */
>> -    cr0 = (cr0 & ~(mask & ~CR0_PG));
>> -    wvmcs(vcpu, VMCS_GUEST_CR0, cr0 | CR0_NE | CR0_ET);
>> +    cr0 = (cr0 & ~(mask & ~CR0_PG_MASK));
>> +    wvmcs(vcpu, VMCS_GUEST_CR0, cr0 | CR0_NE_MASK | CR0_ET_MASK);
>> 
>>     hv_vcpu_invalidate_tlb(vcpu);
>>     hv_vcpu_flush(vcpu);
>> -- 
>> 2.24.0
>> 
> 
> Best regards,
> Roman