Re: [PATCH v4 4/6] kvm, mem-hotplug: Reload L1' apic access page on migration in vcpu_enter_guest().

[tangchen <tangchen@xxxxxxxxxxxxxx>]

Hi Gleb,

On 09/03/2014 12:00 AM, Gleb Natapov wrote:
> ......
> +static void vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * apic access page could be migrated. When the page is being migrated,
> +	 * GUP will wait till the migrate entry is replaced with the new pte
> +	 * entry pointing to the new page.
> +	 */
> +	vcpu->kvm->arch.apic_access_page = gfn_to_page(vcpu->kvm,
> +				APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
> +	kvm_x86_ops->set_apic_access_page_addr(vcpu->kvm,
> +				page_to_phys(vcpu->kvm->arch.apic_access_page));
> I am a little bit worried that here all vcpus write to vcpu->kvm->arch.apic_access_page
> without any locking. It is probably benign since pointer write is atomic on x86. Paolo?
>
> Do we even need apic_access_page? Why not call
>   gfn_to_page(APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT)
>   put_page()
> on rare occasions we need to know its address?

Isn't it a necessary item defined in hardware spec ?

I didn't read intel spec deeply, but according to the code, the page's 
address is
written into vmcs. And it made me think that we cannnot remove it.

Thanks.

> > +}
> > +
> >   /*
> >    * Returns 1 to let __vcpu_run() continue the guest execution loop without
> >    * exiting to the userspace.  Otherwise, the value will be returned to the
> > @@ -6049,6 +6062,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >   			kvm_deliver_pmi(vcpu);
> >   		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
> >   			vcpu_scan_ioapic(vcpu);
> > +		if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu))
> > +			vcpu_reload_apic_access_page(vcpu);
> >   	}
> >   
> >   	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index a4c33b3..8be076a 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -136,6 +136,7 @@ static inline bool is_error_page(struct page *page)
> >   #define KVM_REQ_GLOBAL_CLOCK_UPDATE 22
> >   #define KVM_REQ_ENABLE_IBS        23
> >   #define KVM_REQ_DISABLE_IBS       24
> > +#define KVM_REQ_APIC_PAGE_RELOAD  25
> >   
> >   #define KVM_USERSPACE_IRQ_SOURCE_ID		0
> >   #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
> > @@ -579,6 +580,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm);
> >   void kvm_reload_remote_mmus(struct kvm *kvm);
> >   void kvm_make_mclock_inprogress_request(struct kvm *kvm);
> >   void kvm_make_scan_ioapic_request(struct kvm *kvm);
> > +void kvm_reload_apic_access_page(struct kvm *kvm);
> >   
> >   long kvm_arch_dev_ioctl(struct file *filp,
> >   			unsigned int ioctl, unsigned long arg);
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 33712fb..d8280de 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -210,6 +210,11 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
> >   	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
> >   }
> >   
> > +void kvm_reload_apic_access_page(struct kvm *kvm)
> > +{
> > +	make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
> > +}
> > +
> >   int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
> >   {
> >   	struct page *page;
> > @@ -294,6 +299,13 @@ static void kvm_mmu_notifier_invalidate_page(struct mmu_notifier *mn,
> >   	if (need_tlb_flush)
> >   		kvm_flush_remote_tlbs(kvm);
> >   
> > +	/*
> > +	 * The physical address of apic access page is stroed in VMCS.
> > +	 * So need to update it when it becomes invalid.
> > +	 */
> > +	if (address == gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT))
> > +		kvm_reload_apic_access_page(kvm);
> > +
> >   	spin_unlock(&kvm->mmu_lock);
> >   	srcu_read_unlock(&kvm->srcu, idx);
> >   }
> > --
> > 1.8.3.1
> --
> 			Gleb.
> .

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html