Re: [PATCH v4 06/13] ARM: KVM: VGIC distributor handling
On 28/11/12 13:21, Will Deacon wrote:
> On Sat, Nov 10, 2012 at 03:44:58PM +, Christoffer Dall wrote:
>> From: Marc Zyngier
>>
>> Add the GIC distributor emulation code. A number of the GIC features
>> are simply ignored as they are not required to boot a Linux guest.
>>
>> Signed-off-by: Marc Zyngier
>> Signed-off-by: Christoffer Dall
>> ---
>> arch/arm/include/asm/kvm_vgic.h | 167 ++
>> arch/arm/kvm/vgic.c | 471
>> +++
>> 2 files changed, 637 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_vgic.h
>> b/arch/arm/include/asm/kvm_vgic.h
>> index 9ca8d21..9e60b1d 100644
>> --- a/arch/arm/include/asm/kvm_vgic.h
>> +++ b/arch/arm/include/asm/kvm_vgic.h
>> @@ -19,10 +19,177 @@
>> #ifndef __ASM_ARM_KVM_VGIC_H
>> #define __ASM_ARM_KVM_VGIC_H
>>
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define VGIC_NR_IRQS 128
>
> #define VGIC_NR_PRIVATE_IRQS32?
>
>> +#define VGIC_NR_SHARED_IRQS(VGIC_NR_IRQS - 32)
>
> then subtract it here
Sure.
>> +#define VGIC_MAX_CPUS NR_CPUS
>
> We already have KVM_MAX_VCPUS, why do we need another?
They really should be the same, and this NR_CPUS is a bug that has
already been fixed.
>> +
>> +/* Sanity checks... */
>> +#if (VGIC_MAX_CPUS > 8)
>> +#error Invalid number of CPU interfaces
>> +#endif
>> +
>> +#if (VGIC_NR_IRQS & 31)
>> +#error "VGIC_NR_IRQS must be a multiple of 32"
>> +#endif
>> +
>> +#if (VGIC_NR_IRQS > 1024)
>> +#error "VGIC_NR_IRQS must be <= 1024"
>> +#endif
>
> Maybe put each check directly below the #define being tested, to make it
> super-obvious to people thinking of changing the constants?
OK.
>> +/*
>> + * The GIC distributor registers describing interrupts have two parts:
>> + * - 32 per-CPU interrupts (SGI + PPI)
>> + * - a bunch of shared interrups (SPI)
>
> interrupts
>
>> + */
>> +struct vgic_bitmap {
>> + union {
>> + u32 reg[1];
>> + unsigned long reg_ul[0];
>> + } percpu[VGIC_MAX_CPUS];
>> + union {
>> + u32 reg[VGIC_NR_SHARED_IRQS / 32];
>> + unsigned long reg_ul[0];
>> + } shared;
>> +};
>
> Whoa, this is nasty!
>
> Firstly, let's replace the `32' with sizeof(u32) for fun. Secondly, can
> we make the reg_ul arrays sized using the BITS_TO_LONGS macro?
This has already been replaced with:
struct vgic_bitmap {
union {
u32 reg[1];
DECLARE_BITMAP(reg_ul, 32);
} percpu[VGIC_MAX_CPUS];
union {
u32 reg[VGIC_NR_SHARED_IRQS / 32];
DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
} shared;
};
which should address most of your concerns. As for the sizeof(u32), I
think assuming that u32 has a grand total of 32bits is safe enough ;-)
>> +
>> +static inline u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
>> + int cpuid, u32 offset)
>> +{
>> + offset >>= 2;
>> + BUG_ON(offset > (VGIC_NR_IRQS / 32));
>
> Hmm, where is offset sanity-checked before here? Do you just rely on all
> trapped accesses being valid?
You've already validated the access being in a valid range. Offset is
just derived the access address. the BUG_ON() is a leftover from early
debugging and should go away now.
>> + if (!offset)
>> + return x->percpu[cpuid].reg;
>> + else
>> + return x->shared.reg + offset - 1;
>
> An alternative to this would be to have a single array, with the per-cpu
> interrupts all laid out at the start and a macro to convert an offset to
> an index. Might make the code more readable and the struct definition more
> concise.
I'll try and see if this makes the code more palatable.
>> +}
>> +
>> +static inline int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
>> +int cpuid, int irq)
>> +{
>> + if (irq < 32)
>
> VGIC_NR_PRIVATE_IRQS (inless you go with the suggestion above)
yep.
>> + return test_bit(irq, x->percpu[cpuid].reg_ul);
>> +
>> + return test_bit(irq - 32, x->shared.reg_ul);
>> +}
>> +
>> +static inline void vgic_bitmap_set_irq_val(struct vgic_bitmap *x,
>> + int cpuid, int irq, int val)
>> +{
>> + unsigned long *reg;
>> +
>> + if (irq < 32)
>> + reg = x->percpu[cpuid].reg_ul;
>> + else {
>> + reg = x->shared.reg_ul;
>> + irq -= 32;
>> + }
>
> Likewise.
>
>> +
>> + if (val)
>> + set_bit(irq, reg);
>> + else
>> + clear_bit(irq, reg);
>> +}
>> +
>> +static inline unsigned long *vgic_bitmap_get_cpu_map(struct vgic_bitmap *x,
>> +int cpuid)
>> +{
>> + if (unlikely(cpuid >= VGIC_MAX_CPUS))
>> + return NULL;
>> + return x->percpu[cpuid].r
Re: [PATCH v4 06/13] ARM: KVM: VGIC distributor handling
On Sat, Nov 10, 2012 at 03:44:58PM +, Christoffer Dall wrote:
> From: Marc Zyngier
>
> Add the GIC distributor emulation code. A number of the GIC features
> are simply ignored as they are not required to boot a Linux guest.
>
> Signed-off-by: Marc Zyngier
> Signed-off-by: Christoffer Dall
> ---
> arch/arm/include/asm/kvm_vgic.h | 167 ++
> arch/arm/kvm/vgic.c | 471
> +++
> 2 files changed, 637 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> index 9ca8d21..9e60b1d 100644
> --- a/arch/arm/include/asm/kvm_vgic.h
> +++ b/arch/arm/include/asm/kvm_vgic.h
> @@ -19,10 +19,177 @@
> #ifndef __ASM_ARM_KVM_VGIC_H
> #define __ASM_ARM_KVM_VGIC_H
>
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define VGIC_NR_IRQS 128
#define VGIC_NR_PRIVATE_IRQS32?
> +#define VGIC_NR_SHARED_IRQS(VGIC_NR_IRQS - 32)
then subtract it here
> +#define VGIC_MAX_CPUS NR_CPUS
We already have KVM_MAX_VCPUS, why do we need another?
> +
> +/* Sanity checks... */
> +#if (VGIC_MAX_CPUS > 8)
> +#error Invalid number of CPU interfaces
> +#endif
> +
> +#if (VGIC_NR_IRQS & 31)
> +#error "VGIC_NR_IRQS must be a multiple of 32"
> +#endif
> +
> +#if (VGIC_NR_IRQS > 1024)
> +#error "VGIC_NR_IRQS must be <= 1024"
> +#endif
Maybe put each check directly below the #define being tested, to make it
super-obvious to people thinking of changing the constants?
> +/*
> + * The GIC distributor registers describing interrupts have two parts:
> + * - 32 per-CPU interrupts (SGI + PPI)
> + * - a bunch of shared interrups (SPI)
interrupts
> + */
> +struct vgic_bitmap {
> + union {
> + u32 reg[1];
> + unsigned long reg_ul[0];
> + } percpu[VGIC_MAX_CPUS];
> + union {
> + u32 reg[VGIC_NR_SHARED_IRQS / 32];
> + unsigned long reg_ul[0];
> + } shared;
> +};
Whoa, this is nasty!
Firstly, let's replace the `32' with sizeof(u32) for fun. Secondly, can
we make the reg_ul arrays sized using the BITS_TO_LONGS macro?
> +
> +static inline u32 *vgic_bitmap_get_reg(struct vgic_bitmap *x,
> + int cpuid, u32 offset)
> +{
> + offset >>= 2;
> + BUG_ON(offset > (VGIC_NR_IRQS / 32));
Hmm, where is offset sanity-checked before here? Do you just rely on all
trapped accesses being valid?
> + if (!offset)
> + return x->percpu[cpuid].reg;
> + else
> + return x->shared.reg + offset - 1;
An alternative to this would be to have a single array, with the per-cpu
interrupts all laid out at the start and a macro to convert an offset to
an index. Might make the code more readable and the struct definition more
concise.
> +}
> +
> +static inline int vgic_bitmap_get_irq_val(struct vgic_bitmap *x,
> +int cpuid, int irq)
> +{
> + if (irq < 32)
VGIC_NR_PRIVATE_IRQS (inless you go with the suggestion above)
> + return test_bit(irq, x->percpu[cpuid].reg_ul);
> +
> + return test_bit(irq - 32, x->shared.reg_ul);
> +}
> +
> +static inline void vgic_bitmap_set_irq_val(struct vgic_bitmap *x,
> + int cpuid, int irq, int val)
> +{
> + unsigned long *reg;
> +
> + if (irq < 32)
> + reg = x->percpu[cpuid].reg_ul;
> + else {
> + reg = x->shared.reg_ul;
> + irq -= 32;
> + }
Likewise.
> +
> + if (val)
> + set_bit(irq, reg);
> + else
> + clear_bit(irq, reg);
> +}
> +
> +static inline unsigned long *vgic_bitmap_get_cpu_map(struct vgic_bitmap *x,
> +int cpuid)
> +{
> + if (unlikely(cpuid >= VGIC_MAX_CPUS))
> + return NULL;
> + return x->percpu[cpuid].reg_ul;
> +}
> +
> +static inline unsigned long *vgic_bitmap_get_shared_map(struct vgic_bitmap
> *x)
> +{
> + return x->shared.reg_ul;
> +}
> +
> +struct vgic_bytemap {
> + union {
> + u32 reg[8];
> + unsigned long reg_ul[0];
> + } percpu[VGIC_MAX_CPUS];
> + union {
> + u32 reg[VGIC_NR_SHARED_IRQS / 4];
> + unsigned long reg_ul[0];
> + } shared;
> +};
Argh, it's another one! :)
> +
> +static inline u32 *vgic_bytemap_get_reg(struct vgic_bytemap *x,
> + int cpuid, u32 offset)
> +{
> + offset >>= 2;
> + BUG_ON(offset > (VGIC_NR_IRQS / 4));
> + if (offset < 4)
> + return x->percpu[cpuid].reg + offset;
> + else
> + return x->shared.reg + offset - 8;
> +}
> +
> +static inline int vgic_bytemap_get_irq_val(struct vgic_bytemap *x,
> + int cpuid, int irq)
> +{
> + u32 *reg, shift;
> +
Re: [PATCH v4 06/13] ARM: KVM: VGIC distributor handling
On Mon, Nov 12, 2012 at 4:29 AM, Dong Aisheng wrote:
> On Sat, Nov 10, 2012 at 04:44:58PM +0100, Christoffer Dall wrote:
> [...]
>> @@ -141,7 +519,98 @@ struct mmio_range *find_matching_range(const struct
>> mmio_range *ranges,
>> */
>> bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, struct
>> kvm_exit_mmio *mmio)
>> {
>> - return KVM_EXIT_MMIO;
>> + const struct mmio_range *range;
>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>> + unsigned long base = dist->vgic_dist_base;
>> + bool updated_state;
>> +
>> + if (!irqchip_in_kernel(vcpu->kvm) ||
>> + mmio->phys_addr < base ||
>> + (mmio->phys_addr + mmio->len) > (base + dist->vgic_dist_size))
>> + return false;
>> +
>> + range = find_matching_range(vgic_ranges, mmio, base);
>> + if (unlikely(!range || !range->handle_mmio)) {
>> + pr_warn("Unhandled access %d %08llx %d\n",
>> + mmio->is_write, mmio->phys_addr, mmio->len);
>> + return false;
>> + }
>> +
>> + spin_lock(&vcpu->kvm->arch.vgic.lock);
>> + updated_state = range->handle_mmio(vcpu, mmio,mmio->phys_addr -
>> range->base - base);
> Missing space after ','.
> Checkpatch may fail here.
>
thanks,
-Christoffer
--
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
Re: [PATCH v4 06/13] ARM: KVM: VGIC distributor handling
On Sat, Nov 10, 2012 at 04:44:58PM +0100, Christoffer Dall wrote:
[...]
> @@ -141,7 +519,98 @@ struct mmio_range *find_matching_range(const struct
> mmio_range *ranges,
> */
> bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, struct
> kvm_exit_mmio *mmio)
> {
> - return KVM_EXIT_MMIO;
> + const struct mmio_range *range;
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + unsigned long base = dist->vgic_dist_base;
> + bool updated_state;
> +
> + if (!irqchip_in_kernel(vcpu->kvm) ||
> + mmio->phys_addr < base ||
> + (mmio->phys_addr + mmio->len) > (base + dist->vgic_dist_size))
> + return false;
> +
> + range = find_matching_range(vgic_ranges, mmio, base);
> + if (unlikely(!range || !range->handle_mmio)) {
> + pr_warn("Unhandled access %d %08llx %d\n",
> + mmio->is_write, mmio->phys_addr, mmio->len);
> + return false;
> + }
> +
> + spin_lock(&vcpu->kvm->arch.vgic.lock);
> + updated_state = range->handle_mmio(vcpu, mmio,mmio->phys_addr -
> range->base - base);
Missing space after ','.
Checkpatch may fail here.
Regards
Dong Aisheng
--
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
