On 26 August 2015 at 11:28, Pavel Fedin <p.fe...@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.fe...@samsung.com>
> Tested-by: Ashok kumar <ash...@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
