Re: [PATCH 4/5] KVM: s390: Add a channel I/O based virtio transport driver.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 29 Oct 2012 19:12:54 +0100
Alexander Graf <agraf@suse.de> 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.)