On 03.04.2013, at 03:57, Scott Wood wrote:
> Hook the MPIC code up to the KVM interfaces, add locking, etc.
>
> TODO: irqfd support, split up into multiple patches, KVM_IRQ_LINE
> support
>
> Signed-off-by: Scott Wood <[email protected]>
> ---
> v3: mpic_put -> kvmppc_mpic_put
>
>
[...]
> +void kvmppc_mpic_set_epr(struct kvm_vcpu *vcpu);
> +
> int kvm_vcpu_ioctl_config_tlb(struct kvm_vcpu *vcpu,
> struct kvm_config_tlb *cfg);
> int kvm_vcpu_ioctl_dirty_tlb(struct kvm_vcpu *vcpu,
> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
> index 63c67ec..a87139b 100644
> --- a/arch/powerpc/kvm/Kconfig
> +++ b/arch/powerpc/kvm/Kconfig
> @@ -151,6 +151,11 @@ config KVM_E500MC
>
> If unsure, say N.
>
> +config KVM_MPIC
> + bool "KVM in-kernel MPIC emulation"
> + depends on KVM
This should probably depend on FSL KVM for now, until someone adds support for
other MPIC revisions.
> +
> +
> source drivers/vhost/Kconfig
>
> endif # VIRTUALIZATION
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index b772ede..4a2277a 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -103,6 +103,8 @@ kvm-book3s_32-objs := \
> book3s_32_mmu.o
> kvm-objs-$(CONFIG_KVM_BOOK3S_32) := $(kvm-book3s_32-objs)
>
> +kvm-objs-$(CONFIG_KVM_MPIC) += mpic.o
> +
> kvm-objs := $(kvm-objs-m) $(kvm-objs-y)
>
> obj-$(CONFIG_KVM_440) += kvm.o
>
[...]
> struct irq_dest {
> + struct kvm_vcpu *vcpu;
> +
> int32_t ctpr; /* CPU current task priority */
> struct irq_queue raised;
> struct irq_queue servicing;
> - qemu_irq *irqs;
>
> /* Count of IRQ sources asserting on non-INT outputs */
> - uint32_t outputs_active[OPENPIC_OUTPUT_NB];
> + uint32_t outputs_active[NUM_OUTPUTS];
> };
>
> +struct openpic;
Isn't this superfluous?
> +
> struct openpic {
> + struct kvm *kvm;
> + struct kvm_io_device mmio;
> + struct list_head mmio_regions;
> + atomic_t users;
> + bool mmio_mapped;
> +
> + gpa_t reg_base;
> + spinlock_t lock;
> +
> /* Behavior control */
> struct fsl_mpic_info *fsl;
> uint32_t model;
> @@ -208,6 +231,47 @@ struct openpic {
> uint32_t irq_msi;
> };
>
>
[...]
> -static uint64_t openpic_gbl_read(void *opaque, gpa_t addr, unsigned len)
> +static int openpic_gbl_read(void *opaque, gpa_t addr, u32 *ptr)
> {
> struct openpic *opp = opaque;
> - uint32_t retval;
> + u32 retval;
>
> - pr_debug("%s: addr %#" HWADDR_PRIx "\n", __func__, addr);
> + pr_debug("%s: addr %#llx\n", __func__, addr);
> retval = 0xFFFFFFFF;
> if (addr & 0xF)
> - return retval;
> + goto out;
>
> switch (addr) {
> case 0x1000: /* FRR */
> retval = opp->frr;
> + retval |= (opp->nb_cpus - 1) << FRR_NCPU_SHIFT;
> break;
> case 0x1020: /* GCR */
> retval = opp->gcr;
> @@ -731,8 +771,8 @@ static uint64_t openpic_gbl_read(void *opaque, gpa_t
> addr, unsigned len)
> case 0x90:
> case 0xA0:
> case 0xB0:
> - retval =
> - openpic_cpu_read_internal(opp, addr, get_current_cpu());
> + retval = openpic_cpu_read_internal(opp, addr,
> + &retval, get_current_cpu());
This looks bogus. You're passing &retval and overwrite it with the return value
right after the function returns?
> break;
> case 0x10A0: /* IPI_IVPR */
> case 0x10B0:
> @@ -750,28 +790,28 @@ static uint64_t openpic_gbl_read(void *opaque, gpa_t
> addr, unsigned len)
> default:
> break;
> }
> - pr_debug("%s: => 0x%08x\n", __func__, retval);
>
> - return retval;
> +out:
> + pr_debug("%s: => 0x%08x\n", __func__, retval);
> + *ptr = retval;
> + return 0;
> }
>
[...]
>
> +static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
> + int len, void *ptr)
> +{
> + struct openpic *opp = container_of(this, struct openpic, mmio);
> + int ret;
> +
> + /*
> + * Technically only 32-bit accesses are allowed, but be nice to
> + * people dumping registers a byte at a time -- it works in real
> + * hardware (reads only, not writes).
Do 16-bit accesses work in real hardware?
> + */
> + if (len == 4) {
> + if (addr & 3) {
> + pr_debug("%s: bad alignment %llx/%d\n",
> + __func__, addr, len);
> + return -EINVAL;
> + }
if (addr & (len - 1))
Then the read_internal call can be shared between the different access sizes,
no?
> +
> + spin_lock_irq(&opp->lock);
> + ret = kvm_mpic_read_internal(opp, addr - opp->reg_base, ptr);
> + spin_unlock_irq(&opp->lock);
> +
> + pr_debug("%s: addr %llx ret %d len 4 val %x\n",
> + __func__, addr, ret, *(const u32 *)ptr);
> + } else if (len == 1) {
> + union {
> + u32 val;
> + u8 bytes[4];
> + } u;
> +
> + spin_lock_irq(&opp->lock);
> + ret = kvm_mpic_read_internal(opp, addr - opp->reg_base, &u.val);
> + spin_unlock_irq(&opp->lock);
> +
> + *(u8 *)ptr = u.bytes[addr & 3];
> +
> + pr_debug("%s: addr %llx ret %d len 1 val %x\n",
> + __func__, addr, ret, *(const u8 *)ptr);
> + } else {
> + pr_debug("%s: bad length %d\n", __func__, len);
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +
[...]
>
> +static int mpic_set_attr(struct openpic *opp, struct kvm_device_attr *attr)
> +{
> + u32 attr32;
> +
> + switch (attr->group) {
> + case KVM_DEV_MPIC_GRP_MISC:
> + switch (attr->attr) {
> + case KVM_DEV_MPIC_BASE_ADDR:
> + return set_base_addr(opp, attr);
> + }
> +
> + break;
> +
> + case KVM_DEV_MPIC_GRP_REGISTER:
> + if (copy_from_user(&attr32, (u32 __user *)(long)attr->addr,
> + sizeof(u32)))
get_user?
> + return -EFAULT;
> +
> + return access_reg(opp, attr->attr, &attr32, ATTR_SET);
> +
> + case KVM_DEV_MPIC_GRP_IRQ_ACTIVE:
> + if (attr->attr > MAX_SRC)
> + return -EINVAL;
> +
> + if (copy_from_user(&attr32, (u32 __user *)(long)attr->addr,
> + sizeof(u32)))
same here
> + return -EFAULT;
> +
> + if (attr32 != 0 && attr32 != 1)
> + return -EINVAL;
> +
> + spin_lock_irq(&opp->lock);
> + openpic_set_irq(opp, attr->attr, attr32);
> + spin_unlock_irq(&opp->lock);
> + return 0;
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int mpic_get_attr(struct openpic *opp, struct kvm_device_attr *attr)
> +{
> + u64 attr64;
> + u32 attr32;
> + int ret;
> +
> + switch (attr->group) {
> + case KVM_DEV_MPIC_GRP_MISC:
> + switch (attr->attr) {
> + case KVM_DEV_MPIC_BASE_ADDR:
> + mutex_lock(&opp->kvm->slots_lock);
> + attr64 = opp->reg_base;
> + mutex_unlock(&opp->kvm->slots_lock);
> +
> + if (copy_to_user((u64 __user *)(long)attr->addr,
> + &attr64, sizeof(u64)))
u64 is tricky with put_user on 32bit hosts, so here copy_to_user makes sense
> + return -EFAULT;
> +
> + return 0;
> + }
> +
> + break;
> +
> + case KVM_DEV_MPIC_GRP_REGISTER:
> + ret = access_reg(opp, attr->attr, &attr32, ATTR_GET);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user((u32 __user *)(long)attr->addr, &attr32,
> + sizeof(u32)))
put_user
> + return -EFAULT;
> +
> + return 0;
> +
> + case KVM_DEV_MPIC_GRP_IRQ_ACTIVE:
> + if (attr->attr > MAX_SRC)
> + return -EINVAL;
> +
> + attr32 = opp->src[attr->attr].pending;
Isn't this missing a lock?
> +
> + if (copy_to_user((u32 __user *)(long)attr->addr, &attr32,
> + sizeof(u32)))
> + return -EFAULT;
> +
> + return 0;
> + }
> +
> + return -ENXIO;
> +}
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
