On 11.09.14 12:14, bogdan.purcare...@freescale.com wrote: >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Wednesday, September 10, 2014 4:56 PM >> To: Purcareata Bogdan-B43198; Wood Scott-B07421 >> Cc: Caraman Mihai Claudiu-B02008; qemu-...@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.purcare...@freescale.com wrote: >>>> -----Original Message----- >>>> From: Wood Scott-B07421 >>>> Sent: Friday, September 05, 2014 6:47 PM >>>> To: Purcareata Bogdan-B43198 >>>> Cc: qemu-...@nongnu.org; Caraman Mihai Claudiu-B02008; qemu- >> de...@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