Re: [kvm-ppc-devel] [PATCH] [3/4] Add userpace tlb access

[Christian E. <ehr...@li...>]

Hollis Blanchard wrote:
> On Wed, 2008-01-30 at 13:31 +0100, ehr...@li... wrote:
>> This version of the userspace tlb access has some changes out of discussions
>> with Zhang Wei responsible for the e500 work and Hollis for ppc440. It now
>> transports only a single tlb entry per get/set and uses an index specifier
>> that every architecture can interpret as needed e.g. e500 could map tlb0/tlb1
>> in the high bits and the array index in them in the low bits while ppc440 only
>> needs a linear index. The right vcpu is already selected since this is a vcpu
>> ioctl.
>> Internally the selction of the right tlb layout in kernel e.g. for set/get_tlb
>> or in userspace for kvm_dump_tlb is now based on the pvr that selects the
>> defines the guest type.
>>
>> Subject: [PATCH] [3/4] Add userpace tlb access
>> From: Christian Ehrhardt <ehr...@li...>
>>
>> This adds userspace tlb access transporting a single tlb entry to/from
>> userspace. It has an index variable passed with the call to specify the tlb
>> entry to get/set. For the guest tlb get&set is supported while the shadow
>> tlb can only be read from userspace.
> 
> Didn't we agree not to export the shadow TLB?

The last agreement I found was to export shadow TLB read only which is what I have here.
But it's easy to remove if needed ;-)
So do you think it is really completely useless e.g. on a tlb dump ? If yes we can remove it, but
if it can give us even a small benefit we should keep the read only access.
 
> ...
>> diff --git a/include/asm-powerpc/kvm.h b/include/asm-powerpc/kvm.h
>> --- a/include/asm-powerpc/kvm.h
>> +++ b/include/asm-powerpc/kvm.h
>> @@ -21,6 +21,7 @@
>>  #define __POWERPC_KVM_H__
>>
>>  #include <asm/types.h>
>> +#include <asm/mmu-44x.h>
>>
>>  struct kvm_regs {
>>  	__u32 pc;
>> @@ -53,6 +54,22 @@ struct kvm_fpu {
>>  struct kvm_fpu {
>>  };
>>
>> +struct tlbe {
>> +	__u32 tid; /* Only the low 8 bits are used. */
>> +	__u32 word0;
>> +	__u32 word1;
>> +	__u32 word2;
>> +};
> 
> This definitely must be renamed.

I'll add a renaming patch on top when I resend the patches next time.

>> +struct kvm_tlbe {
>> +	__u32 index;
>> +	union {
>> +		struct tlbe tlbe_ppc440;
>> +		/* Fix the size of the union (hopefully) */
>> +		char padding[32];
>> +	};
>> +};
> 
> Are you sure 32 bytes will be large enough? I think Freescale uses a ton
> of MAS registers for TLB insertion.

Well I thought it should be enough but we can use 64bit just to be safer.

-- 

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