[Qemu-devel] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[Qemu-devel] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[Bogdan Purcareata]

On target-ppc, the kvm-openpic memory region is part of the E500-CCSR memory
region. On the kernel side, the MPIC is mapped at the same offset as the
kvm-openpic within the address space.

When adding the PCI BAR0 memory region, an alias is created to point to the
E500-CCSR memory region. This results in firing the kvm_openpic_region_add once
more, since kvm-openpic is part of the latter. Only this time, the offset is
wrong - it's part of the PCI memory region. This leads to the in-kernel MPIC to
be remapped at a wrong address, and thus all traps to the kvm-openpic
address to be emulated in userspace.

The fix consists in an additional filter in kvm_openpic_region_{add,del} to
consider only addresses matching the start of the kvm-openpic memory region.

[Qemu-devel] [PATCH 1/2] memory: Add MemoryRegion get address space offset helper function

[Bogdan Purcareata]

Adding this function would allow a MemoryRegion to compute its start address
within the AddressSpace. This is done recursively based on mr->container.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>
---
 include/exec/memory.h |    8 ++++++++
 memory.c              |   10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index d165b27..7503819 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -444,6 +444,14 @@ struct Object *memory_region_owner(MemoryRegion *mr);
 uint64_t memory_region_size(MemoryRegion *mr);
 
 /**
+ * memory_region_get_address_space_offset: get a memory region's address
+ * within the address space
+ *
+ * @mr: the memory region being queried.
+ */
+hwaddr memory_region_get_address_space_offset(MemoryRegion *mr);
+
+/**
  * memory_region_is_ram: check whether a memory region is random access
  *
  * Returns %true is a memory region is random access.
diff --git a/memory.c b/memory.c
index ef0be1c..7445032 100644
--- a/memory.c
+++ b/memory.c
@@ -1307,6 +1307,16 @@ uint64_t memory_region_size(MemoryRegion *mr)
     return int128_get64(mr->size);
 }
 
+hwaddr memory_region_get_address_space_offset(MemoryRegion *mr)
+{
+    MemoryRegion *p;
+    hwaddr result = 0x0;
+
+    for (p = mr; p != NULL; result += p->addr, p = p->container);
+
+    return result;
+}
+
 const char *memory_region_name(const MemoryRegion *mr)
 {
     if (!mr->name) {
-- 
1.7.10.4

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

[Bogdan Purcareata]

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.

The fix consists in an additional filter in kvm_openpic_region_{add,del} to
consider only addresses matching the start of the kvm-openpic memory region.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>
Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 hw/intc/openpic_kvm.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index e3bce04..45d8736 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -124,7 +124,9 @@ static void kvm_openpic_region_add(MemoryListener *listener,
     }
 
     /* Ignore events on regions that are not us */
-    if (section->mr != &opp->mem) {
+    if (section->mr != &opp->mem ||
+            section->offset_within_address_space !=
+            memory_region_address_space_offset(section->mr)) {
         return;
     }
 
@@ -151,7 +153,9 @@ static void kvm_openpic_region_del(MemoryListener *listener,
     int ret;
 
     /* Ignore events on regions that are not us */
-    if (section->mr != &opp->mem) {
+    if (section->mr != &opp->mem ||
+            section->offset_within_address_space !=
+            memory_region_address_space_offset(section->mr)) {
         return;
     }
 
-- 
1.7.10.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[Alexander Graf]

On 03.09.14 20:36, Bogdan Purcareata wrote:
> On target-ppc, the kvm-openpic memory region is part of the E500-CCSR memory
> region. On the kernel side, the MPIC is mapped at the same offset as the
> kvm-openpic within the address space.
> 
> When adding the PCI BAR0 memory region, an alias is created to point to the
> E500-CCSR memory region. This results in firing the kvm_openpic_region_add once
> more, since kvm-openpic is part of the latter. Only this time, the offset is
> wrong - it's part of the PCI memory region. This leads to the in-kernel MPIC to
> be remapped at a wrong address, and thus all traps to the kvm-openpic
> address to be emulated in userspace.
> 
> The fix consists in an additional filter in kvm_openpic_region_{add,del} to
> consider only addresses matching the start of the kvm-openpic memory region.

If this is true, wouldn't vhost and vfio be broken too?


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[Alexander Graf]

On 03.09.14 20:36, Bogdan Purcareata wrote:
> On target-ppc, the kvm-openpic memory region is part of the E500-CCSR memory
> region. On the kernel side, the MPIC is mapped at the same offset as the
> kvm-openpic within the address space.
> 
> When adding the PCI BAR0 memory region, an alias is created to point to the
> E500-CCSR memory region. This results in firing the kvm_openpic_region_add once
> more, since kvm-openpic is part of the latter. Only this time, the offset is
> wrong - it's part of the PCI memory region. This leads to the in-kernel MPIC to
> be remapped at a wrong address, and thus all traps to the kvm-openpic
> address to be emulated in userspace.
> 
> The fix consists in an additional filter in kvm_openpic_region_{add,del} to
> consider only addresses matching the start of the kvm-openpic memory region.

If this is true, wouldn't vfio and host be broken too?


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[mihai.caraman]

> -----Original Message-----
> From: qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org
> [mailto:qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org] On
> Behalf Of Alexander Graf
> Sent: Friday, September 05, 2014 12:08 PM
> To: Purcareata Bogdan-B43198; qemu-ppc@nongnu.org
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect
> remapping of in-kernel MPIC
> 
> 
> 
> On 03.09.14 20:36, Bogdan Purcareata wrote:
> > On target-ppc, the kvm-openpic memory region is part of the E500-CCSR
> memory
> > region. On the kernel side, the MPIC is mapped at the same offset as
> the
> > kvm-openpic within the address space.
> >
> > When adding the PCI BAR0 memory region, an alias is created to point to
> the
> > E500-CCSR memory region. This results in firing the
> kvm_openpic_region_add once
> > more, since kvm-openpic is part of the latter. Only this time, the
> offset is
> > wrong - it's part of the PCI memory region. This leads to the in-kernel
> MPIC to
> > be remapped at a wrong address, and thus all traps to the kvm-openpic
> > address to be emulated in userspace.
> >
> > The fix consists in an additional filter in
> kvm_openpic_region_{add,del} to
> > consider only addresses matching the start of the kvm-openpic memory
> region.
> 
> If this is true, wouldn't vfio and host be broken too?

You should have put the same question for 87d8354d "PPC: openpic_kvm: Filter
memory events properly". I think vhost and vfio (except for peer to peer PCI)
use region_add memory listener because they need to access the _RAM_ memory
for DMA, so they skip BAR notifications (at least in FSL SDK version of qemu).
Openpic on the other hand uses region_add as a trigger for KVM_SET_DEVICE_ATTR
ioctl (the device base address) so it takes into account non-RAM memory regions.

Vhost uses another memory listener, eventfd_add that follows a slightly
different path then region_add, as a trigger to call KVM_IOEVENTFD ioctl.
Though vhost seems to work properly we can further trace the ioctl to double
check.

Peer to peer PCI might reveal the issue on vfio but this feature is not
supported by the current FSL PAMU driver. If you think of another platform
which supports peer to peer PCI and registers a memory region alias like
this patch do 3eddc1be "Adding BAR0 for e500 PCI controller", then it worth
validating it.

I see that vfio_listener_skipped_section() changed upstream so vfio may not
skip BAR notifications anymore. What qemu version are you using on FSL boards
like T424QDS, are you using top of the tree? If it works we would like to try
it to validate vfio and vhost.

-Mike

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[mihai.caraman]

> -----Original Message-----
> From: qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org
> [mailto:qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org] On
> Behalf Of Alexander Graf
> Sent: Friday, September 05, 2014 12:08 PM
> To: Purcareata Bogdan-B43198; qemu-ppc@nongnu.org
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect
> remapping of in-kernel MPIC
> 
> 
> 
> On 03.09.14 20:36, Bogdan Purcareata wrote:
> > On target-ppc, the kvm-openpic memory region is part of the E500-CCSR
> memory
> > region. On the kernel side, the MPIC is mapped at the same offset as
> the
> > kvm-openpic within the address space.
> >
> > When adding the PCI BAR0 memory region, an alias is created to point to
> the
> > E500-CCSR memory region. This results in firing the
> kvm_openpic_region_add once
> > more, since kvm-openpic is part of the latter. Only this time, the
> offset is
> > wrong - it's part of the PCI memory region. This leads to the in-kernel
> MPIC to
> > be remapped at a wrong address, and thus all traps to the kvm-openpic
> > address to be emulated in userspace.
> >
> > The fix consists in an additional filter in
> kvm_openpic_region_{add,del} to
> > consider only addresses matching the start of the kvm-openpic memory
> region.
> 
> If this is true, wouldn't vfio and host be broken too?

You should have put the same question for 87d8354d "PPC: openpic_kvm: Filter
memory events properly". I think vhost and vfio (except for peer to peer PCI)
use region_add memory listener because they need to access the _RAM_ memory
for DMA, so they skip BAR notifications (at least in FSL SDK version of qemu).
Openpic on the other hand uses region_add as a trigger for KVM_SET_DEVICE_ATTR
ioctl (the device base address) so it takes into account non-RAM memory regions.

Vhost uses another memory listener, eventfd_add that follows a slightly
different path then region_add, as a trigger to call KVM_IOEVENTFD ioctl.
Though vhost seems to work properly we can further trace the ioctl to double
check.

Peer to peer PCI might reveal the issue on vfio but this feature is not
supported by the current FSL PAMU driver. If you think of another platform
which supports peer to peer PCI and registers a memory region alias like
this patch do 3eddc1be "Adding BAR0 for e500 PCI controller", then it worth
validating it.

I see that vfio_listener_skipped_section() changed upstream so vfio may not
skip BAR notifications anymore. What qemu version are you using on FSL boards
like T424QDS, are you using top of the tree? If it works we would like to try
it to validate vfio and vhost.

-Mike

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] memory: Add MemoryRegion get address space offset helper function

[Scott Wood]

On Wed, 2014-09-03 at 14:36 -0400, Bogdan Purcareata wrote:
> Adding this function would allow a MemoryRegion to compute its start address
> within the AddressSpace. This is done recursively based on mr->container.
> 
> Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>
> ---
>  include/exec/memory.h |    8 ++++++++
>  memory.c              |   10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d165b27..7503819 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -444,6 +444,14 @@ struct Object *memory_region_owner(MemoryRegion *mr);
>  uint64_t memory_region_size(MemoryRegion *mr);
>  
>  /**
> + * memory_region_get_address_space_offset: get a memory region's address
> + * within the address space
> + *
> + * @mr: the memory region being queried.
> + */
> +hwaddr memory_region_get_address_space_offset(MemoryRegion *mr);

Hmm, this seems familiar:
http://patchwork.ozlabs.org/patch/220385/
http://patchwork.ozlabs.org/patch/236764/

:-)

> +
> +/**
>   * memory_region_is_ram: check whether a memory region is random access
>   *
>   * Returns %true is a memory region is random access.
> diff --git a/memory.c b/memory.c
> index ef0be1c..7445032 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1307,6 +1307,16 @@ uint64_t memory_region_size(MemoryRegion *mr)
>      return int128_get64(mr->size);
>  }
>  
> +hwaddr memory_region_get_address_space_offset(MemoryRegion *mr)
> +{
> +    MemoryRegion *p;
> +    hwaddr result = 0x0;
> +
> +    for (p = mr; p != NULL; result += p->addr, p = p->container);
> +
> +    return result;
> +}

This would be more readable as:

	while (mr) {
		result += mr->addr;
		mr = mr->container;
	}

-Scott

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

[Scott Wood]

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.

-Scott

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

[bogdan.purcareata]

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

[1] http://git.qemu.org/?p=qemu.git;a=blob;f=docs/memory.txt

Thanks,
Bogdan P.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect remapping of in-kernel MPIC

[Alexander Graf]

On 05.09.14 16:31, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org
>> [mailto:qemu-devel-bounces+mihai.caraman=freescale.com@nongnu.org] On
>> Behalf Of Alexander Graf
>> Sent: Friday, September 05, 2014 12:08 PM
>> To: Purcareata Bogdan-B43198; qemu-ppc@nongnu.org
>> Cc: qemu-devel@nongnu.org
>> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] PPC: kvm: Fix incorrect
>> remapping of in-kernel MPIC
>>
>>
>>
>> On 03.09.14 20:36, Bogdan Purcareata wrote:
>>> On target-ppc, the kvm-openpic memory region is part of the E500-CCSR
>> memory
>>> region. On the kernel side, the MPIC is mapped at the same offset as
>> the
>>> kvm-openpic within the address space.
>>>
>>> When adding the PCI BAR0 memory region, an alias is created to point to
>> the
>>> E500-CCSR memory region. This results in firing the
>> kvm_openpic_region_add once
>>> more, since kvm-openpic is part of the latter. Only this time, the
>> offset is
>>> wrong - it's part of the PCI memory region. This leads to the in-kernel
>> MPIC to
>>> be remapped at a wrong address, and thus all traps to the kvm-openpic
>>> address to be emulated in userspace.
>>>
>>> The fix consists in an additional filter in
>> kvm_openpic_region_{add,del} to
>>> consider only addresses matching the start of the kvm-openpic memory
>> region.
>>
>> If this is true, wouldn't vfio and host be broken too?
> 
> You should have put the same question for 87d8354d "PPC: openpic_kvm: Filter
> memory events properly". I think vhost and vfio (except for peer to peer PCI)
> use region_add memory listener because they need to access the _RAM_ memory
> for DMA, so they skip BAR notifications (at least in FSL SDK version of qemu).
> Openpic on the other hand uses region_add as a trigger for KVM_SET_DEVICE_ATTR
> ioctl (the device base address) so it takes into account non-RAM memory regions.
> 
> Vhost uses another memory listener, eventfd_add that follows a slightly
> different path then region_add, as a trigger to call KVM_IOEVENTFD ioctl.
> Though vhost seems to work properly we can further trace the ioctl to double
> check.
> 
> Peer to peer PCI might reveal the issue on vfio but this feature is not
> supported by the current FSL PAMU driver. If you think of another platform
> which supports peer to peer PCI and registers a memory region alias like
> this patch do 3eddc1be "Adding BAR0 for e500 PCI controller", then it worth
> validating it.
> 
> I see that vfio_listener_skipped_section() changed upstream so vfio may not
> skip BAR notifications anymore. What qemu version are you using on FSL boards
> like T424QDS, are you using top of the tree? If it works we would like to try
> it to validate vfio and vhost.

Yes I'm running pure upstream code. However, I haven't tried to use VFIO
on e500 at all yet.


Alex

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

[Alexander Graf]

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;

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

[bogdan.purcareata]

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

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

[Alexander Graf]

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