| Message ID | CANM98qLHao0dJU57MbAtopVpTJDOVdqNtw5JT-DXyQNa7iD_qw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Christoffer Dall <c.dall@virtualopensystems.com> writes: > On Mon, Jan 14, 2013 at 11:24 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >> On Tue, Jan 08, 2013 at 01:38:55PM -0500, Christoffer Dall wrote: >>> + /* -ENOENT for unknown features, -EINVAL for invalid combinations. */ >>> + for (i = 0; i < sizeof(init->features)*8; i++) { >>> + if (init->features[i / 32] & (1 << (i % 32))) { >> >> Isn't this an open-coded version of test_bit() ? > > indeed, nicely spotted: BTW, I wrote it that was out of excessive paranoia: it's a userspace API, and test_bit() won't be right on 64 bit BE systems. Cheers, Rusty.
On Wed, Jan 16, 2013 at 01:26:01PM +1030, Rusty Russell wrote: > Christoffer Dall <c.dall@virtualopensystems.com> writes: > > > On Mon, Jan 14, 2013 at 11:24 AM, Russell King - ARM Linux > > <linux@arm.linux.org.uk> wrote: > >> On Tue, Jan 08, 2013 at 01:38:55PM -0500, Christoffer Dall wrote: > >>> + /* -ENOENT for unknown features, -EINVAL for invalid combinations. */ > >>> + for (i = 0; i < sizeof(init->features)*8; i++) { > >>> + if (init->features[i / 32] & (1 << (i % 32))) { > >> > >> Isn't this an open-coded version of test_bit() ? > > > > indeed, nicely spotted: > > BTW, I wrote it that was out of excessive paranoia: it's a userspace > API, and test_bit() won't be right on 64 bit BE systems. So why is this a concern for 32-bit systems (which are, by definition, only in arch/arm) ?
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Wed, Jan 16, 2013 at 01:26:01PM +1030, Rusty Russell wrote: >> Christoffer Dall <c.dall@virtualopensystems.com> writes: >> >> > On Mon, Jan 14, 2013 at 11:24 AM, Russell King - ARM Linux >> > <linux@arm.linux.org.uk> wrote: >> >> On Tue, Jan 08, 2013 at 01:38:55PM -0500, Christoffer Dall wrote: >> >>> + /* -ENOENT for unknown features, -EINVAL for invalid combinations. */ >> >>> + for (i = 0; i < sizeof(init->features)*8; i++) { >> >>> + if (init->features[i / 32] & (1 << (i % 32))) { >> >> >> >> Isn't this an open-coded version of test_bit() ? >> > >> > indeed, nicely spotted: >> >> BTW, I wrote it that was out of excessive paranoia: it's a userspace >> API, and test_bit() won't be right on 64 bit BE systems. > > So why is this a concern for 32-bit systems (which are, by definition, > only in arch/arm) ? Because this feature bitmap system is the same code which other archs *should* be using for specifying kvm cpus :) Cheers, Rusty.
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c index 65ae563..2339d96 100644 --- a/arch/arm/kvm/guest.c +++ b/arch/arm/kvm/guest.c @@ -193,8 +193,8 @@ int kvm_vcpu_set_target(struct kvm_vcpu *vcpu, bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES); /* -ENOENT for unknown features, -EINVAL for invalid combinations. */ - for (i = 0; i < sizeof(init->features)*8; i++) { - if (init->features[i / 32] & (1 << (i % 32))) { + for (i = 0; i < sizeof(init->features) * 8; i++) { + if (test_bit(i, (void *)init->features)) { if (i >= KVM_VCPU_MAX_FEATURES) return -ENOENT; set_bit(i, vcpu->arch.features);