RE: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

RE: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Nakajima, Jun]

Jeremy Fitzhardinge wrote:
> Anthony Liguori wrote:
> > Sure.  It adds a few more cycles onto native though (two memory
reads,
> > and some math).
> 
> As opposed to a serializing control-register read?  I think that's
> probably a win.
> 
>     J
> 

And actually you don't need the write to CR3 to flush TLB because the
one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is
updated at the same time?

Jun
---
Intel Open Source Technology Center

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Zachary Amsden]

Nakajima, Jun wrote:
> And actually you don't need the write to CR3 to flush TLB because the
> one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is
> updated at the same time?
>
> Jun

It should not be necessary, but I believe this was added as a workaround 
to a PII erratum.  I can't find the erratum, however, and the history of 
using G bits in Linux is complicated (several bugs introduced and many 
intermediate versions of this code).  Since this is not performance 
critical, I think it is probably best to leave the CR3 reload.

However, being unnecessary on modern processors, I already submitted a 
patch to eliminate it on 64-bit (or maybe just told Andi about it, I 
can't recall).

Zach

RE: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Nakajima, Jun]

Zachary Amsden wrote:
> Nakajima, Jun wrote:
> > And actually you don't need the write to CR3 to flush TLB because
the
> > one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is
updated
> > at the same time? 
> > 
> > Jun
> 
> It should not be necessary, but I believe this was added as a
workaround
> to a PII erratum.  I can't find the erratum, however, and the history
of
> using G bits in Linux is complicated (several bugs introduced and many
> intermediate versions of this code).  Since this is not performance
> critical, I think it is probably best to leave the CR3 reload.

I don't recommend this for old processors.

> 
> However, being unnecessary on modern processors, I already submitted a
> patch to eliminate it on 64-bit (or maybe just told Andi about it, I
> can't recall).
> 
> Zach

For KVM, it should be okay as well. But we can replace two CR4 accesses
with just one hypercall.

Jun
---
Intel Open Source Technology Center

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Anthony Liguori]

Nakajima, Jun wrote:
> Zachary Amsden wrote:
>   
>> Nakajima, Jun wrote:
>>     
>>> And actually you don't need the write to CR3 to flush TLB because
>>>       
> the
>   
>>> one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is
>>>       
> updated
>   
>>> at the same time? 
>>>
>>> Jun
>>>       
>> It should not be necessary, but I believe this was added as a
>>     
> workaround
>   
>> to a PII erratum.  I can't find the erratum, however, and the history
>>     
> of
>   
>> using G bits in Linux is complicated (several bugs introduced and many
>> intermediate versions of this code).  Since this is not performance
>> critical, I think it is probably best to leave the CR3 reload.
>>     
>
> I don't recommend this for old processors.
>
>   
>> However, being unnecessary on modern processors, I already submitted a
>> patch to eliminate it on 64-bit (or maybe just told Andi about it, I
>> can't recall).
>>
>> Zach
>>     
>
> For KVM, it should be okay as well. But we can replace two CR4 accesses
> with just one hypercall.
>   

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a 
control register.  The thought here is that it would be useful not just 
for the global bit but also for CR0.TS although we would need another 
paravirt_op hook for stts.

Regards,

Anthony Liguori

> Jun
> ---
> Intel Open Source Technology Center
>   

RE: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Nakajima, Jun]

Anthony Liguori wrote:
> Nakajima, Jun wrote:

<snip>

> > For KVM, it should be okay as well. But we can replace two CR4
accesses
> > with just one hypercall. 
> > 
> 
> I was thinking the same thing :-)
> 
> I was actually thinking about adding a hypercall to set/clear a bit in
a
> control register.  The thought here is that it would be useful not
just
> for the global bit but also for CR0.TS although we would need another
> paravirt_op hook for stts.

Given the optimizations for CPU virtualization in the current H/W, I'm
not sure if such hooks are useful. Do you have any performance data that
justify such hooks? 

> 
> Regards,
> 
> Anthony Liguori
> 


Jun
---
Intel Open Source Technology Center

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Zachary Amsden]

Anthony Liguori wrote:
>
> I was thinking the same thing :-)
>
> I was actually thinking about adding a hypercall to set/clear a bit in 
> a control register.  The thought here is that it would be useful not 
> just for the global bit but also for CR0.TS although we would need 
> another paravirt_op hook for stts.

You don't need STTS: just cache CR0 value on writes and replace 
read_cr0.  More paravirt_op hooks would likely be frowned upon, we've 
already got too many.

Zach

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Anthony Liguori]

Nakajima, Jun wrote:
> Anthony Liguori wrote:
>   
>
> Given the optimizations for CPU virtualization in the current H/W, I'm
> not sure if such hooks are useful. Do you have any performance data that
> justify such hooks? 
>   

No, I don't.  It was just a thought that was yet to be confirmed.

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>>     
>
>
> Jun
> ---
> Intel Open Source Technology Center
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Rusty Russell]

On Wed, 2007-05-30 at 14:22 -0500, Anthony Liguori wrote:
> I was actually thinking about adding a hypercall to set/clear a bit in a 
> control register.  The thought here is that it would be useful not just 
> for the global bit but also for CR0.TS although we would need another 
> paravirt_op hook for stts.

We don't really need one, because Linux (i386) only cares about the TS
bit of cr0.  From lguest (you'd want this per-cpu of course):

        static unsigned long current_cr0, current_cr3;
        static void lguest_write_cr0(unsigned long val)
        {
        	lazy_hcall(LHCALL_TS, val & 8, 0, 0);
        	current_cr0 = val;
        }
        
        static unsigned long lguest_read_cr0(void)
        {
        	return current_cr0;
        }

Cheers,
Rusty.

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Avi Kivity]

Anthony Liguori wrote:
>>>     
>>>       
>> For KVM, it should be okay as well. But we can replace two CR4 accesses
>> with just one hypercall.
>>   
>>     
>
> I was thinking the same thing :-)
>
> I was actually thinking about adding a hypercall to set/clear a bit in a 
> control register.  The thought here is that it would be useful not just 
> for the global bit but also for CR0.TS although we would need another 
> paravirt_op hook for stts.
>   

Are global tlb flushes frequent enough to warrant optimization?

-- 
error compiling committee.c: too many arguments to function

Re: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>   
>>>>     
>>>>       
>>>>         
>>> For KVM, it should be okay as well. But we can replace two CR4 accesses
>>> with just one hypercall.
>>>   
>>>     
>>>       
>> I was thinking the same thing :-)
>>
>> I was actually thinking about adding a hypercall to set/clear a bit in a 
>> control register.  The thought here is that it would be useful not just 
>> for the global bit but also for CR0.TS although we would need another 
>> paravirt_op hook for stts.
>>   
>>     
>
> Are global tlb flushes frequent enough to warrant optimization?
>   

I don't think so, so I'm going to remove this from the series.  I'm 
adding another an MMU batching patch which should be enough measurable 
performance advantage to warrant the series.

Regards,

Anthony Liguori