thr3ads.net - Virtualization - [PATCH RFC v6 04/20] virtio: add feature checking helpers [Dec 2014]

If this information is useful, please help other people find it:
Share via:

Thomas Huth

2014-Dec-11 14:46 UTC

[PATCH RFC v6 04/20] virtio: add feature checking helpers

On Thu, 11 Dec 2014 14:25:06 +0100
Cornelia Huck <cornelia.huck at de.ibm.com> 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 <cornelia.huck at de.ibm.com>
...> 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

Michael S. Tsirkin

2014-Dec-11 17:05 UTC

head link

[PATCH RFC v6 04/20] virtio: add feature checking helpers

On Thu, Dec 11, 2014 at 03:46:23PM +0100, Thomas Huth
wrote:> On Thu, 11 Dec 2014 14:25:06 +0100
> Cornelia Huck <cornelia.huck at de.ibm.com> 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 <cornelia.huck at de.ibm.com>
> ...
> > 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?
Yes, please do.

> >          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

Cornelia Huck

2014-Dec-12 08:37 UTC

head link

[PATCH RFC v6 04/20] virtio: add feature checking helpers

On Thu, 11 Dec 2014 19:05:26 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Thu, Dec 11, 2014 at 03:46:23PM +0100, Thomas Huth wrote:
> > On Thu, 11 Dec 2014 14:25:06 +0100
> > Cornelia Huck <cornelia.huck at de.ibm.com> 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 <cornelia.huck at de.ibm.com>
> > ...
> > > 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?
I've clearly been looking at this stuff for too long, as I didn't even
notice this bug :)
> > 
> > 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?
> 
> Yes, please do.
OK, will send.

Cornelia Huck

2014-Dec-12 10:07 UTC

head link

[PATCH RFC v6 04/20] virtio: add feature checking helpers

On Thu, 11 Dec 2014 15:46:23 +0100
Thomas Huth <thuth at linux.vnet.ibm.com> wrote:

> > +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?
Renamed the functions just like this.

Maybe Matching Threads

Search for more maybe matching threads

Virtualization - Dec 2014 - [PATCH RFC v6 04/20] virtio: add feature checking helpers

[PATCH RFC v6 04/20] virtio: add feature checking helpers

[PATCH RFC v6 04/20] virtio: add feature checking helpers

[PATCH RFC v6 04/20] virtio: add feature checking helpers

[PATCH RFC v6 04/20] virtio: add feature checking helpers

Maybe Matching Threads