From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [RFC PATCH v3 5/6] kvm/ppc/mpic: in-kernel MPIC emulation Date: Wed, 3 Apr 2013 17:54:34 -0500 Message-ID: <1365029674.25627.17@snotra> References: <1365026850.25627.16@snotra> <87E821FC-0AFF-462E-947B-FCF2BC4AB5EB@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Transfer-Encoding: 8BIT Cc: "" , "" , "" To: Alexander Graf Return-path: In-Reply-To: <87E821FC-0AFF-462E-947B-FCF2BC4AB5EB@suse.de> (from agraf@suse.de on Wed Apr 3 17:12:06 2013) Content-Disposition: inline Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 04/03/2013 05:12:06 PM, Alexander Graf wrote: > > > Am 04.04.2013 um 00:07 schrieb Scott Wood : > > > On 04/03/2013 04:58:56 PM, Alexander Graf wrote: > >> Am 03.04.2013 um 23:38 schrieb Scott Wood > : > >> > 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 > >> >> > --- > >> >> > 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? > >> You support only FSL mpic device IDs :). So if someone on book3s > goes along and sees this, he'd think "yes, I want an in-kernel MPIC", > enables the option and wastes space. > > > > "Would this waste space" is not generally the criteria for kconfig > dependencies. Who is the kernel to get in the way of someone that > wants an FSL MPIC on a 4xx VM? :-) > > > > And again, there's no symbol for FSL KVM -- I'd have to use a list > that could get out of date. And it would reduce build testing in > allyesconfig-type configs. > > Ok, please indicate compatibility limitations in the Kconfig > description at least then. OK -- not really a "compatibility" limitation so much as what models are supported. Note that mpc86xx has a 74xx-derived core, but also has an FSL MPIC... Is 74xx/e600 supported by book3s_pr? Can't tell from the kconfig text. :-) > >> >> > + 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. > >> Ok, so this relies on 32bit read accesses being atomic and stale > values ok. > > > > Not just 32-bit, but only two possible values, with one bitflip > between them... Even if GCC does the read a byte at a time it'd be > OK. > > > >> That works for me, but deserves a comment. > > > > If I'm going to change it, might as well just put the lock in to be > consistent. > > Either way works, just want to make sure whoever reads the code knows > that things were done on purpose :) OTOH, it looks like access_reg() *was* missing a lock. :-P -Scott