[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/3] hvf: use standard CR0 and CR4 register definitions
From: |
Cameron Esfahani |
Subject: |
Re: [PATCH v1 1/3] hvf: use standard CR0 and CR4 register definitions |
Date: |
Tue, 07 Apr 2020 23:09:12 -0700 |
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