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

[Paolo Bonzini]

On 30/06/2018 07:25, Jan Kiszka wrote:
> 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?

Yes, hflags become flags of the translation block, while hflags2 are
just random processor state.  If translate.c uses it you must use
hflags, but here hflags2 should be safe.

Thanks,

Paolo

>>>
>>> +
>>> +        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
>