Hi Paolo,

On Tue, Apr 11, 2017 at 01:25:04PM +0800, Paolo Bonzini wrote:
> On 07/04/2017 05:02, James Hogan wrote:
> > This presumably changes the behaviour on x86, from != OUTSIDE_GUEST_MODE
> > to == IN_GUEST_MODE. so:
> > - you'll no longer get IPIs if its in READING_SHADOW_PAGE_TABLES (which
> > MIPS also now uses when accessing mappings outside of guest mode and
> > depends upon to wait until the old mappings are no longer in use).
>
> This is wrong, the purpose of READING_SHADOW_PAGE_TABLES is "kvm_flush_remote_tlbs
> should send me an IPI, because I want to stop kvm_flush_remote_tlbs until I'm done
> reading the page tables".

That sounds equivalent to what I meant for MIPS, i.e.
kvm_flush_remote_tlbs() does the waiting (not the thing accessing guest
mappings).

Cheers
James

Attachments:

(No filename) (799.00 B)
signature.asc (801.00 B)
Digital signature Download all attachments

2017-04-11 10:44:44

----- Original Message -----
> From: "Radim Krčmář" <[email protected]>
> To: "Paolo Bonzini" <[email protected]>
> Cc: "James Hogan" <[email protected]>, [email protected], [email protected], "Christoffer Dall"
> <[email protected]>, "Andrew Jones" <[email protected]>, "Marc Zyngier" <[email protected]>, "Christian
> Borntraeger" <[email protected]>, "Cornelia Huck" <[email protected]>, "Paul Mackerras"
> <[email protected]>
> 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