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

["Huang, Kai" <kai.huang@linux.intel.com>]

On 5/6/2017 7:25 AM, Bandan Das wrote:
> With EPT A/D enabled, processor access to L2 guest
> paging structures will result in a write violation.
> When this happens, write the GUEST_PHYSICAL_ADDRESS
> to the pml buffer provided by L1 if the access is
> write and the dirty bit is being set.
>
> This patch also adds necessary checks during VMEntry if L1
> has enabled PML. If the PML index overflows, we change the
> exit reason and run L1 to simulate a PML full event.
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 2211697..8b9e942 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -248,6 +248,7 @@ struct __packed vmcs12 {
>  	u64 xss_exit_bitmap;
>  	u64 guest_physical_address;
>  	u64 vmcs_link_pointer;
> +	u64 pml_address;
>  	u64 guest_ia32_debugctl;
>  	u64 guest_ia32_pat;
>  	u64 guest_ia32_efer;
> @@ -369,6 +370,7 @@ struct __packed vmcs12 {
>  	u16 guest_ldtr_selector;
>  	u16 guest_tr_selector;
>  	u16 guest_intr_status;
> +	u16 guest_pml_index;
>  	u16 host_es_selector;
>  	u16 host_cs_selector;
>  	u16 host_ss_selector;
> @@ -407,6 +409,7 @@ struct nested_vmx {
>  	/* Has the level1 guest done vmxon? */
>  	bool vmxon;
>  	gpa_t vmxon_ptr;
> +	bool pml_full;
>
>  	/* The guest-physical address of the current VMCS L1 keeps for L2 */
>  	gpa_t current_vmptr;
> @@ -742,6 +745,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
>  	FIELD(GUEST_LDTR_SELECTOR, guest_ldtr_selector),
>  	FIELD(GUEST_TR_SELECTOR, guest_tr_selector),
>  	FIELD(GUEST_INTR_STATUS, guest_intr_status),
> +	FIELD(GUEST_PML_INDEX, guest_pml_index),
>  	FIELD(HOST_ES_SELECTOR, host_es_selector),
>  	FIELD(HOST_CS_SELECTOR, host_cs_selector),
>  	FIELD(HOST_SS_SELECTOR, host_ss_selector),
> @@ -767,6 +771,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
>  	FIELD64(XSS_EXIT_BITMAP, xss_exit_bitmap),
>  	FIELD64(GUEST_PHYSICAL_ADDRESS, guest_physical_address),
>  	FIELD64(VMCS_LINK_POINTER, vmcs_link_pointer),
> +	FIELD64(PML_ADDRESS, pml_address),
>  	FIELD64(GUEST_IA32_DEBUGCTL, guest_ia32_debugctl),
>  	FIELD64(GUEST_IA32_PAT, guest_ia32_pat),
>  	FIELD64(GUEST_IA32_EFER, guest_ia32_efer),
> @@ -1349,6 +1354,11 @@ static inline bool nested_cpu_has_xsaves(struct vmcs12 *vmcs12)
>  		vmx_xsaves_supported();
>  }
>
> +static inline bool nested_cpu_has_pml(struct vmcs12 *vmcs12)
> +{
> +	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML);
> +}
> +
>  static inline bool nested_cpu_has_virt_x2apic_mode(struct vmcs12 *vmcs12)
>  {
>  	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE);
> @@ -9368,13 +9378,20 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
>  		struct x86_exception *fault)
>  {
>  	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	u32 exit_reason;
> +	unsigned long exit_qualification = vcpu->arch.exit_qualification;
>
> -	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?


> +	} else if (fault->error_code & PFERR_RSVD_MASK)
>  		exit_reason = EXIT_REASON_EPT_MISCONFIG;
>  	else
>  		exit_reason = EXIT_REASON_EPT_VIOLATION;
> -	nested_vmx_vmexit(vcpu, exit_reason, 0, vcpu->arch.exit_qualification);
> +
> +	nested_vmx_vmexit(vcpu, exit_reason, 0, exit_qualification);
>  	vmcs12->guest_physical_address = fault->address;
>  }
>
> @@ -9717,6 +9734,22 @@ static int nested_vmx_check_msr_switch_controls(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>
> +static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
> +					 struct vmcs12 *vmcs12)
> +{
> +	u64 address = vmcs12->pml_address;
> +	int maxphyaddr = cpuid_maxphyaddr(vcpu);
> +
> +	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.

> +
> +	return 0;
> +}
> +
>  static int nested_vmx_msr_check_common(struct kvm_vcpu *vcpu,
>  				       struct vmx_msr_entry *e)
>  {
> @@ -10252,6 +10285,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
>  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>
> +	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
>  	if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
>  				vmx->nested.nested_vmx_procbased_ctls_low,
>  				vmx->nested.nested_vmx_procbased_ctls_high) ||
> @@ -11146,6 +11182,46 @@ static void vmx_flush_log_dirty(struct kvm *kvm)
>  	kvm_flush_pml_buffers(kvm);
>  }
>
> +static int vmx_write_pml_buffer(struct kvm_vcpu *vcpu)
> +{
> +	struct vmcs12 *vmcs12;
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	gpa_t gpa;
> +	struct page *page = NULL;
> +	u64 *pml_address;
> +
> +	if (is_guest_mode(vcpu)) {
> +		WARN_ON_ONCE(vmx->nested.pml_full);
> +
> +		/*
> +		 * 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 (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?

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

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

Thanks,
-Kai

> +		kunmap(page);
> +		nested_release_page_clean(page);
> +	}
> +
> +	return 0;
> +}
> +
>  static void vmx_enable_log_dirty_pt_masked(struct kvm *kvm,
>  					   struct kvm_memory_slot *memslot,
>  					   gfn_t offset, unsigned long mask)
> @@ -11505,6 +11581,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
>  	.slot_disable_log_dirty = vmx_slot_disable_log_dirty,
>  	.flush_log_dirty = vmx_flush_log_dirty,
>  	.enable_log_dirty_pt_masked = vmx_enable_log_dirty_pt_masked,
> +	.write_log_dirty = vmx_write_pml_buffer,
>
>  	.pre_block = vmx_pre_block,
>  	.post_block = vmx_post_block,
>