From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH 3/3] KVM: Expose MSRs to userspace Date: Thu, 16 Nov 2006 20:08:48 +0100 Message-ID: <200611162008.48931.arnd@arndb.de> References: <455CA70C.9060307@qumranet.com> <20061116180422.0CC9325015E@cleopatra.q> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: akpm-3NddpPZAyC0@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, uril-atKUWr5tajBWk0Htik3J/w@public.gmane.org Return-path: To: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org In-Reply-To: <20061116180422.0CC9325015E@cleopatra.q> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org On Thursday 16 November 2006 19:04, Avi Kivity wrote: > +struct kvm_msr_entry { > +=A0=A0=A0=A0=A0=A0=A0__u32 index; > +=A0=A0=A0=A0=A0=A0=A0__u32 reserved; > +=A0=A0=A0=A0=A0=A0=A0__u64 data; > +}; > + > +/* for KVM_GET_MSRS and KVM_SET_MSRS */ > +struct kvm_msrs { > +=A0=A0=A0=A0=A0=A0=A0__u32 vcpu; > +=A0=A0=A0=A0=A0=A0=A0__u32 nmsrs; /* number of msrs in entries */ > + > +=A0=A0=A0=A0=A0=A0=A0union { > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0struct kvm_msr_entry __user= *entries; > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0__u64 padding; > +=A0=A0=A0=A0=A0=A0=A0}; > +}; ioctl interfaces with pointers in them are generally a bad idea, though you handle most of the points against them fine here (endianess doesn't matter, padding is correct). Still, it might be better not to set a bad example. Is accessing the MSRs actually performance critical? If not, you could define the ioctl to take only a single entry argument. A possible alternative could also be to have a variable length argument like below, but that creates other problems: +struct kvm_msrs { + __u32 vcpu; + __u32 nmsrs; /* number of msrs in entries */ + struct kvm_msr_entry entries[0]; /* followed by actual msrs */ +}; This would mean that you can't tell the transfer size from the ioctl number, but you can't do that in your code either, because you do two separate transfers. Arnd <>< ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=3Djoin.php&p=3Dsourceforge&CID=3DDE= VDEV