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

[Scott Wood <scottwood@freescale.com>]

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