On Tue, 2009-05-12 at 22:39 +0300, Michael S. Tsirkin wrote:
> On Fri, May 08, 2009 at 04:31:21PM -0600, Alex Williamson wrote:
> > +#ifdef KVM_CAP_IRQ_ROUTING
>
> We don't need these anymore.
Doesn't that depend on the kernel it's built against? In any case, I
think it would deserve it's own patch to remove those throughout.
Removing them here would make their usage inconsistent.
> > +static inline void set_bit(unsigned int *buf, int bit)
> > +{
> > + buf[bit >> 5] |= (1U << (bit & 0x1f));
> > +}
>
> external () not needed here. bit >> 5 might be clearer as bit / 32 IMO.
Fixed
> > +
> > +static inline void clear_bit(unsigned int *buf, int bit)
> > +{
> > + buf[bit >> 5] &= ~(1U << (bit & 0x1f));
> > +}
>
> Make bit unsigned. And then bit & 0x1f can be written as bit % 32.
> IMO that's easier to parse.
Fixed
> > +
> > +static int kvm_find_free_gsi(kvm_context_t kvm)
> > +{
> > + int i, bit, gsi;
> > + unsigned int *buf = kvm->used_gsi_bitmap;
> > +
> > + for (i = 0; i < (kvm->max_gsi >> 5); i++) {
>
> may be clearer as kvm->max_gsi / 32
Fixed
> > + if (buf[i] != ~0U)
> > + break;
> > + }
>
> {} around single statement isn't needed
This is different in my new version, so the {} makes sense.
> > +
> > + if (i == kvm->max_gsi >> 5)
> > + return -ENOSPC;
>
> May be clearer as kvm->max_gsi / 32
> By the way, this math means we can't use all gsi's
> if the number is not a multiple of 32.
> Round up instead? It's not hard: (kvm->max_gsi + 31) / 32
Fixed
> > +
> > + bit = ffs(~buf[i]);
> > + if (!bit)
> > + return -EAGAIN;
>
> We know it won't be 0, right?
> Instead of checking twice, move the ffs call within the loop above?
> E.g. like this:
> for (i = 0; i < kvm->max_gsi / 32; ++i)
> if ((bit = ffs(~buf[i])) {
> gsi = bit - 1 + i * 32;
> set_bit(buf, gsi);
> return gsi;
> }
>
> return -ENOSPC;
Nice, I updated to something similar.
> > +
> > + gsi = (bit - 1) | (i << 5);
>
> clearer as bit - 1 + i * 32
Yup, fixed.
> > + set_bit(buf, gsi);
> > + return gsi;
> > +}
> > +#endif
> > +
> > int kvm_get_irq_route_gsi(kvm_context_t kvm)
> > {
> > #ifdef KVM_CAP_IRQ_ROUTING
> > - if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS) {
> > - if (kvm->max_used_gsi + 1 < kvm_get_gsi_count(kvm))
> > - return kvm->max_used_gsi + 1;
> > - else
> > - return -ENOSPC;
> > - } else
> > - return KVM_IOAPIC_NUM_PINS;
> > + int gsi;
> > +
> > + pthread_mutex_lock(&kvm->gsi_mutex);
> > +
> > + if (!kvm->max_gsi) {
>
> Why is this lazy allocation required?
> Let's do the below in some init function,
> and keep code simple?
Moved to kvm_init()
> > + int i;
> > +
> > + /* Round the number of GSIs supported to a 4 byte
> > + * value so we can search it using ints and ffs */
> > + i = kvm_get_gsi_count(kvm) & ~0x1f;
>
> Should not we round up? Why not?
> Again, we can count = kvm_get_gsi_count(kvm) / 32, and use count * 4 below.
Yep, rounded up, and pre-marked the excess bits as used.
> > + kvm->used_gsi_bitmap = malloc(i >> 3);
>
> why not qemu_malloc?
qemu_malloc isn't available in libkvm.c. malloc is consistent with the
rest of the file.
> > + if (!kvm->used_gsi_bitmap) {
> > + pthread_mutex_unlock(&kvm->gsi_mutex);
> > + return -ENOMEM;
> > + }
> > + memset(kvm->used_gsi_bitmap, 0, i >> 3);
> > + kvm->max_gsi = i;
> > +
> > + /* Mark all the IOAPIC pin GSIs as already used */
> > + for (i = 0; i <= KVM_IOAPIC_NUM_PINS; i++)
>
> Is this really <=? Not <?
Fixed.
Thanks, I'll send out a new version shortly.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
