PowerPC patches for 2.6.27

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hollis Blanchard
Sent: Saturday, July 26, 2008 2:55 AM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 13 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hollis Blanchard
Sent: Saturday, July 26, 2008 2:55 AM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

(quoted diff) +0 -0

@@ -229,6 +235,7 @@ void kvmppc_mmu_priv_switch(struct kvm_v
 

arch/powerpc/kvm/44x_tlb.c void kvmppc_mmu_invalidate(struct kvm_vc · line 220

Quoted 7 lines

arch/powerpc/kvm/44x_tlb.c +1 -0

 		kvmppc_44x_shadow_release(vcpu, i);
 		stlbe->word0 = 0;
+		kvmppc_tlbe_set_modified(vcpu, i);
 		KVMTRACE_5D(STLB_INVAL, vcpu, i,
 				stlbe->tid, stlbe->word0, stlbe->word1,
 				stlbe->word2, handler);

Can this place use "_tlbia()" instead?

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Monday, August 11, 2008 4:33 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 5 lines · Liu Yu

Quoted 5 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hollis

Blanchard

Quoted 4 lines

Quoted 4 lines

Sent: Saturday, July 26, 2008 2:55 AM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 4 of 5] kvm: ppc: Write only modified

shadow entries

Quoted 4 lines

Quoted 4 lines

into theTLB on exit

(quoted diff) +0 -0

@@ -229,6 +235,7 @@ void kvmppc_mmu_priv_switch(struct kvm_v
 

Quoted 10 lines

arch/powerpc/kvm/44x_tlb.c void kvmppc_mmu_invalidate(struct kvm_vc · line 220

Quoted 7 lines

arch/powerpc/kvm/44x_tlb.c +1 -0

 		kvmppc_44x_shadow_release(vcpu, i);
 		stlbe->word0 = 0;
+		kvmppc_tlbe_set_modified(vcpu, i);
 		KVMTRACE_5D(STLB_INVAL, vcpu, i,
 				stlbe->tid, stlbe->word0, stlbe->word1,
 				stlbe->word2, handler);

Can this place use "_tlbia()" instead?

Hi Liu,
when we exit&reenter a guest on the lightweight path this patch saves us from rebuilding too much of the tlb. It's just the point of the patch to not invalidate all tlb entries on every exit, but keeping as much as possible. Additionally I'm not even sure if tlbia is available on all cores used.

Am I overlooking something ? - what do you think would be the benefit of using _tlbia() here?
Do you mean the loop to clear up to the hwater mark where this patch makes changes and not this patch itself?

Quoted 48 lines

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Monday, August 11, 2008 4:33 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 6 lines · Liu Yu

Quoted 6 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hollis

Blanchard

Quoted 5 lines

Quoted 5 lines

Sent: Saturday, July 26, 2008 2:55 AM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 4 of 5] kvm: ppc: Write only modified

shadow entries

Quoted 4 lines

Quoted 4 lines

into theTLB on exit

(quoted diff) +0 -0

@@ -229,6 +235,7 @@ void kvmppc_mmu_priv_switch(struct kvm_v
 

Quoted 11 lines

arch/powerpc/kvm/44x_tlb.c void kvmppc_mmu_invalidate(struct kvm_vc · line 220

Quoted 8 lines

arch/powerpc/kvm/44x_tlb.c +1 -0

 		kvmppc_44x_shadow_release(vcpu, i);
 		stlbe->word0 = 0;
+		kvmppc_tlbe_set_modified(vcpu, i);
 		KVMTRACE_5D(STLB_INVAL, vcpu, i,
 				stlbe->tid, stlbe->word0, stlbe->word1,
 				stlbe->word2, handler);

Can this place use "_tlbia()" instead?

Hi Liu,
when we exit&reenter a guest on the lightweight path this patch saves us from rebuilding too much of the tlb. It's just the point of the patch to not invalidate all tlb entries on every exit, but keeping as much as possible. Additionally I'm not even sure if tlbia is available on all cores used.

Quoted 6 lines

Am I overlooking something ? - what do you think would be the benefit of using _tlbia() here?
Do you mean the loop to clear up to the hwater mark where this patch makes changes and not this patch itself?

Yes.

When you set nearly all tlb entries modified in this loop,
you have to write all of them back before entering the guest in booke_interrupts.S.

So, why not "_tlbia()" here outside this loop, and don't set them modified.
Then, you needn't write them back.
In fact, the shadow tlb entries are still consistent with the hardware, as all of them are invalidated.

Am I overlooking something ?

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Monday, August 11, 2008 7:48 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 2 lines

When you set nearly all tlb entries modified in this loop,

you have to

Quoted 1 line

write all of them back before entering the guest in

booke_interrupts.S.

Quoted 2 lines

So, why not "_tlbia()" here outside this loop, and don't

set them modified.

Quoted 2 lines

Then, you needn't write them back.
In fact, the shadow tlb entries are still consistent with

the hardware, as all of them are invalidated.

Quoted 3 lines

Am I overlooking something ?

As Hollis wrote in his patch comment usually just one tlb entry is modified which saves a lot of updates. If we invalidate all we later on would have to satisfy the misses we cause by doing so.

With some free time we could test _tlbia() here, but I expect it to be slower by causing the need for tlb re-population compared to what we save here.

Quoted 25 lines

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Monday, August 11, 2008 7:48 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 2 lines

When you set nearly all tlb entries modified in this loop,

you have to

Quoted 2 lines

write all of them back before entering the guest in

booke_interrupts.S.

Quoted 2 lines

So, why not "_tlbia()" here outside this loop, and don't

set them modified.

Quoted 3 lines

Then, you needn't write them back.
In fact, the shadow tlb entries are still consistent with

the hardware, as all of them are invalidated.

Quoted 8 lines

Quoted 3 lines

Am I overlooking something ?

As Hollis wrote in his patch comment usually just one tlb entry is modified which saves a lot of updates. If we invalidate all we later on would have to satisfy the misses we cause by doing so.

In this loop, aren't you invalidating them all?

Quoted 5 lines

With some free time we could test _tlbia() here, but I expect it to be slower by causing the need for tlb re-population compared to what we save here.

I don't understand why it will be slower.
I think _tlbia will take the same effect as what you do here. That is to say, it will always cause the need for tlb re-population. And what's more, _tlbia will save some time when it enter the guest.

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Tuesday, August 12, 2008 4:04 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 23 lines · Liu Yu

Quoted 23 lines

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Monday, August 11, 2008 7:48 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]
Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into theTLB on exit

Quoted 2 lines

When you set nearly all tlb entries modified in this loop,

you have to

Quoted 2 lines

write all of them back before entering the guest in

booke_interrupts.S.

Quoted 2 lines

So, why not "_tlbia()" here outside this loop, and don't

set them modified.

Quoted 3 lines

Then, you needn't write them back.
In fact, the shadow tlb entries are still consistent with

Quoted 8 lines

Quoted 8 lines

the hardware, as all of them are invalidated.

Quoted 3 lines

Am I overlooking something ?

As Hollis wrote in his patch comment usually just one tlb entry is modified which saves a lot of updates.
If we invalidate all we later on would have to satisfy the

misses we

Quoted 7 lines

Quoted 2 lines

cause by doing so.

In this loop, aren't you invalidating them all?

Quoted 1 line

With some free time we could test _tlbia() here, but I

expect it to

Quoted 1 line

Quoted 1 line

be slower by causing the need for tlb re-population

compared to what

Quoted 10 lines

Quoted 3 lines

we save here.

I don't understand why it will be slower.
I think _tlbia will take the same effect as what you do here. That is to say, it will always cause the need for tlb re-population. And what's more, _tlbia will save some time when it enter the guest.

On 440 which this file 440x_tlb.c is for, the implementation of tlbia is more or less the same than Hollis code when reentering the guest (there is no tlbia instruction on 440) with the addition of a check of the shadow modified flag.

I agree with you that the code in the kvmppc_mmu_priv_switch effectively releases all tlbe's to the high water mark, marks them modified and later on pushes them (being invalid) when entering the guest.
That means that the part of revoking all tlbe's to the high water mark can be done with tlbia() instead of using the kvmppc_tlbe_set_modified call for all indexes here.

I also agree that this can save us memory accesses from 0 to tlb_44x_hwater for the setting of the shadow modified flag, but everything else will be the same. Don't get me wrong I usually embrace any performance improvement, but when you look in the patched source file the is a comment just above the patched section:

   /* Future optimization: clear only userspace mappings. */ 

Once we do that (or anything similar if we get other ideas here) keeping the guest kernel mapping we can't use _tlbia() anymore because it clears everyting. That as an example would save us a guest exit which should save more time than some memory accesses.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Hollis Blanchard
Sent: Saturday, July 26, 2008 2:55 AM
To: avi-atKUWr5tajBWk0Htik3J/[email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH 5 of 5] kvm: powerpc: Map guest userspace
with TID=0 mappings

# HG changeset patch
# User Hollis Blanchard <hollisb-r/[email protected]> # Date 1217011852 18000 # Node ID 080b9c9515a5593babc8cac73687b053d7d3f124 # Parent c832dfc7a6b50ace3c37d00b16ef9f5b46284b3e
kvm: powerpc: Map guest userspace with TID=0 mappings.

When we use TID=N userspace mappings, we must ensure that kernel mappings have been destroyed when entering userspace. Using TID=1/TID=0 for kernel/user mappings and running userspace with PID=0 means that userspace can't access the kernel mappings, but the kernel can directly access userspace.

The net is that we don't need to flush the TLB on privilege switches, but we do on guest context switches (which are far more infrequent). Guest boot time performance improvement: about 30%.

Signed-off-by Hollis Blanchard <hollisb-r/[email protected]>

---

7 files changed, 40 insertions(+), 18 deletions(-)
arch/powerpc/kernel/asm-offsets.c   |    2 -
arch/powerpc/kvm/44x_tlb.c          |   37 
++++++++++++++++++++---------------
arch/powerpc/kvm/booke_guest.c      |    2 +
arch/powerpc/kvm/booke_interrupts.S |    2 -
arch/powerpc/kvm/emulate.c          |    2 -
include/asm-powerpc/kvm_host.h      |    4 +++
include/asm-powerpc/kvm_ppc.h       |    9 ++++++++

I have a question that I could not think through.
While multiple qemu/kvm processes are running at the same time, how to prevent one guest from using others' TLB? For all the guests have the same TID=0 for userspace and TID=1 for kernel.

Quoted 5 lines

The net is that we don't need to flush the TLB on privilege switches, but we do on guest context switches (which are far more infrequent). Guest boot time performance improvement: about 30%.

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Tuesday, July 29, 2008 3:03 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 5 of 5] kvm: powerpc: Map guest userspace with TID=0 mappings

Quoted 2 lines · On Monday 28 July 2008 12:33:41 Liu Yu wrote:

I have a question that I could not think through. While multiple qemu/kvm processes are running at the same

time, how to

Quoted 1 line

prevent one guest from using others' TLB? For all the

guests have the

Quoted 1 line

same TID=0 for userspace and TID=1 for kernel.

[...]

Hi Yu Liu, thats a good question.
Afaik thats solved by the fact that the shadow tlb which is used when entering guest context is per vcpu. Therefor a guest has always it's own shadow tlb active and no mappings to the content of other guests.

This patch just allows us that a single guest userspace process accessing the kernel 20 times (and changing privilege level 20 times by doing so) can run without tlb flushes. Guest-userspace context switch (pid is changing) -> tlb flush; and guest switches (guest A -> guest B) -> other shadow tlb active; should still be working fine.

Quoted 4 lines

Quoted 4 lines

The net is that we don't need to flush the TLB on privilege switches, but we do on guest context switches (which are far more infrequent). Guest boot time performance

improvement: about 30%.

Quoted 1 line

Quoted 1 line

Signature · 3 lines

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Liu Yu
Sent: Tuesday, July 29, 2008 3:48 PM
To: Christian Ehrhardt
Cc: Hollis Blanchard; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH 5 of 5] kvm: powerpc: Map guest userspace with TID=0 mappings

Quoted 12 lines

-----Original Message-----
From: Christian Ehrhardt [mailto:[email protected]]
Sent: Tuesday, July 29, 2008 3:03 PM
To: Liu Yu
Cc: Hollis Blanchard; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 5 of 5] kvm: powerpc: Map guest userspace with TID=0 mappings

Quoted 2 lines · On Monday 28 July 2008 12:33:41 Liu Yu wrote:

I have a question that I could not think through. While multiple qemu/kvm processes are running at the same

Quoted 8 lines

time, how to

Quoted 1 line

prevent one guest from using others' TLB? For all the

guests have the

Quoted 1 line

same TID=0 for userspace and TID=1 for kernel.

[...]

Hi Yu Liu, thats a good question.
Afaik thats solved by the fact that the shadow tlb which is

used when

Quoted 1 line

entering guest context is per vcpu. Therefor a guest has

always it's

Quoted 1 line

own shadow tlb active and no mappings to the content of

other guests.

Yes, shadow tlb is per vcpu.
But in the patch 4/5, before entering guest context, not all shadow tlb will be written back.
So if (guest A -> guest B) happen, after entering guest B, is there any possibility that A's tlb is still existing in hardware?

Quoted 5 lines

This patch just allows us that a single guest userspace process accessing the kernel 20 times (and changing privilege level

20 times

Quoted 3 lines

by doing so) can run without tlb flushes. Guest-userspace context switch (pid is changing) -> tlb flush; and guest switches (guest A -> guest B) -> other shadow tlb

active; should

Quoted 5 lines

still be working fine.

Quoted 3 lines

Quoted 3 lines

The net is that we don't need to flush the TLB on privilege switches, but we do on guest context switches (which

are far more

Quoted 10 lines

Quoted 1 line

Quoted 1 line

infrequent). Guest boot time performance

improvement: about 30%.

Quoted 1 line

Quoted 1 line

--

Grüsse / regards,
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

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

Hi Avi, can these patches go upstream for 2.6.27? There's a bug fix, the addition of hardware breakpoint functionality, and three very significant performance improvements.

By the way, I will be on vacation for a few weeks starting Monday, but Christian Ehrhardt should be able to take care of any technical issues.

Quoted 6 lines · Hollis Blanchard

Hi Avi, can these patches go upstream for 2.6.27? There's a bug fix, the addition of hardware breakpoint functionality, and three very significant performance improvements.

By the way, I will be on vacation for a few weeks starting Monday, but Christian Ehrhardt should be able to take care of any technical issues.

Applied all; thanks. I prefer to only merge bug fixes at this time for 2.6.27. As far as I can tell, patch 2 is independent of the rest so I'll queue that. Let me know if that works.

Quoted 4 lines

Applied all; thanks. I prefer to only merge bug fixes at this time for 2.6.27. As far as I can tell, patch 2 is independent of the rest so I'll queue that. Let me know if that works.

Yes these patches are independent - for the type question you can consider 1/5 - feature
2/5 - bug fix
3,4,5/5 - yeah what is it .. neither a pure bug fix nor a feature let's call it "performance fix"

We tested most of those performance fixes at least 1.5 Months now, having them applied while developing new things on top and I feel very good with 3&4. It would be nice if at least the performance fixes 3&4 could go in now under the bug fix label too, but it's up to you.