| Message ID | 54105889.7010804@suse.de |
|---|---|
| State | New |
| Headers | show |
> -----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? Thanks, Bogdan P. > > Alex > > diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c > index e3bce04..3e2cd18 100644 > --- a/hw/intc/openpic_kvm.c > +++ b/hw/intc/openpic_kvm.c > @@ -45,6 +45,7 @@ typedef struct KVMOpenPICState { > MemoryListener mem_listener; > uint32_t fd; > uint32_t model; > + hwaddr mapped; > } KVMOpenPICState; > > static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level) > @@ -128,7 +129,16 @@ static void kvm_openpic_region_add(MemoryListener > *listener, > return; > } > > + if (opp->mapped) { > + /* > + * We can only map the MPIC once. Since we are already mapped, > + * the best we can do is ignore new maps. > + */ > + return; > + } > + > reg_base = section->offset_within_address_space; > + opp->mapped = reg_base; > > attr.group = KVM_DEV_MPIC_GRP_MISC; > attr.attr = KVM_DEV_MPIC_BASE_ADDR; > @@ -155,6 +165,15 @@ static void kvm_openpic_region_del(MemoryListener > *listener, > return; > } > > + if (section->offset_within_address_space != opp->mapped) { > + /* > + * We can only map the MPIC once. This mapping was a secondary > + * one that we couldn't fulfill. Ignore it. > + */ > + return; > + } > + opp->mapped = 0; > + > attr.group = KVM_DEV_MPIC_GRP_MISC; > attr.attr = KVM_DEV_MPIC_BASE_ADDR; > attr.addr = (uint64_t)(unsigned long)®_base;
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
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c index e3bce04..3e2cd18 100644 --- a/hw/intc/openpic_kvm.c +++ b/hw/intc/openpic_kvm.c @@ -45,6 +45,7 @@ typedef struct KVMOpenPICState { MemoryListener mem_listener; uint32_t fd; uint32_t model; + hwaddr mapped; } KVMOpenPICState; static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level) @@ -128,7 +129,16 @@ static void kvm_openpic_region_add(MemoryListener *listener, return; } + if (opp->mapped) { + /* + * We can only map the MPIC once. Since we are already mapped, + * the best we can do is ignore new maps. + */ + return; + } + reg_base = section->offset_within_address_space; + opp->mapped = reg_base; attr.group = KVM_DEV_MPIC_GRP_MISC;