lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20141127162804.GB27124@redhat.com>
Date:	Thu, 27 Nov 2014 18:28:04 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	David Hildenbrand <dahi@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, David Miller <davem@...emloft.net>,
	cornelia.huck@...ibm.com, rusty@....ibm.com, nab@...ux-iscsi.org,
	pbonzini@...hat.com, thuth@...ux.vnet.ibm.com,
	Rusty Russell <rusty@...tcorp.com.au>,
	Brian Swetland <swetland@...gle.com>,
	Christian Borntraeger <borntraeger@...ibm.com>,
	Pawel Moll <pawel.moll@....com>,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Amit Shah <amit.shah@...hat.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux390@...ibm.com, Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Ashutosh Dixit <ashutosh.dixit@...el.com>,
	Sudeep Dutt <sudeep.dutt@...el.com>,
	Siva Yerramreddy <yshivakrishna@...il.com>,
	Joel Stanley <joel@....id.au>,
	virtualization@...ts.linux-foundation.org, lguest@...ts.ozlabs.org,
	linux-s390@...r.kernel.org
Subject: Re: [PATCH v5 01/45] virtio: use u32, not bitmap for struct
 virtio_device's features

On Thu, Nov 27, 2014 at 05:15:42PM +0100, David Hildenbrand wrote:
> > From: Rusty Russell <rusty@...tcorp.com.au>
> > 
> > It seemed like a good idea, but it's actually a pain when we get more
> > than 32 feature bits.  Just change it to a u32 for now.
> > 
> > Cc: Brian Swetland <swetland@...gle.com>
> > Cc: Christian Borntraeger <borntraeger@...ibm.com>
> > Signed-off-by: Rusty Russell <rusty@...tcorp.com.au>
> > Signed-off-by: Cornelia Huck <cornelia.huck@...ibm.com>
> > Acked-by: Pawel Moll <pawel.moll@....com>
> > Acked-by: Ohad Ben-Cohen <ohad@...ery.com>
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> > ---
> 
> Wouldn't it be easier to turn
>   test_bit(i, dev->features)
> 
> into a simple makro like
>   #define test_bit(i, features) (features & (1 << i))
> 
> Similar one for clear_bit and set_bit.

I don't really see why it's easier.
These wrappers resulted in a ton of code
converting from bit numbers to set_bit calls
and back.

E.g. with plan integer you can do (a|b|c)
to get a set with 3 bits.


> 
> if names collide with existing functions, we could simply rename them to:
> 
> set_feature_bit() ...
> clear_feature_bit() ...

In fact that's why we have BIT macro.

vdev->features & BIT_ULL(x) is only longer by 1
character than test_feature_bit(vdev, x), and
it's much clearer than a non standard wrapper.

vdev->features & (1ULL << x) is longer by 3 chars.

I guess this is a suggestion to use BIT / BIT_ULL more.

Good point.


> > diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
> > index e647947..0acd564 100644
> > --- a/drivers/misc/mic/card/mic_virtio.c
> > +++ b/drivers/misc/mic/card/mic_virtio.c
> > @@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev)
> >  	bits = min_t(unsigned, feature_len,
> >  		sizeof(vdev->features)) * 8;
> >  	for (i = 0; i < bits; i++) {
> > -		if (test_bit(i, vdev->features))
> > +		if (vdev->features & BIT(bits))
> 
> Hm, shouldn't that also be "if (vdev->features & (1 << i))"?

Ouch. you are right of course. Will fix.

> >  			iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
> >  				 &out_features[i / 8]);
> >  	}
> 
> >  out_free:
> >  	kfree(features);
> >  	kfree(ccw);
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index df598dd..7814b1f 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -49,9 +49,9 @@ static ssize_t features_show(struct device *_d,
> > 
> >  	/* We actually represent this as a bitstring, as it could be
> >  	 * arbitrary length in future. */
> > -	for (i = 0; i < ARRAY_SIZE(dev->features)*BITS_PER_LONG; i++)
> > +	for (i = 0; i < sizeof(dev->features)*8; i++)
> 
> ... sizeof(dev->features) * 8; ...
> 
> Do we have a define for 8 ?

We do: BITS_PER_BYTE.

> >  		len += sprintf(buf+len, "%c",
> > -			       test_bit(i, dev->features) ? '1' : '0');
> > +			       dev->features & (1ULL << i) ? '1' : '0');
> >  	len += sprintf(buf+len, "\n");
> >  	return len;
> >  }
> > @@ -168,18 +168,18 @@ static int virtio_dev_probe(struct device *_d)
> >  	device_features = dev->config->get_features(dev);
> > 
> 
> [...]
> 
> > @@ -483,7 +483,7 @@ int main(int argc, char *argv[])
> > 
> >  	/* Set up host side. */
> >  	vring_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN);
> > -	vringh_init_user(&vrh, vdev.features[0], RINGSIZE, true,
> > +	vringh_init_user(&vrh, vdev.features, RINGSIZE, true,
> >  			 vrh.vring.desc, vrh.vring.avail, vrh.vring.used);
> > 
> >  	/* No descriptor to get yet... */
> > @@ -652,13 +652,13 @@ int main(int argc, char *argv[])
> >  	}
> > 
> >  	/* Test weird (but legal!) indirect. */
> > -	if (vdev.features[0] & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
> > +	if (vdev.features & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
> >  		char *data = __user_addr_max - USER_MEM/4;
> >  		struct vring_desc *d = __user_addr_max - USER_MEM/2;
> >  		struct vring vring;
> > 
> >  		/* Force creation of direct, which we modify. */
> > -		vdev.features[0] &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
> > +		vdev.features &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
> 
> That could then be something like 
> 
> clear_feature_bit(VIRTIO_RING_F_INDIRECT_DESC, vdev.features);
> 
> (I would prefer such makros over repeating bit actions)

That's the whole reason for the patch.

I guess you disagree with it, but it's much easier
to deal with simple integers.


> >  		vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true,
> >  					 __user_addr_min,
> >  					 never_notify_host,
> 
> 
> But on the whole this looks good to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ