Re: [Qemu-devel] [PATCH v12 2/5] intc/gic: Extract some reusable vGIC code

[Peter Maydell <peter.maydell@linaro.org>]

On 26 August 2015 at 11:28, Pavel Fedin <p.fedin@samsung.com> wrote:
> Some functions previously used only by vGICv2 are useful also for vGICv3
> implementation. Untie them from GICState and make accessible from within
> other modules:
> - kvm_arm_gic_set_irq()
> - kvm_gic_supports_attr() - moved to common code and renamed to
>   kvm_device_check_attr()
> - kvm_gic_access() - turned into GIC-independent kvm_device_access().
>   Data pointer changed to void * because some GICv3 registers are
>   64-bit wide
> - kvm_gicd_access()
> - kvm_gicc_access() - actually GICv2-specific, but changed to keep the
>   code style unified with kvm_gicd_access()
>
> Some of these changes are not used right now, but they will be helpful for
> implementing live migration.
>
> Actually kvm_dist_get() and kvm_dist_put() could also be made reusable, but
> they would require two extra parameters (s->dev_fd and s->num_cpu) as well as
> lots of typecasts of 's' to DeviceState * and back to GICState *. This makes
> the code very ugly so i decided to stop at this point. I tried also an
> approach with making a base class for all possible GICs, but it would contain
> only three variables (dev_fd, cpu_num and irq_num), and accessing them through
> the rest of the code would be again tedious (either ugly casts or qemu-style
> separate object pointer). So i disliked it too.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Tested-by: Ashok kumar <ashoks@broadcom.com>

Minor issues only:

> -static bool kvm_arm_gic_can_save_restore(GICState *s)
> -{
> -    return s->dev_fd >= 0;
> -}
> -
> -static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
> -{
> -    struct kvm_device_attr attr = {
> -        .group = group,
> -        .attr = attrnum,
> -        .flags = 0,
> -    };
> -
> -    if (s->dev_fd == -1) {
> -        return false;
> -    }
> -
> -    return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
> -}

This function copes with being handed a -1 filedescriptor
(as will happen for old kernels with vGICv2 support but no
irqchip device API support), but your new kvm_device_check_attr()
doesn't. You either need to support it in that new function, or
make the calling code avoid calling it for the -1 case.

> +
> +/**
> + * kvm_arm_gic_set_irq - Send an IRQ to the in-kernel vGIC
> + * @num_irq: Total number of IRQs configured for the GIC instance
> + * @irq: qemu internal IRQ line number:
> + *  [0..N-1] : external interrupts
> + *  [N..N+31] : PPI (internal) interrupts for CPU 0
> + *  [N+32..N+63] : PPI (internal interrupts for CPU 1
> + * @level: level of the IRQ line.
> + */
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level);
> +
> +#define KVM_VGIC_ATTR(offset, cpu) \
> +    ((((uint64_t)(cpu) << KVM_DEV_ARM_VGIC_CPUID_SHIFT) & \
> +      KVM_DEV_ARM_VGIC_CPUID_MASK) | \
> +     (((uint64_t)(offset) << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) & \
> +      KVM_DEV_ARM_VGIC_OFFSET_MASK))

This is assuming that GICv3 will use the same masks/shifts
for register access that GICv2 does at the moment, right?
I'm not sure that's necessarily the case, especially since
the v2 mask limits us to 256 CPUs maximum.

I guess we can fix that up when we need to, though.

> +
> +#define kvm_gicd_access(s, offset, cpu, val, write) \
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, \
> +                      KVM_VGIC_ATTR(offset, cpu), val, write)
> +
> +#define kvm_gicc_access(s, offset, cpu, val, write) \
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS, \
> +                      KVM_VGIC_ATTR(offset, cpu), val, write)

Can you make these two static inline functions, not #defines, please?
Doc comments would be nice too.

thanks
-- PMM