> -----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?
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;
