From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34819) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz50p-0003Hg-QQ for qemu-devel@nongnu.org; Thu, 11 Dec 2014 09:46:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xz50f-00041W-Br for qemu-devel@nongnu.org; Thu, 11 Dec 2014 09:46:39 -0500 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:45854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz50f-00040w-3u for qemu-devel@nongnu.org; Thu, 11 Dec 2014 09:46:29 -0500 Received: from /spool/local by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Dec 2014 14:46:27 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id D802417D8056 for ; Thu, 11 Dec 2014 14:46:48 +0000 (GMT) Received: from d06av03.portsmouth.uk.ibm.com (d06av03.portsmouth.uk.ibm.com [9.149.37.213]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sBBEkQq062324758 for ; Thu, 11 Dec 2014 14:46:26 GMT Received: from d06av03.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av03.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sBBEkPba018995 for ; Thu, 11 Dec 2014 07:46:25 -0700 Date: Thu, 11 Dec 2014 15:46:23 +0100 From: Thomas Huth Message-ID: <20141211154623.4a73fef9@oc7435384737.ibm.com> In-Reply-To: <1418304322-7546-5-git-send-email-cornelia.huck@de.ibm.com> References: <1418304322-7546-1-git-send-email-cornelia.huck@de.ibm.com> <1418304322-7546-5-git-send-email-cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v6 04/20] virtio: add feature checking helpers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: rusty@rustcorp.com.au, mst@redhat.com, qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org On Thu, 11 Dec 2014 14:25:06 +0100 Cornelia Huck wrote: > Add a helper function for checking whether a bit is set in the guest > features for a vdev as well as one that works on a feature bit set. > > Convert code that open-coded this: It cleans up the code and makes it > easier to extend the guest feature bits. > > Signed-off-by: Cornelia Huck ... > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index ef48550..56c92fb 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -144,7 +144,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req, > * > * TODO: always disable this workaround for virtio 1.0 devices. > */ > - if ((vdev->guest_features & VIRTIO_F_ANY_LAYOUT) == 0) { > + if (!virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT)) { Wait ... this does not only look like a clean-up, but also like a bug-fix to me, since it should have been "(1 << VIRTIO_F_ANY_LAYOUT)" instead of "VIRTIO_F_ANY_LAYOUT" in the original code instead? So in case this patch queue takes a little bit longer 'til it gets upstream, do we might want to submit a separate patch for fixing this issue first? > req_size = req->elem.out_sg[0].iov_len; > resp_size = req->elem.in_sg[0].iov_len; > } > @@ -748,7 +748,7 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense) > VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus); > VirtIODevice *vdev = VIRTIO_DEVICE(s); > > - if (((vdev->guest_features >> VIRTIO_SCSI_F_CHANGE) & 1) && > + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_CHANGE) && > dev->type != TYPE_ROM) { > virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, > sense.asc | (sense.ascq << 8)); > @@ -769,7 +769,7 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, > blk_op_block_all(sd->conf.blk, s->blocker); > } > > - if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) { > + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { > virtio_scsi_push_event(s, sd, > VIRTIO_SCSI_T_TRANSPORT_RESET, > VIRTIO_SCSI_EVT_RESET_RESCAN); > @@ -783,7 +783,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, > VirtIOSCSI *s = VIRTIO_SCSI(vdev); > SCSIDevice *sd = SCSI_DEVICE(dev); > > - if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) { > + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { > virtio_scsi_push_event(s, sd, > VIRTIO_SCSI_T_TRANSPORT_RESET, > VIRTIO_SCSI_EVT_RESET_REMOVED); ... > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 2fede2e..f6c0379 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -278,6 +278,17 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > *features &= ~(1 << fbit); > } > > +static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > +{ > + assert(fbit < 32); > + return !!(features & (1 << fbit)); > +} > + > +static inline bool virtio_has_feature(VirtIODevice *vdev, unsigned int fbit) > +{ > + return __virtio_has_feature(vdev->guest_features, fbit); > +} > + I've got to say that I'm a little bit unhappy with the naming of the functions - and in contrast to the Linux kernel code, I think it is also quite uncommon in the QEMU sources to use function names with double underscores at the beginning. Could you maybe rename the second function to "virtio_vdev_has_feature" instead? And then remove the double underscores from the first function? Thomas