From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cornelia Huck Subject: Re: [PATCH 4/5] KVM: s390: Add a channel I/O based virtio transport driver. Date: Mon, 29 Oct 2012 19:34:47 +0100 Message-ID: <20121029193447.05dad97b@BR9GNB5Z> References: <1351516046-30037-1-git-send-email-cornelia.huck@de.ibm.com> <1351516046-30037-5-git-send-email-cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , KVM , linux-s390 , Christian Borntraeger , Carsten Otte , Heiko Carstens , Martin Schwidefsky , Sebastian Ott To: Alexander Graf Return-path: Received: from e06smtp18.uk.ibm.com ([195.75.94.114]:34514 "EHLO e06smtp18.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932260Ab2J2Sey (ORCPT ); Mon, 29 Oct 2012 14:34:54 -0400 Received: from /spool/local by e06smtp18.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Oct 2012 18:34:52 -0000 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 29 Oct 2012 19:12:54 +0100 Alexander Graf wrote: > > On 29.10.2012, at 14:07, Cornelia Huck wrote: > > +static void virtio_ccw_kvm_notify(struct virtqueue *vq) > > +{ > > + struct virtio_ccw_vq_info *info = vq->priv; > > + struct virtio_ccw_device *vcdev; > > + struct subchannel_id schid; > > + __u32 reg2; > > + > > + vcdev = to_vc_device(info->vq->vdev); > > + ccw_device_get_schid(vcdev->cdev, &schid); > > + reg2 = *(__u32 *)&schid; > > That cast looks quite ugly. Can't you just access the field in there you need? Or if it's multiple fields do a union over them? Or assemble them by hand in C? I think the cast looks less ugly than using a union to morph it around. I want the schid with all fields filled out anyway, since this is what identifies the subchannel. > > > + kvm_hypercall2(3 /* CCW_NOTIFY */, reg2, info->queue_index); > > This wants to be a #define :) Probably :) > > > +} > > + > > +static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, int index) > > +{ > > + vcdev->config_block->index = index; > > + vcdev->ccw->cmd_code = CCW_CMD_READ_VQ_CONF; > > + vcdev->ccw->flags = 0; > > + vcdev->ccw->count = sizeof(struct vq_config_block); > > + vcdev->ccw->cda = (__u32)(unsigned long)(vcdev->config_block); > > Is this casting a pointer to a u32? What if this is in highmem? Ah, I just saw the comment that ccw memory needs to be <2GB. Phew. Any plans to get rid of that limitation? Well, we could do full-blown IDAW handling to get to 64bit addresses - which would need a lot of extra code in the host. I doubt whether it would be worth it. (Well, we'll probably want IDAWs sometime in the future - I just think it's overkill for those tiny snippets.)