Hi Pavel,
On 08/18/2015 03:33 PM, Pavel Fedin wrote:
> These functions are useful also for vGICv3 implementation. Make them
> accessible
> from within other modules.
I think it would be worth justifying the changes in signature:
removal of GICState* due to the introduction of GICV3State and also
justify replacement of uint32_t *val into void*.
>
> 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.
I would put the above paragraph below "---"
>
> Signed-off-by: Pavel Fedin <p.fe...@samsung.com>
> ---
> hw/intc/arm_gic_kvm.c | 42 +++++++++++++++++----------------------
> hw/intc/vgic_common.h | 55
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 73 insertions(+), 24 deletions(-)
> create mode 100644 hw/intc/vgic_common.h
>
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index e5d0f67..0c71ef8 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -23,6 +23,7 @@
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> #include "gic_internal.h"
> +#include "vgic_common.h"
>
> //#define DEBUG_GIC_KVM
>
> @@ -52,7 +53,7 @@ typedef struct KVMARMGICClass {
> void (*parent_reset)(DeviceState *dev);
> } KVMARMGICClass;
>
> -static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level)
> {
> /* Meaning of the 'irq' parameter:
> * [0..N-1] : external interrupts
> @@ -63,10 +64,9 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int
> level)
> * has separate fields in the irq number for type,
> * CPU number and interrupt number.
> */
> - GICState *s = (GICState *)opaque;
> int kvm_irq, irqtype, cpu;
>
> - if (irq < (s->num_irq - GIC_INTERNAL)) {
> + if (irq < (num_irq - GIC_INTERNAL)) {
> /* External interrupt. The kernel numbers these like the GIC
> * hardware, with external interrupt IDs starting after the
> * internal ones.
> @@ -77,7 +77,7 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int
> level)
> } else {
> /* Internal interrupt: decode into (cpu, interrupt id) */
> irqtype = KVM_ARM_IRQ_TYPE_PPI;
> - irq -= (s->num_irq - GIC_INTERNAL);
> + irq -= (num_irq - GIC_INTERNAL);
> cpu = irq / GIC_INTERNAL;
> irq %= GIC_INTERNAL;
> }
> @@ -87,6 +87,13 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int
> level)
> kvm_set_irq(kvm_state, kvm_irq, !!level);
> }
>
> +static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)
inline?
> +{
> + GICState *s = (GICState *)opaque;
> +
> + kvm_arm_gic_set_irq(s->num_irq, irq, level);
> +}
> +
> static bool kvm_arm_gic_can_save_restore(GICState *s)
> {
> return s->dev_fd >= 0;
> @@ -107,8 +114,8 @@ static bool kvm_gic_supports_attr(GICState *s, int group,
> int attrnum)
> return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
> }
>
> -static void kvm_gic_access(GICState *s, int group, int offset,
> - int cpu, uint32_t *val, bool write)
> +void kvm_gic_access(int dev_fd, int group, int offset,
> + int cpu, void *val, bool write)
> {
> struct kvm_device_attr attr;
> int type;
> @@ -130,7 +137,7 @@ static void kvm_gic_access(GICState *s, int group, int
> offset,
> type = KVM_GET_DEVICE_ATTR;
> }
>
> - err = kvm_device_ioctl(s->dev_fd, type, &attr);
> + err = kvm_device_ioctl(dev_fd, type, &attr);
> if (err < 0) {
> fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
> strerror(-err));
> @@ -138,20 +145,6 @@ static void kvm_gic_access(GICState *s, int group, int
> offset,
> }
> }
>
> -static void kvm_gicd_access(GICState *s, int offset, int cpu,
> - uint32_t *val, bool write)
> -{
> - kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> - offset, cpu, val, write);
> -}
> -
> -static void kvm_gicc_access(GICState *s, int offset, int cpu,
> - uint32_t *val, bool write)
> -{
> - kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
> - offset, cpu, val, write);
> -}
> -
> #define for_each_irq_reg(_ctr, _max_irq, _field_width) \
> for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
>
> @@ -559,7 +552,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error
> **errp)
> return;
> }
>
> - gic_init_irqs_and_mmio(s, kvm_arm_gic_set_irq, NULL);
> + gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
>
> for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
> qemu_irq irq = qdev_get_gpio_in(dev, i);
> @@ -578,13 +571,14 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error
> **errp)
>
> if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
> uint32_t numirqs = s->num_irq;
> - kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
> + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0,
> + &numirqs, 1);
> }
>
> /* Tell the kernel to complete VGIC initialization now */
> if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> KVM_DEV_ARM_VGIC_CTRL_INIT)) {
> - kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
> }
>
> diff --git a/hw/intc/vgic_common.h b/hw/intc/vgic_common.h
> new file mode 100644
> index 0000000..130ea64
> --- /dev/null
> +++ b/hw/intc/vgic_common.h
> @@ -0,0 +1,55 @@
> +/*
> + * ARM KVM vGIC utility functions
> + *
> + * Copyright (c) 2015 Samsung Electronics
> + * Written by Pavel Fedin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef QEMU_ARM_VGIC_COMMON_H
> +#define QEMU_ARM_VGIC_COMMON_H
> +
> +/**
> + * 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);
> +
> +/**
> + * kvm_gic_access - Read or write vGIC memory-mapped register
> + * @dev_fd: fd of the device to act on
> + * @group: ID of the memory-mapped region
> + * @offset: offset within the region
> + * @cpu: vCPU number
> + * @val: pointer to the storage area for the data
> + * @write - true for writing and false for reading
> + */
> +void kvm_gic_access(int dev_fd, int group, int offset, int cpu,
> + void *val, bool write);
> +
> +#define kvm_gicd_access(s, offset, cpu, val, write) \
> + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, \
> + offset, cpu, val, write)
> +
> +#define kvm_gicc_access(s, offset, cpu, val, write) \
> + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS, \
> + offset, cpu, val, write)
what is the point of moving kvm_gicd_access and kvm_gicc_access here? If
I am not mistaken, they only are used in arm_gic_kvm.c? I think they can
stay static in arm_gic_kvm.c?
Best Regards
Eric
> +
> +#endif
>
