Re: [RFC PATCH v3 5/6] kvm/ppc/mpic: in-kernel MPIC emulation

Scott Wood
2013-04-03T21:38:30Z

On 04/03/2013 11:19:42 AM, Alexander Graf wrote:
>
> 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 <scottwood@freescale.com>
> > ---
> > 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.

I don't see a symbol specifically for "FSL KVM". What part of the MPIC
code depends on booke or any FSL-specific code?

> > 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?

Yes, will remove. Probably a leftover from when there was other stuff
in between that referenced it.

> > @@ -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?

Yeah, will fix. Thanks for spotting it.

> > +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?

Probably, though according to the documentation only 32-bit accesses
should be used. As the comment says, I'm just being nice to hexdumps
and such, which are unlikely to use 16-bit accesses.

> > + */
> > + 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?

Not as is, because the read_internal call passes a different pointer in
the two different cases. I originally tried to write this with more in
common between the two cases, and it got a bit messy. I'll give it
another shot, though.

> > + 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?

OK.

> > + 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

What are the issues with put_user? It looks like it's supported with a
pair of "stw" instructions.

> > + 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?

I don't see why it needs one. If the pending status changes during the
ioctl, it's undefined which state you'll read back, and a lock wouldn't
change that (you could end up taking the lock before or after the
change gets made).

reg_base above was a different situation -- it's 64-bit, so we can't
read it atomically on 32-bit.

-Scott