Re: RFC: New API for PPC for vcpu mmu access

[Scott Wood <scottwood@freescale.com>]

On Wed, 9 Feb 2011 18:21:40 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 07.02.2011, at 21:15, Scott Wood wrote:
> 
> > That's pretty much what the proposed API does -- except it uses a void
> > pointer instead of uint64_t *.
> 
> Oh? Did I miss something there? The proposal looked as if it only transfers a single TLB entry at a time.

Right, I just meant in terms of avoiding a fixed reference to a hw-specific
type.

> > How about:
> > 
> > struct kvmppc_booke_tlb_entry {
> > 	union {
> > 		__u64 mas0_1;
> > 		struct {
> > 			__u32 mas0;
> > 			__u32 mas1;
> > 		};
> > 	};
> > 	__u64 mas2;
> > 	union {
> > 		__u64 mas7_3	
> > 		struct {
> > 			__u32 mas7;
> > 			__u32 mas3;
> > 		};
> > 	};
> > 	__u32 mas8;
> > 	__u32 pad;
> 
> Would it make sense to add some reserved fields or would we just bump up the mmu id?

I was thinking we'd just bump the ID.  I only stuck "pad" in there for
alignment.  And we're making a large array of it, so padding could hurt.

> > struct kvmppc_booke_tlb_params {
> > 	/*
> > 	 * book3e defines 4 TLBs.  Individual implementations may have
> > 	 * fewer.  TLBs that do not exist on the target must be configured
> > 	 * with a size of zero.  KVM will adjust TLBnCFG based on the sizes
> > 	 * configured here, though arrays greater than 2048 entries will
> > 	 * have TLBnCFG[NENTRY] set to zero.
> > 	 */
> > 	__u32 tlb_sizes[4];
> 
> Add some reserved fields?

MMU type ID also controls this, but could add some padding to make
extensions simpler (esp. since we're not making an array of it).  How much
would you recommend?

> > struct kvmppc_booke_tlb_search {
> 
> Search? I thought we agreed on having a search later, after the full get/set is settled?

We agreed on having a full array-like get/set... my preference was to keep
it all under one capability, which implies adding it at the same time.
But if we do KVM_TRANSLATE, we can probably drop KVM_SEARCH_TLB.  I'm
skeptical that array-only will not be a performance issue under any usage
pattern, but we can implement it and try it out before finalizing any of
this.

> > 	struct kvmppc_booke_tlb_entry entry;
> > 	union {
> > 		__u64 mas5_6;
> > 		struct {
> > 			__u64 mas5;
> > 			__u64 mas6;
> > 		};
> > 	};
> > };

The fields inside the struct should be __u32, of course. :-P

> > - An entry with MAS1[V] = 0 terminates the list early (but there will
> >   be no terminating entry if the full array is valid).  On a call to
> >   KVM_GET_TLB, the contents of elemnts after the terminator are undefined.
> >   On a call to KVM_SET_TLB, excess elements beyond the terminating
> >   entry may not be accessed by KVM.
> 
> Very implementation specific, but ok with me. 

I assumed most MMU types would have some straightforward way of marking an
entry invalid (if not, it can add a software field in the struct), and that
it would be MMU-specific code that is processing the list.

> It's constrained to the BOOKE implementation of that GET/SET anyway. Is
> this how the hardware works too?

Hardware doesn't process lists of entries.  But MAS1[V] is the valid
bit in hardware.

> > [Note: Once we implement sregs, Qemu can determine which TLBs are
> > implemented by reading MMUCFG/TLBnCFG -- but in no case should a TLB be
> > unsupported by KVM if its existence is implied by the target CPU]
> > 
> > KVM_SET_TLB
> > -----------
> > 
> > Capability: KVM_CAP_SW_TLB
> > Type: vcpu ioctl
> > Parameters: struct kvm_set_tlb (in)
> > Returns: 0 on success
> >         -1 on error
> > 
> > struct kvm_set_tlb {
> > 	__u64 params;
> > 	__u64 array;
> > 	__u32 mmu_type;
> > };
> > 
> > [Note: I used __u64 rather than void * to avoid the need for special
> > compat handling with 32-bit userspace on a 64-bit kernel -- if the other
> > way is preferred, that's fine with me]
> 
> Oh, now I understand what you were proposing :). Sorry. No, this way is sane.

What about the ioctls that take only a pointer?  The actual calling
mechanism should work without compat, but in order for _IOR and such to not
assign a different IOCTL number based on the size of void *, we'd need to
lie and use plain _IO().  It looks like some ioctls such as
KVM_SET_TSS_ADDR already do this.

If we drop KVM_SEARCH_TLB and struct-ize KVM_GET_TLB to fit in a buffer
size parameter, it's moot though.

> > KVM_GET_TLB
> > -----------
> > 
> > Capability: KVM_CAP_SW_TLB
> > Type: vcpu ioctl
> > Parameters: void pointer (out)
> > Returns: 0 on success
> >         -1 on error
> > 
> > Reads the TLB array from a virtual CPU.  A successful call to
> > KVM_SET_TLB must have been previously made on this vcpu.  The argument
> > must point to space for an array of the size and type of TLB entry
> > structs configured by the most recent successful call to KVM_SET_TLB.
> > 
> > For mmu types BOOKE_NOHV and BOOKE_HV, the array is of type "struct
> > kvmppc_booke_tlb_entry", and must hold a number of entries equal to
> > the sum of the elements of tlb_sizes in the most recent successful
> > TLB configuration call.
> 
> We should add some sort of safety net here. The caller really should pass in how big that pointer is.

The caller must have previously called KVM_SET_TLB (or KVM_GET_TLB will
return an error), so it implicitly told KVM how much data to send, based
on MMU type and params.

I'm OK with an explicit buffer size for added safety, though.

-Scott