From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: RFC: New API for PPC for vcpu mmu access Date: Mon, 14 Feb 2011 11:11:53 -0600 Message-ID: <20110214111153.07f884b6@udp111988uds> References: <9F6FE96B71CF29479FF1CDC8046E15030BCD40@039-SN1MPN1-002.039d.mgd.msft.net> <20110202160821.5a223366@udp111988uds> <20110204163338.54690220@udp111988uds> <30BEE027-929B-43E5-A638-A58389F90B6F@suse.de> <20110207141547.58e49caa@udp111988uds> <220F22AA-31E5-4ACB-B0D5-557010096B91@suse.de> <20110209170928.6c629514@udp111988uds> <4D53CFE2.6080008@suse.de> <20110210125112.6d1f0380@udp111988uds> <8ACEDFEA-AA7F-400F-88F1-5F99864E8AAF@suse.de> <63E8AA2B-685F-4360-9BC8-E760A2CAD570@suse.de> <49812881-9E7C-4295-B708-CFA986EE9500@suse.de> <20110211145340.70c5812b@udp111988uds> <113B6114-C44E-4DD4-B318-4CAC826179DE@suse.de> <20110211185734.42f7f73f@udp111988uds> <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Cc: Yoder Stuart-B08248 , , "kvm@vger.kernel.org list" , "qemu-devel@nongnu.org List" To: Alexander Graf Return-path: Received: from ch1outboundpool.messaging.microsoft.com ([216.32.181.184]:54297 "EHLO ch1outboundpool.messaging.microsoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751515Ab1BNRMM (ORCPT ); Mon, 14 Feb 2011 12:12:12 -0500 In-Reply-To: <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, 13 Feb 2011 23:43:40 +0100 Alexander Graf wrote: > > struct kvmppc_book3e_tlb_entry { > > union { > > __u64 mas8_1; > > struct { > > __u32 mas8; > > __u32 mas1; > > }; > > }; > > __u64 mas2; > > union { > > __u64 mas7_3 > > struct { > > __u32 mas7; > > __u32 mas3; > > }; > > }; > > }; > > Looks good to me, except for the anonymous unions and structs of course. Avi dislikes those :). :-( > Is there any obvious reason we need to have unions in the first place? The > compiler should be clever enough to pick the right size accessors when > writing/reading masked __u64 entries, no? Yes, the intent was just to make it easier to access individual mas registers, and reuse existing bit declarations that are defined relative to individual registers. Why clutter up the source code when the compiler can deal with it? Same applies to the anonymous unions/structs -- it's done that way to present the fields in the most straightforward manner (equivalent to how the SPRs themselves are named and aliased). If it's a firm NACK on the existing structure, I think I'd rather just drop the paired versions altogether (but still order them this way so it can be changed back if there's any desire to in the future, without breaking compatibility). > The struct name should also have > a version indicator - it's the data descriptor only a single specific > mmu_type, right? It handles both KVM_MMU_PPC_BOOK3E_NOHV and KVM_MMU_PPC_BOOK3E_HV. > > For an MMU type of KVM_MMU_PPC_BOOK3E_NOHV, the mas8 in kvmppc_book3e_tlb_entry is > > present but not supported. > > > > struct kvmppc_book3e_tlb_params { > > /* > > * book3e defines 4 TLBs. Individual implementations may have > > * fewer. TLBs that do not already exist on the target must be > > * configured with a size of zero. A tlb_ways value of zero means > > * the array is fully associative. Only TLBs that are already > > * set-associative on the target may be configured with a different > > * associativity. A set-associative TLB may not exceed 255 ways. > > * > > * KVM will adjust TLBnCFG based on the sizes configured here, > > * though arrays greater than 2048 entries will have TLBnCFG[NENTRY] > > * set to zero. > > * > > * The size of any TLB that is set-associative must be a multiple of > > * the number of ways, and the number of sets must be a power of two. > > * > > * The page sizes supported by a TLB shall be determined by reading > > * the TLB configuration registers. This is not adjustable by userspace. > > * [Note: need sregs] > > */ > > __u32 tlb_sizes[4]; > > __u8 tlb_ways[4]; > > __u32 reserved[11]; > > }; > > > > KVM_CONFIG_TLB > > -------------- > > > > Capability: KVM_CAP_SW_TLB > > Type: vcpu ioctl > > Parameters: struct kvm_config_tlb (in) > > Returns: 0 on success > > -1 on error > > > > struct kvm_config_tlb { > > __u64 params; > > __u64 array; > > __u32 mmu_type; > > __u32 array_len; > > Some reserved bits please. IIRC Avi also really likes it when in addition to reserved fields there's also a "features" int that indicates which reserved fields are actually usable. Shouldn't hurt here either, right? params itself is a versioned struct, with reserved bits. Do we really need it here as well? > > - The hash for determining set number is: > > (MAS2[EPN] / page_size) & (num_sets - 1) > > where "page_size" is the smallest page size supported by the TLB, and > > "num_sets" is the tlb_sizes[] value divided by the tlb_ways[] value. > > If a book3e chip is made for which a different hash is needed, a new > > MMU type must be used, to ensure that userspace and KVM agree on layout. > > Please state the size explicitly then. It's 1k, right? It's 4K on Freescale chips. We should probably implement sregs first, in which case qemu can read the MMU config registers to find out the minimum supported page size. If we specify 4K here, we should probably just go ahead and stick FSL in the MMU type name. Specifying the hash itself already makes me nervous about claiming the more generic name. -Scott From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57204 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pp1yA-0001V5-Kp for qemu-devel@nongnu.org; Mon, 14 Feb 2011 12:12:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pp1y9-0001bG-76 for qemu-devel@nongnu.org; Mon, 14 Feb 2011 12:12:14 -0500 Received: from ch1outboundpool.messaging.microsoft.com ([216.32.181.186]:54320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pp1y9-0001b3-1g for qemu-devel@nongnu.org; Mon, 14 Feb 2011 12:12:13 -0500 Date: Mon, 14 Feb 2011 11:11:53 -0600 From: Scott Wood Message-ID: <20110214111153.07f884b6@udp111988uds> In-Reply-To: <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> References: <9F6FE96B71CF29479FF1CDC8046E15030BCD40@039-SN1MPN1-002.039d.mgd.msft.net> <20110202160821.5a223366@udp111988uds> <20110204163338.54690220@udp111988uds> <30BEE027-929B-43E5-A638-A58389F90B6F@suse.de> <20110207141547.58e49caa@udp111988uds> <220F22AA-31E5-4ACB-B0D5-557010096B91@suse.de> <20110209170928.6c629514@udp111988uds> <4D53CFE2.6080008@suse.de> <20110210125112.6d1f0380@udp111988uds> <8ACEDFEA-AA7F-400F-88F1-5F99864E8AAF@suse.de> <63E8AA2B-685F-4360-9BC8-E760A2CAD570@suse.de> <49812881-9E7C-4295-B708-CFA986EE9500@suse.de> <20110211145340.70c5812b@udp111988uds> <113B6114-C44E-4DD4-B318-4CAC826179DE@suse.de> <20110211185734.42f7f73f@udp111988uds> <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: RFC: New API for PPC for vcpu mmu access List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Yoder Stuart-B08248 , "kvm@vger.kernel.org list" , kvm-ppc@vger.kernel.org, "qemu-devel@nongnu.org List" On Sun, 13 Feb 2011 23:43:40 +0100 Alexander Graf wrote: > > struct kvmppc_book3e_tlb_entry { > > union { > > __u64 mas8_1; > > struct { > > __u32 mas8; > > __u32 mas1; > > }; > > }; > > __u64 mas2; > > union { > > __u64 mas7_3 > > struct { > > __u32 mas7; > > __u32 mas3; > > }; > > }; > > }; > > Looks good to me, except for the anonymous unions and structs of course. Avi dislikes those :). :-( > Is there any obvious reason we need to have unions in the first place? The > compiler should be clever enough to pick the right size accessors when > writing/reading masked __u64 entries, no? Yes, the intent was just to make it easier to access individual mas registers, and reuse existing bit declarations that are defined relative to individual registers. Why clutter up the source code when the compiler can deal with it? Same applies to the anonymous unions/structs -- it's done that way to present the fields in the most straightforward manner (equivalent to how the SPRs themselves are named and aliased). If it's a firm NACK on the existing structure, I think I'd rather just drop the paired versions altogether (but still order them this way so it can be changed back if there's any desire to in the future, without breaking compatibility). > The struct name should also have > a version indicator - it's the data descriptor only a single specific > mmu_type, right? It handles both KVM_MMU_PPC_BOOK3E_NOHV and KVM_MMU_PPC_BOOK3E_HV. > > For an MMU type of KVM_MMU_PPC_BOOK3E_NOHV, the mas8 in kvmppc_book3e_tlb_entry is > > present but not supported. > > > > struct kvmppc_book3e_tlb_params { > > /* > > * book3e defines 4 TLBs. Individual implementations may have > > * fewer. TLBs that do not already exist on the target must be > > * configured with a size of zero. A tlb_ways value of zero means > > * the array is fully associative. Only TLBs that are already > > * set-associative on the target may be configured with a different > > * associativity. A set-associative TLB may not exceed 255 ways. > > * > > * KVM will adjust TLBnCFG based on the sizes configured here, > > * though arrays greater than 2048 entries will have TLBnCFG[NENTRY] > > * set to zero. > > * > > * The size of any TLB that is set-associative must be a multiple of > > * the number of ways, and the number of sets must be a power of two. > > * > > * The page sizes supported by a TLB shall be determined by reading > > * the TLB configuration registers. This is not adjustable by userspace. > > * [Note: need sregs] > > */ > > __u32 tlb_sizes[4]; > > __u8 tlb_ways[4]; > > __u32 reserved[11]; > > }; > > > > KVM_CONFIG_TLB > > -------------- > > > > Capability: KVM_CAP_SW_TLB > > Type: vcpu ioctl > > Parameters: struct kvm_config_tlb (in) > > Returns: 0 on success > > -1 on error > > > > struct kvm_config_tlb { > > __u64 params; > > __u64 array; > > __u32 mmu_type; > > __u32 array_len; > > Some reserved bits please. IIRC Avi also really likes it when in addition to reserved fields there's also a "features" int that indicates which reserved fields are actually usable. Shouldn't hurt here either, right? params itself is a versioned struct, with reserved bits. Do we really need it here as well? > > - The hash for determining set number is: > > (MAS2[EPN] / page_size) & (num_sets - 1) > > where "page_size" is the smallest page size supported by the TLB, and > > "num_sets" is the tlb_sizes[] value divided by the tlb_ways[] value. > > If a book3e chip is made for which a different hash is needed, a new > > MMU type must be used, to ensure that userspace and KVM agree on layout. > > Please state the size explicitly then. It's 1k, right? It's 4K on Freescale chips. We should probably implement sregs first, in which case qemu can read the MMU config registers to find out the minimum supported page size. If we specify 4K here, we should probably just go ahead and stick FSL in the MMU type name. Specifying the hash itself already makes me nervous about claiming the more generic name. -Scott From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Mon, 14 Feb 2011 17:11:53 +0000 Subject: Re: RFC: New API for PPC for vcpu mmu access Message-Id: <20110214111153.07f884b6@udp111988uds> List-Id: References: <9F6FE96B71CF29479FF1CDC8046E15030BCD40@039-SN1MPN1-002.039d.mgd.msft.net> <20110202160821.5a223366@udp111988uds> <20110204163338.54690220@udp111988uds> <30BEE027-929B-43E5-A638-A58389F90B6F@suse.de> <20110207141547.58e49caa@udp111988uds> <220F22AA-31E5-4ACB-B0D5-557010096B91@suse.de> <20110209170928.6c629514@udp111988uds> <4D53CFE2.6080008@suse.de> <20110210125112.6d1f0380@udp111988uds> <8ACEDFEA-AA7F-400F-88F1-5F99864E8AAF@suse.de> <63E8AA2B-685F-4360-9BC8-E760A2CAD570@suse.de> <49812881-9E7C-4295-B708-CFA986EE9500@suse.de> <20110211145340.70c5812b@udp111988uds> <113B6114-C44E-4DD4-B318-4CAC826179DE@suse.de> <20110211185734.42f7f73f@udp111988uds> <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> In-Reply-To: <4DC23D10-A9C9-4C11-A344-A9779C370296@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexander Graf Cc: Yoder Stuart-B08248 , kvm-ppc@vger.kernel.org, "kvm@vger.kernel.org list" , "qemu-devel@nongnu.org List" On Sun, 13 Feb 2011 23:43:40 +0100 Alexander Graf wrote: > > struct kvmppc_book3e_tlb_entry { > > union { > > __u64 mas8_1; > > struct { > > __u32 mas8; > > __u32 mas1; > > }; > > }; > > __u64 mas2; > > union { > > __u64 mas7_3 > > struct { > > __u32 mas7; > > __u32 mas3; > > }; > > }; > > }; > > Looks good to me, except for the anonymous unions and structs of course. Avi dislikes those :). :-( > Is there any obvious reason we need to have unions in the first place? The > compiler should be clever enough to pick the right size accessors when > writing/reading masked __u64 entries, no? Yes, the intent was just to make it easier to access individual mas registers, and reuse existing bit declarations that are defined relative to individual registers. Why clutter up the source code when the compiler can deal with it? Same applies to the anonymous unions/structs -- it's done that way to present the fields in the most straightforward manner (equivalent to how the SPRs themselves are named and aliased). If it's a firm NACK on the existing structure, I think I'd rather just drop the paired versions altogether (but still order them this way so it can be changed back if there's any desire to in the future, without breaking compatibility). > The struct name should also have > a version indicator - it's the data descriptor only a single specific > mmu_type, right? It handles both KVM_MMU_PPC_BOOK3E_NOHV and KVM_MMU_PPC_BOOK3E_HV. > > For an MMU type of KVM_MMU_PPC_BOOK3E_NOHV, the mas8 in kvmppc_book3e_tlb_entry is > > present but not supported. > > > > struct kvmppc_book3e_tlb_params { > > /* > > * book3e defines 4 TLBs. Individual implementations may have > > * fewer. TLBs that do not already exist on the target must be > > * configured with a size of zero. A tlb_ways value of zero means > > * the array is fully associative. Only TLBs that are already > > * set-associative on the target may be configured with a different > > * associativity. A set-associative TLB may not exceed 255 ways. > > * > > * KVM will adjust TLBnCFG based on the sizes configured here, > > * though arrays greater than 2048 entries will have TLBnCFG[NENTRY] > > * set to zero. > > * > > * The size of any TLB that is set-associative must be a multiple of > > * the number of ways, and the number of sets must be a power of two. > > * > > * The page sizes supported by a TLB shall be determined by reading > > * the TLB configuration registers. This is not adjustable by userspace. > > * [Note: need sregs] > > */ > > __u32 tlb_sizes[4]; > > __u8 tlb_ways[4]; > > __u32 reserved[11]; > > }; > > > > KVM_CONFIG_TLB > > -------------- > > > > Capability: KVM_CAP_SW_TLB > > Type: vcpu ioctl > > Parameters: struct kvm_config_tlb (in) > > Returns: 0 on success > > -1 on error > > > > struct kvm_config_tlb { > > __u64 params; > > __u64 array; > > __u32 mmu_type; > > __u32 array_len; > > Some reserved bits please. IIRC Avi also really likes it when in addition to reserved fields there's also a "features" int that indicates which reserved fields are actually usable. Shouldn't hurt here either, right? params itself is a versioned struct, with reserved bits. Do we really need it here as well? > > - The hash for determining set number is: > > (MAS2[EPN] / page_size) & (num_sets - 1) > > where "page_size" is the smallest page size supported by the TLB, and > > "num_sets" is the tlb_sizes[] value divided by the tlb_ways[] value. > > If a book3e chip is made for which a different hash is needed, a new > > MMU type must be used, to ensure that userspace and KVM agree on layout. > > Please state the size explicitly then. It's 1k, right? It's 4K on Freescale chips. We should probably implement sregs first, in which case qemu can read the MMU config registers to find out the minimum supported page size. If we specify 4K here, we should probably just go ahead and stick FSL in the MMU type name. Specifying the hash itself already makes me nervous about claiming the more generic name. -Scott