On Fri, May 08, 2009 at 04:31:21PM -0600, Alex Williamson wrote:
> +#ifdef KVM_CAP_IRQ_ROUTING
We don't need these anymore.
> +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.
> +
> +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.
> +
> +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
> + if (buf[i] != ~0U)
> + break;
> + }
{} around single statement isn't needed
> +
> + 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
> +
> + 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;
> +
> + gsi = (bit - 1) | (i << 5);
clearer as bit - 1 + i * 32
> + 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?
> + 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.
> + kvm->used_gsi_bitmap = malloc(i >> 3);
why not qemu_malloc?
> + 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 <?
> + set_bit(kvm->used_gsi_bitmap, i);
> + }
> +
> + gsi = kvm_find_free_gsi(kvm);
> + pthread_mutex_unlock(&kvm->gsi_mutex);
> + return gsi;
> #else
> return -ENOSYS;
> #endif
> }
> +
> +void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi)
> +{
> +#ifdef KVM_CAP_IRQ_ROUTING
> + pthread_mutex_lock(&kvm->gsi_mutex);
> + clear_bit(kvm->used_gsi_bitmap, gsi);
> + pthread_mutex_unlock(&kvm->gsi_mutex);
> +#endif
> +}
>
> #ifdef KVM_CAP_DEVICE_MSIX
> int kvm_assign_set_msix_nr(kvm_context_t kvm,
> diff --git a/kvm/libkvm/libkvm.h b/kvm/libkvm/libkvm.h
> index c23d37b..4e9344c 100644
> --- a/kvm/libkvm/libkvm.h
> +++ b/kvm/libkvm/libkvm.h
> @@ -856,6 +856,16 @@ int kvm_commit_irq_routes(kvm_context_t kvm);
> */
> int kvm_get_irq_route_gsi(kvm_context_t kvm);
>
> +/*!
> + * \brief Free used GSI number
> + *
> + * Free used GSI number acquired from kvm_get_irq_route_gsi()
> + *
> + * \param kvm Pointer to the current kvm_context
> + * \param gsi GSI number to free
> + */
> +void kvm_free_irq_route_gsi(kvm_context_t kvm, int gsi);
> +
> #ifdef KVM_CAP_DEVICE_MSIX
> int kvm_assign_set_msix_nr(kvm_context_t kvm,
> struct kvm_assigned_msix_nr *msix_nr);
>
--
MST
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
