Re: [PATCH v2 2/3] nVMX: Implement emulated Page Modification Logging

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/05/2017 12:48, Huang, Kai wrote:
>>
>> -    if (fault->error_code & PFERR_RSVD_MASK)
>> +    if (vmx->nested.pml_full) {
>> +        exit_reason = EXIT_REASON_PML_FULL;
>> +        vmx->nested.pml_full = false;
>> +        exit_qualification &= INTR_INFO_UNBLOCK_NMI;
> 
> Sorry I cannot recall the details. probably better to add a comment to
> indicate why mask out INTR_INFO_UNBLOCK_NMI?

It only keeps that bit, because it's the only exit qualification bit
defined for PML full vmexits (27.2.2).

>> +    if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
>> +        if (!nested_cpu_has_ept(vmcs12) ||
>> +            !IS_ALIGNED(address, 4096)  ||
>> +            address >> maxphyaddr)
>> +            return -EINVAL;
>> +    }
> 
> Do we also need to check whether EPT A/D has been enabled for vmcs12 to
> make vmentry work? I cannot recall details but probably not necessary.

The SDM doesn't say so.  The "if" above matches the checks in 26.2.1.1
of the manual (Checks on VMX Controls, VM-Execution Control Fields).


>> +        /*
>> +         * Check if PML is enabled for the nested guest.
>> +         * Whether eptp bit 6 is set is already checked
>> +         * as part of A/D emulation.
>> +         */
>> +        vmcs12 = get_vmcs12(vcpu);
>> +        if (!nested_cpu_has_pml(vmcs12))
>> +            return 0;
> 
> Since above nested_vmx_check_pml_controls doesn't check EPT A/D bit in
> L1, seems we need to add this check here.

If EPT A/D is disabled we cannot get here (update_accessed_dirty_bits is
never called).

>> +
>> +        if (vmcs12->guest_pml_index > PML_ENTITY_NUM) {
>> +            vmx->nested.pml_full = true;
>> +            return 1;
>> +        }
> 
> Is the purpose of returning 1 to make upper layer code to inject PML
> full VMEXIt to L1 in nested_ept_inject_page_fault?

Yes, it triggers a fault
>> +
>> +        gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS) & ~0xFFFull;
>> +
>> +        page = nested_get_page(vcpu, vmcs12->pml_address);
>> +        if (!page)
>> +            return 0;
> 
> If PML is enabled in L1, I think nested_get_page should never return a
> NULL PML page (unless L1 does something wrong)? Probably better to
> return 1 rather than 0, and handle error in nested_ept_inject_page_fault
> according to vmcs12->pml_address?

This happens if the PML address is invalid (where on real hardware, the
write would just be "eaten") or MMIO (where we expect to diverge from
real hardware behavior).

>> +
>> +        pml_address = kmap(page);
>> +        pml_address[vmcs12->guest_pml_index--] = gpa;
> 
> This gpa is L2 guest's GPA. Do we also need to mark L1's GPA (which is
> related to L2 guest's GPA above) in to dirty-log? Or has this already
> been done?

L1's PML contains L1 host physical addresses, i.e. L0 guest physical
addresses.  This GPA comes from vmcs02 and hence it is L0's GPA.

L0's HPA is marked by hardware through PML, as usual.  If L0 has EPT A/D
but not PML, it can still provide emulated PML to L1, but L0's HPA will
be marked as dirty via write protection.

Paolo