DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A3F8D3D970
Date: Tue, 11 Apr 2017 20:15:15 -0400 (EDT)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Cc: James Hogan <james.hogan@imgtec.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Christoffer Dall <cdall@linaro.org>,
        Andrew Jones <drjones@redhat.com>, Marc Zyngier <marc.zyngier@arm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Cornelia Huck <cornelia.huck@de.ibm.com>,
        Paul Mackerras <paulus@ozlabs.org>
Message-ID: <1614033008.12916030.1491956115175.JavaMail.zimbra@redhat.com>
In-Reply-To: <20170411204536.GA20384@potion>
References: <20170406202056.18379-1-rkrcmar@redhat.com> <20170406202056.18379-2-rkrcmar@redhat.com> <20170406210215.GV31606@jhogan-linux.le.imgtec.org> <a6091fa0-8107-239a-ad89-34ca2cba4fa8@redhat.com> <20170411204536.GA20384@potion>
Subject: Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in
 kvm_make_all_cpus_request()
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Thread-Topic: fix guest_mode optimization in kvm_make_all_cpus_request()
Thread-Index: 5bYHZ106OgKat/9VvoOMG3S4f3l1DQ==
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2908
Lines: 76


----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "James Hogan" <james.hogan@imgtec.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Christoffer Dall"
> <cdall@linaro.org>, "Andrew Jones" <drjones@redhat.com>, "Marc Zyngier" <marc.zyngier@arm.com>, "Christian
> Borntraeger" <borntraeger@de.ibm.com>, "Cornelia Huck" <cornelia.huck@de.ibm.com>, "Paul Mackerras"
> <paulus@ozlabs.org>
> Sent: Wednesday, April 12, 2017 4:45:36 AM
> Subject: Re: [PATCH RFC 1/6] KVM: fix guest_mode optimization in kvm_make_all_cpus_request()
> 
> >  void kvm_reload_remote_mmus(struct kvm *kvm)
> >  {
> > -	kvm_make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD);
> > +	/* FIXME, is wait=true really needed?  */
> 
> Probably not.  There are two uses,
> 
> in kvm_mmu_prepare_zap_page():
>   The only change that happens between kvm_reload_remote_mmus() and
>   kvm_flush_remote_tlbs() in kvm_mmu_commit_zap_page() is setting of
>   sp->role.invalid -- synchronizing it doesn't prevent any race with
>   READING_SHADOW_PAGE_TABLES mode and the unconditional TLB flush is the
>   important one.  I think that kvm_reload_remote_mmus doesn't even need
>   to kick in this case.
> 
> in kvm_mmu_invalidate_zap_all_pages():
>   Same situation: the guest cannot do an entry without increasing the
>   generation number, but can enter READING_SHADOW_PAGE_TABLES mode
>   between reload and flush.
>   I think that we don't need to call
> 
> but my knowledge of this area is obviously lacking ...

Yes, you're right - I just was too lazy. :)

> > +	kvm_make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD, true);
> >  }
> >  
> >  int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
> > 
> > 
> > Other users do not need wait=false.
> 
> You mean "wait=true"?
> 
> (Would be safer to assume they depend on the VM exit wait until proved
>  otherwise ...)

Yeah, I audited them.

> > - bit 9 = kick after making request
> 
> Maybe add bit mask to denote in which modes the kick/wait is necessary?
> 
>   bit 9  : IN_GUEST_MODE
>   bit 10 : EXITING_GUEST_MODE
>   bit 11 : READING_SHADOW_PAGE_TABLES
> 
> TLB_FLUSH would set bits 8-11.  IIUC, ARM has use for requests that need
> to make sure that the guest is not in guest mode before proceeding and
> those would set bit 8-10.

No, checking vcpu->requests after setting IN_GUEST_MODE is done separately.
EXITING_GUEST_MODE's meaning *is* "no IPI needed".

> The common requests, "notice me as soon as possible", would set bit 9.
> The bits 9-11 could also be used only when bit 8 is set, to make the
> transition easier. (9 and 10 could be squished then as well.)

Maybe, depending on how the code looks like.  But considering we have
to do the cmpxchg, I think the should_kick and should_wait logic should
be embedded in kvm_make_all_cpus_request (and later on, kvm_make_request).

Paolo