Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks based on memory region offset

[Alexander Graf <agraf@suse.de>]

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?


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)&reg_base;