From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56913) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1bV-00087s-0L for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:27:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS1bP-0003do-AW for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:27:52 -0400 Message-ID: <54117922.7@suse.de> Date: Thu, 11 Sep 2014 12:27:46 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1409769375-22286-1-git-send-email-bogdan.purcareata@freescale.com> <1409769375-22286-3-git-send-email-bogdan.purcareata@freescale.com> <1409932036.24184.198.camel@snotra.buserror.net> <6b0bda2e95db48f79fb0767aee7bfc4c@BY2PR03MB189.namprd03.prod.outlook.com> <54105889.7010804@suse.de> <6003c61e8bca47a694cb4ba6d871544f@BN1PR03MB185.namprd03.prod.outlook.com> In-Reply-To: <6003c61e8bca47a694cb4ba6d871544f@BN1PR03MB185.namprd03.prod.outlook.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks based on memory region offset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "bogdan.purcareata@freescale.com" Cc: Scott Wood , "mihai.caraman@freescale.com" , "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" On 11.09.14 12:14, bogdan.purcareata@freescale.com wrote: >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Wednesday, September 10, 2014 4:56 PM >> To: Purcareata Bogdan-B43198; Wood Scott-B07421 >> Cc: Caraman Mihai Claudiu-B02008; qemu-ppc@nongnu.org; qemu-devel@nongnu.org >> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks >> based on memory region offset >> >> >> >> On 10.09.14 13:40, bogdan.purcareata@freescale.com wrote: >>>> -----Original Message----- >>>> From: Wood Scott-B07421 >>>> Sent: Friday, September 05, 2014 6:47 PM >>>> To: Purcareata Bogdan-B43198 >>>> Cc: qemu-ppc@nongnu.org; Caraman Mihai Claudiu-B02008; qemu- >> devel@nongnu.org >>>> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region >> callbacks >>>> based on memory region offset >>>> >>>> On Wed, 2014-09-03 at 14:36 -0400, Bogdan Purcareata wrote: >>>>> This is done due to the fact that the kvm-openpic region_{add,del} >> callbacks >>>>> can be invoked for sections generated from other memory regions as well. >>>> These >>>>> callbacks should handle only requests for the kvm-openpic memory region. >>>>> >>>>> The patch fixes a bug on target-ppc occuring when the "e500-pci-bar0" >> memory >>>>> region is added. This memory region registers an alias to the "e500-ccsr" >>>> memory >>>>> region, which further contains the "kvm-openpic" subregion. Due to this >>>> alias, >>>>> the kvm_openpic_region_add is called once more, with an offset within the >>>>> "e500-pci-bar" memory region. This generates the remapping of the >>>>> in-kernel MPIC at a wrong offset. >>>> >>>> OK, so the problem is that we really do have the MPIC mapped in two >>>> locations (and the kernel API only lets us map one of them). It would >>>> be nice if the MemoryRegionSection struct indicated the alias being >>>> represented rather than needing to recalculate the non-aliased address. >>> >>> Not sure I fully understand the terminology of the alias being represented. >> In fact, out of the two mentioned in the previous mail, "e500-pci-bar0" and >> "e500-ccsr", I don't know which one is the "alias". >>> >>> If it's the former, thus "e500-pci-bar0" is an alias for "e500-ccsr", this >> information could be propagated down to the MemoryRegionSection. However, >> according to [1], "aliases may point to any type of region, including other >> aliases". So if the MemoryRegionSection has been built by going through a >> chain of aliases, all this information must be included in the structure. >>> >>> If it's the latter, thus "e500-ccsr" is an alias for "e500-pci-bar0", we can >> get to it in the current model as well. For our specific case, the >> MemoryRegionSection points to the "kvm-openpic" MemoryRegion, which in turn >> points to "e500-ccsr" as its parent (container). >>> >>> What do you think? >> >> I don't think it matters whether a mapping is an alias or not. What your >> patch really does is it only allows for a single mapping to happen. The >> first one wins. >> >> I think that's reasonable. >> >> However, there's no need to extend the memory API with anything here. >> The only reason you need the additional call is because you need to >> figure out where you think you were mapped. But since we set up the map >> ourselves in an ioctl, we already know where we are mapped. >> >> How about this patch? > > Yes, this patch fixes the issue and follows a simpler approach. > > Would you like to submit it or should I send a v2 with your changes? I've sent it as an official patch. Thanks a lot again for digging down into this! Alex