Re: [Qemu-devel] [PATCH v2 4/4] target-i386: Add NPT support

[Jan Kiszka]

On 2018-06-27 14:14, Paolo Bonzini wrote:
> On 03/04/2018 17:36, Jan Kiszka wrote:
>>  
>> +static hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType 
>> access_type,
>> +                        int *prot)
>> +{
>> +    CPUX86State *env = &X86_CPU(cs)->env;
>> +    uint64_t rsvd_mask = PG_HI_RSVD_MASK;
>> +    uint64_t ptep, pte;
>> +    uint64_t exit_info_1 = 0;
>> +    target_ulong pde_addr, pte_addr;
>> +    uint32_t page_offset;
>> +    int page_size;
>> +
>> +    if (likely(!(env->hflags & HF_NPT_MASK))) {
>> +        return gphys;
>> +    }
> 
> hflags are a somewhat limited resource.  Can this go in hflags2?
> 

Will have a look - I don't seen why not. Or is there any special
semantical difference between both fields?

>>
>> +
>> +        env->nested_pg_mode = 0;
>> +        if (env->cr[4] & CR4_PAE_MASK) {
>> +            env->nested_pg_mode |= SVM_NPT_PAE;
>> +        }
>> +        if (env->hflags & HF_LMA_MASK) {
>> +            env->nested_pg_mode |= SVM_NPT_LMA;
>> +        }
>> +        if (env->efer & MSR_EFER_NXE) {
>> +            env->nested_pg_mode |= SVM_NPT_NXE;
>> +        }
>> +    }
>> +
> 
> This needs to be migrated.  You can put it in a subsection, conditional
> on hflags & HF_SVMI_MASK.

OK.

> 
> Also, do you need to flush the TLB unconditionally, even if CR0.PG is zero?
Cannot follow you here yet. What flush are you referring to?

Also, CR0.PG would not reflect if NPT is on, which now also contributes
to our TLB.

> 
> Otherwise looks good.  I have queued patches 1-3, but hopefully this one
> can go in the next release too.  Sorry for the delayed review.

No problem.

Thanks,
Jan

signature.asc
Description: OpenPGP digital signature