[Qemu-devel] [PATCH v4 0/3] pci: implement upstream master abort protocol

[Qemu-devel] [PATCH v4 0/3] pci: implement upstream master abort protocol

[Marcel Apfelbaum]

PCI spec requires that a transaction that has not been claimed
by any PCI bus devices will be terminated by the initiator
with "master abort". For read transactions -1(FFFFFFFF) is returned and 
writes are silently dropped.

Implementation:
 - Allowed the MemoryRegion priority to be negative so a subregion will be
   visible on all the addresses not covered by other container subregions.
 - Added a memory region with negative priority that extends over all the
   pci address space. This region catches all the accesses
   to the unassigned pci addresses.
 - The MemoryRegion's ops emulates the master abort scenario.

Note: The code handles only the reads/writes to pci address space
that are done by the cpu.

I am working on implementing the following on top of this series
 - Implement upstream master abort
 - Handling of RECEIVED MASTER ABORT BIT in Stastus register

Changes from v3:
 - Addressed Peter Maydell comments
   - Removed unnecessary changes to priority of MemoryListener
   - Ensured that priority is now signed in all related places
   - Added to memory docs explanation on signed priorities
 - Addresses Michael S. Tsirkin comments
   - Changed the name of the new Memory region to master_abort_mem
   - Made master abort priority INT_MIN instead of -1
 - Removed handling of RECEIVED MASTER ABORT BIT; it will be taken
   care in a different series

Changes from v2:
 - minor: changed nr of patches in the title
 - minor: modified series list

Changes from v1:
 - "pci-unassigned-mem" MemoryRegion resides now in PCIBus and not on
    various Host Bridges
 - "pci-unassgined-mem" does not have a ".valid.accept" field and
    implements read write methods


Marcel Apfelbaum (3):
  memory: allow MemoryRegion's priority field to accept negative values
  docs/memory: Explictly state that MemoryRegion priority is signed
  hw/pci: partially handle pci master abort

 docs/memory.txt          |  4 ++++
 hw/core/sysbus.c         |  4 ++--
 hw/pci/pci.c             | 27 +++++++++++++++++++++++++++
 include/exec/memory.h    |  4 ++--
 include/hw/pci/pci_bus.h |  1 +
 include/hw/sysbus.h      |  2 +-
 memory.c                 |  4 ++--
 7 files changed, 39 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Marcel Apfelbaum]

Priority is used to make visible some subregions by obscuring
the parent MemoryRegion addresses overlapping with the subregion.

By allowing the priority to be negative the opposite can be done:
Allow a subregion to be visible on all the addresses not covered
by the parent MemoryRegion or other subregions.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
Changes from v3:
 - Addressed Peter Maydell comments
   - Removed unnecessary changes to priority of MemoryListener
   - Ensured that priority is now signed in all related places
 
 hw/core/sysbus.c      | 4 ++--
 include/exec/memory.h | 4 ++--
 include/hw/sysbus.h   | 2 +-
 memory.c              | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index b84cd4a..146f50a 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -49,7 +49,7 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 }
 
 static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
-                                   bool may_overlap, unsigned priority)
+                                   bool may_overlap, int priority)
 {
     assert(n >= 0 && n < dev->num_mmio);
 
@@ -81,7 +81,7 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)
 }
 
 void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
-                             unsigned priority)
+                             int priority)
 {
     sysbus_mmio_map_common(dev, n, addr, true, priority);
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ebe0d24..480dfbf 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -153,7 +153,7 @@ struct MemoryRegion {
     bool flush_coalesced_mmio;
     MemoryRegion *alias;
     hwaddr alias_offset;
-    unsigned priority;
+    int priority;
     bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;
@@ -779,7 +779,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
 void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          hwaddr offset,
                                          MemoryRegion *subregion,
-                                         unsigned priority);
+                                         int priority);
 
 /**
  * memory_region_get_ram_addr: Get the ram address associated with a memory
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index bb50a87..f5aaa05 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -68,7 +68,7 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size);
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
 void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
 void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
-                             unsigned priority);
+                             int priority);
 void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
 void sysbus_del_io(SysBusDevice *dev, MemoryRegion *mem);
diff --git a/memory.c b/memory.c
index 5a10fd0..f49d31a 100644
--- a/memory.c
+++ b/memory.c
@@ -1473,7 +1473,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
 void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          hwaddr offset,
                                          MemoryRegion *subregion,
-                                         unsigned priority)
+                                         int priority)
 {
     subregion->may_overlap = true;
     subregion->priority = priority;
@@ -1506,7 +1506,7 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
 {
     MemoryRegion *parent = mr->parent;
-    unsigned priority = mr->priority;
+    int priority = mr->priority;
     bool may_overlap = mr->may_overlap;
 
     if (addr == mr->addr || !parent) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Michael S. Tsirkin]

On Sun, Sep 15, 2013 at 07:16:39PM +0300, Marcel Apfelbaum wrote:
> Priority is used to make visible some subregions by obscuring
> the parent MemoryRegion addresses overlapping with the subregion.
> 
> By allowing the priority to be negative the opposite can be done:
> Allow a subregion to be visible on all the addresses not covered
> by the parent MemoryRegion or other subregions.


This commit log is confusing: priorities are always local.
It should say something like this instead:

When regions overlap, priority can be used to specify
that a given region should obscure others.

Allow the priority to be negative, so tha the opposite
can be done: specify that other regions should obscure
a given one.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> Changes from v3:
>  - Addressed Peter Maydell comments
>    - Removed unnecessary changes to priority of MemoryListener
>    - Ensured that priority is now signed in all related places
>  
>  hw/core/sysbus.c      | 4 ++--
>  include/exec/memory.h | 4 ++--
>  include/hw/sysbus.h   | 2 +-
>  memory.c              | 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index b84cd4a..146f50a 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -49,7 +49,7 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
>  }
>  
>  static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
> -                                   bool may_overlap, unsigned priority)
> +                                   bool may_overlap, int priority)
>  {
>      assert(n >= 0 && n < dev->num_mmio);
>  
> @@ -81,7 +81,7 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)
>  }
>  
>  void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
> -                             unsigned priority)
> +                             int priority)
>  {
>      sysbus_mmio_map_common(dev, n, addr, true, priority);
>  }
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ebe0d24..480dfbf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -153,7 +153,7 @@ struct MemoryRegion {
>      bool flush_coalesced_mmio;
>      MemoryRegion *alias;
>      hwaddr alias_offset;
> -    unsigned priority;
> +    int priority;
>      bool may_overlap;
>      QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>      QTAILQ_ENTRY(MemoryRegion) subregions_link;
> @@ -779,7 +779,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           hwaddr offset,
>                                           MemoryRegion *subregion,
> -                                         unsigned priority);
> +                                         int priority);
>  
>  /**
>   * memory_region_get_ram_addr: Get the ram address associated with a memory
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index bb50a87..f5aaa05 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -68,7 +68,7 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size);
>  void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
>  void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
>  void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
> -                             unsigned priority);
> +                             int priority);
>  void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
>                     MemoryRegion *mem);
>  void sysbus_del_io(SysBusDevice *dev, MemoryRegion *mem);
> diff --git a/memory.c b/memory.c
> index 5a10fd0..f49d31a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1473,7 +1473,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           hwaddr offset,
>                                           MemoryRegion *subregion,
> -                                         unsigned priority)
> +                                         int priority)
>  {
>      subregion->may_overlap = true;
>      subregion->priority = priority;
> @@ -1506,7 +1506,7 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
>  void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
>  {
>      MemoryRegion *parent = mr->parent;
> -    unsigned priority = mr->priority;
> +    int priority = mr->priority;
>      bool may_overlap = mr->may_overlap;
>  
>      if (addr == mr->addr || !parent) {
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Peter Maydell]

On 15 September 2013 17:16, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> Priority is used to make visible some subregions by obscuring
> the parent MemoryRegion addresses overlapping with the subregion.
>
> By allowing the priority to be negative the opposite can be done:
> Allow a subregion to be visible on all the addresses not covered
> by the parent MemoryRegion or other subregions.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Codewise this looks good, so:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

If you have to do a v5 for some reason it would be nice to:
1. update docs/memory.txt to say that priority is a signed value.
Here's my suggested text from my comments on v2 of your patch
(for adding to the 'Overlapping regions and priority' section):
====begin====
Priority values are signed, and the default value is zero. This means
that you can use memory_region_add_subregion_overlap() both to
specify a region that must sit 'above' any others (with a positive priority)
and also a background region that sits 'below' others (with a negative
priority).
====endit====

2. improve the commit message so it doesn't imply that this
  is allowing us to do something previously impossible.
(There is no difference between having a region at priority
-1 and five at priority 0, versus one region at priority 0 and
five at priority 1 -- it's merely more convenient to be able
to create the five regions at default priority (0) and have one
special case for the background region, rather than having
to specify the priority explicitly for the five others.)

I would suggest as a commit message:

===begin===
memory: Change MemoryRegion priorities from unsigned to signed

When memory regions overlap, priority can be used to specify
which of them takes priority. By making the priority values signed
rather than unsigned, we make it more convenient to implement
a situation where one "background" region should appear only
where no other region exists: rather than having to explicitly
specify a high priority for all the other regions, we can let them take
the default (zero) priority and specify a negative priority for the
background region.
===endit===

(I have read MST's followup suggested improved commit message
and borrowed bits of it for this.)

-- PMM

Re: [Qemu-devel] [PATCH v4 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Peter Maydell]

On 15 September 2013 18:25, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 September 2013 17:16, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>> Priority is used to make visible some subregions by obscuring
>> the parent MemoryRegion addresses overlapping with the subregion.

> If you have to do a v5 for some reason it would be nice to:
> 1. update docs/memory.txt to say that priority is a signed value.

Oh, I see you put it in a separate patch. That's OK too I guess.

-- PMM

[Qemu-devel] [PATCH v4 2/3] docs/memory: Explicitly state that MemoryRegion priority is signed

[Marcel Apfelbaum]

Priority was used to make visible some subregions by obscuring
the parent MemoryRegion addresses overlapping with the subregion.

By allowing the priority to be negative the opposite can be done:
Allow a subregion to be visible on all the addresses not covered
by other subregions.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 docs/memory.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/memory.txt b/docs/memory.txt
index feb9fe9..174c0d7 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -80,6 +80,10 @@ guest.  This is done with memory_region_add_subregion_overlap(), which
 allows the region to overlap any other region in the same container, and
 specifies a priority that allows the core to decide which of two regions at
 the same address are visible (highest wins).
+Priority values are signed, and the default value is zero. This means that
+you can use memory_region_add_subregion_overlap() both to specify a region
+that must sit 'above' any others (with a positive priority) and also a
+background region that sits 'below' others (with a negative priority).
 
 Visibility
 ----------
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 2/3] docs/memory: Explicitly state that MemoryRegion priority is signed

[Peter Maydell]

On 15 September 2013 17:16, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> Priority was used to make visible some subregions by obscuring
> the parent MemoryRegion addresses overlapping with the subregion.
>
> By allowing the priority to be negative the opposite can be done:
> Allow a subregion to be visible on all the addresses not covered
> by other subregions.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(but then I would since I wrote the text initially :-))

Remarks about the commit message in 1/3 apply here too I guess.

-- PMM

[Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Marcel Apfelbaum]

A MemoryRegion with negative priority was created and
it spans over all the pci address space.
It "intercepts" the accesses to unassigned pci
address space and will follow the pci spec:
 1. returns -1 on read
 2. does nothing on write

Note: setting the RECEIVED MASTER ABORT bit in the STATUS register
      of the device that initiated the transaction will be
      implemented in another series
Note: This implementation handles only the reads/writes to
      the pci address space that are done by the cpu.(downstream)

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
Changes from v3:
 - Addresses Michael S. Tsirkin comments
   - Changed the name of the new Memory region to master_abort_mem
   - Made master abort priority INT_MIN instead of -1
 - Removed handling of RECEIVED MASTER ABORT BIT; it will be taken
   care in a different series

Changes from v1:
 - "pci-unassigned-mem" MemoryRegion resides now in PCIBus and not on
    various Host Bridges
 - "pci-unassgined-mem" does not have a ".valid.accept" field and
    implements read write methods

 hw/pci/pci.c             | 27 +++++++++++++++++++++++++++
 include/hw/pci/pci_bus.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d00682e..9b12375 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -283,6 +283,24 @@ const char *pci_root_bus_path(PCIDevice *dev)
     return rootbus->qbus.name;
 }
 
+static uint64_t master_abort_mem_read(void *opaque, hwaddr addr, unsigned size)
+{
+   return -1ULL;
+}
+
+static void master_abort_mem_write(void *opaque, hwaddr addr, uint64_t val,
+                                   unsigned size)
+{
+}
+
+static const MemoryRegionOps master_abort_mem_ops = {
+    .read = master_abort_mem_read,
+    .write = master_abort_mem_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+#define MASTER_ABORT_MEM_PRIORITY INT_MIN
+
 static void pci_bus_init(PCIBus *bus, DeviceState *parent,
                          const char *name,
                          MemoryRegion *address_space_mem,
@@ -294,6 +312,15 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
 
+
+    memory_region_init_io(&bus->master_abort_mem, OBJECT(bus),
+                          &master_abort_mem_ops, bus, "pci-master-abort",
+                          memory_region_size(bus->address_space_mem));
+    memory_region_add_subregion_overlap(bus->address_space_mem,
+                                        bus->address_space_mem->addr,
+                                        &bus->master_abort_mem,
+                                        MASTER_ABORT_MEM_PRIORITY);
+
     /* host bridge */
     QLIST_INIT(&bus->child);
 
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 9df1788..2ad5edb 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -23,6 +23,7 @@ struct PCIBus {
     PCIDevice *parent_dev;
     MemoryRegion *address_space_mem;
     MemoryRegion *address_space_io;
+    MemoryRegion master_abort_mem;
 
     QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
     QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Michael S. Tsirkin]

On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
> A MemoryRegion with negative priority was created and
> it spans over all the pci address space.
> It "intercepts" the accesses to unassigned pci
> address space and will follow the pci spec:
>  1. returns -1 on read
>  2. does nothing on write
> 
> Note: setting the RECEIVED MASTER ABORT bit in the STATUS register
>       of the device that initiated the transaction will be
>       implemented in another series

Fine though I'd like to see how it all works
together before applying.

> Note: This implementation handles only the reads/writes to
>       the pci address space that are done by the cpu.(downstream)

Strange, I don't see where does the limitation come from.
Looks like any read returns -1 - what did I miss.

> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> Changes from v3:
>  - Addresses Michael S. Tsirkin comments
>    - Changed the name of the new Memory region to master_abort_mem
>    - Made master abort priority INT_MIN instead of -1
>  - Removed handling of RECEIVED MASTER ABORT BIT; it will be taken
>    care in a different series
> 
> Changes from v1:
>  - "pci-unassigned-mem" MemoryRegion resides now in PCIBus and not on
>     various Host Bridges
>  - "pci-unassgined-mem" does not have a ".valid.accept" field and
>     implements read write methods
> 
>  hw/pci/pci.c             | 27 +++++++++++++++++++++++++++
>  include/hw/pci/pci_bus.h |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index d00682e..9b12375 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -283,6 +283,24 @@ const char *pci_root_bus_path(PCIDevice *dev)
>      return rootbus->qbus.name;
>  }
>  
> +static uint64_t master_abort_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +   return -1ULL;
> +}
> +
> +static void master_abort_mem_write(void *opaque, hwaddr addr, uint64_t val,
> +                                   unsigned size)
> +{
> +}
> +
> +static const MemoryRegionOps master_abort_mem_ops = {
> +    .read = master_abort_mem_read,
> +    .write = master_abort_mem_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +

Please make it little endian.
DEVICE_NATIVE_ENDIAN is almost always a bug.

> +#define MASTER_ABORT_MEM_PRIORITY INT_MIN
> +
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                           const char *name,
>                           MemoryRegion *address_space_mem,
> @@ -294,6 +312,15 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
>  
> +
> +    memory_region_init_io(&bus->master_abort_mem, OBJECT(bus),
> +                          &master_abort_mem_ops, bus, "pci-master-abort",
> +                          memory_region_size(bus->address_space_mem));
> +    memory_region_add_subregion_overlap(bus->address_space_mem,
> +                                        bus->address_space_mem->addr,
> +                                        &bus->master_abort_mem,
> +                                        MASTER_ABORT_MEM_PRIORITY);
> +
>      /* host bridge */
>      QLIST_INIT(&bus->child);
>  
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 9df1788..2ad5edb 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -23,6 +23,7 @@ struct PCIBus {
>      PCIDevice *parent_dev;
>      MemoryRegion *address_space_mem;
>      MemoryRegion *address_space_io;
> +    MemoryRegion master_abort_mem;
>  
>      QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
>      QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Peter Maydell]

On 15 September 2013 18:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
>> +static const MemoryRegionOps master_abort_mem_ops = {
>> +    .read = master_abort_mem_read,
>> +    .write = master_abort_mem_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>
> Please make it little endian.
> DEVICE_NATIVE_ENDIAN is almost always a bug.

...when dealing with PCI devices. For a random device on the system bus
it's often correct.

-- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Marcel Apfelbaum]

On Sun, 2013-09-15 at 18:32 +0100, Peter Maydell wrote:
> On 15 September 2013 18:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
> >> +static const MemoryRegionOps master_abort_mem_ops = {
> >> +    .read = master_abort_mem_read,
> >> +    .write = master_abort_mem_write,
> >> +    .endianness = DEVICE_NATIVE_ENDIAN,
> >> +};
> >> +
> >
> > Please make it little endian.
> > DEVICE_NATIVE_ENDIAN is almost always a bug.
> 
> ...when dealing with PCI devices. For a random device on the system bus
> it's often correct.
But this is a PCI bus, I'll change it to little endian.
Thanks!
Marcel

> 
> -- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Michael S. Tsirkin]

On Sun, Sep 15, 2013 at 06:32:13PM +0100, Peter Maydell wrote:
> On 15 September 2013 18:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
> >> +static const MemoryRegionOps master_abort_mem_ops = {
> >> +    .read = master_abort_mem_read,
> >> +    .write = master_abort_mem_write,
> >> +    .endianness = DEVICE_NATIVE_ENDIAN,
> >> +};
> >> +
> >
> > Please make it little endian.
> > DEVICE_NATIVE_ENDIAN is almost always a bug.
> 
> ...when dealing with PCI devices. For a random device on the system bus
> it's often correct.
> 
> -- PMM

native is really qemu host endian-ness ... what are some
examples when it's actually correct?

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Peter Maydell]

On 15 September 2013 21:25, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 15, 2013 at 06:32:13PM +0100, Peter Maydell wrote:
>> On 15 September 2013 18:30, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
>> >> +static const MemoryRegionOps master_abort_mem_ops = {
>> >> +    .read = master_abort_mem_read,
>> >> +    .write = master_abort_mem_write,
>> >> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> >> +};
>> >> +
>> >
>> > Please make it little endian.
>> > DEVICE_NATIVE_ENDIAN is almost always a bug.
>>
>> ...when dealing with PCI devices. For a random device on the system bus
>> it's often correct.

> native is really qemu host endian-ness ... what are some
> examples when it's actually correct?

"native" means "if the device's MMIO callback does 'return 0x12345678;'
for a 32 bit read then the guest CPU should see 0x12345678". That's
almost always what you want for simple devices (which may in fact
only support 32 bit accesses to registers), because it means you don't
have to fill your device with explicit endianness swaps. It's also useful
if that kind of simple device might be built into either a little endian
or a bigendian system: as far as the device is concerned its 32 bit
registers still have (say) the TXRDY bit at the low end of the status
register even if the CPU is bigendian.

That said, our current setup of marking the mmio ops with an endianness
is really conflating what in real hardware is a number of distinct properties
of the CPU, the bus, any bus controllers (like PCI!) in the path between
CPU and device and finally the device itself. So it's inherently both
confusing and confused.

-- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Michael S. Tsirkin]

On Sun, Sep 15, 2013 at 09:40:37PM +0100, Peter Maydell wrote:
> On 15 September 2013 21:25, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 15, 2013 at 06:32:13PM +0100, Peter Maydell wrote:
> >> On 15 September 2013 18:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
> >> >> +static const MemoryRegionOps master_abort_mem_ops = {
> >> >> +    .read = master_abort_mem_read,
> >> >> +    .write = master_abort_mem_write,
> >> >> +    .endianness = DEVICE_NATIVE_ENDIAN,
> >> >> +};
> >> >> +
> >> >
> >> > Please make it little endian.
> >> > DEVICE_NATIVE_ENDIAN is almost always a bug.
> >>
> >> ...when dealing with PCI devices. For a random device on the system bus
> >> it's often correct.
> 
> > native is really qemu host endian-ness ... what are some
> > examples when it's actually correct?
> 
> "native" means "if the device's MMIO callback does 'return 0x12345678;'
> for a 32 bit read then the guest CPU should see 0x12345678". That's
> almost always what you want for simple devices (which may in fact
> only support 32 bit accesses to registers), because it means you don't
> have to fill your device with explicit endianness swaps.

But this means that you device behaves differently
depending on the endian-ness of the guest system.
So it only makes sense if the device is very
system specific: anything outside hw/<specific architecture>
is at least in theory not a system specific device so
it should not do this.

> It's also useful
> if that kind of simple device might be built into either a little endian
> or a bigendian system: as far as the device is concerned its 32 bit
> registers still have (say) the TXRDY bit at the low end of the status
> register even if the CPU is bigendian.
> 
> That said, our current setup of marking the mmio ops with an endianness
> is really conflating what in real hardware is a number of distinct properties
> of the CPU, the bus, any bus controllers (like PCI!) in the path between
> CPU and device and finally the device itself. So it's inherently both
> confusing and confused.
> 
> -- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Peter Maydell]

On 15 September 2013 22:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 15, 2013 at 09:40:37PM +0100, Peter Maydell wrote:
>> "native" means "if the device's MMIO callback does 'return 0x12345678;'
>> for a 32 bit read then the guest CPU should see 0x12345678". That's
>> almost always what you want for simple devices (which may in fact
>> only support 32 bit accesses to registers), because it means you don't
>> have to fill your device with explicit endianness swaps.
>
> But this means that you device behaves differently
> depending on the endian-ness of the guest system.
> So it only makes sense if the device is very
> system specific

If you mark a device as specifically DEVICE_LITTLE_ENDIAN
or DEVICE_BIG_ENDIAN this is *also* very system specific.
So you're a bit stuck either way. As I say, for basic "this just
provides a bunch of registers" devices _NATIVE_ is the
pragmatic answer, since it effectively models the way that the
same bit of hardware is wired up to the bus differently if it's
expected to be in a big or little endian system.
(Any device where you can make byte accesses into the "middle"
of a 32 bit register probably needs to think more carefully, but those
are pretty rare.)

>anything outside hw/<specific architecture>
> is at least in theory not a system specific device

This is wrong, by the way. hw/$arch contains:
 * board models
 * things we haven't properly separated out into self contained devices
 * random "not actually a device" things like boot code

Anything that's really a device goes in its appropriate subdirectory
(char, video, etc etc), whether it happens to be used only on one
system or one architecture or not. (For instance all the interrupt
controllers live in hw/intc though obviously they're hopelessly
system specific.)

-- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Michael S. Tsirkin]

On Sun, Sep 15, 2013 at 10:41:26PM +0100, Peter Maydell wrote:
> On 15 September 2013 22:07, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 15, 2013 at 09:40:37PM +0100, Peter Maydell wrote:
> >> "native" means "if the device's MMIO callback does 'return 0x12345678;'
> >> for a 32 bit read then the guest CPU should see 0x12345678". That's
> >> almost always what you want for simple devices (which may in fact
> >> only support 32 bit accesses to registers), because it means you don't
> >> have to fill your device with explicit endianness swaps.
> >
> > But this means that you device behaves differently
> > depending on the endian-ness of the guest system.
> > So it only makes sense if the device is very
> > system specific
> 
> If you mark a device as specifically DEVICE_LITTLE_ENDIAN
> or DEVICE_BIG_ENDIAN this is *also* very system specific.

No, this just means the device is always wired in
the same way on all systems. It's the pragmatic
choice for any bus that supports device plug-in.

> So you're a bit stuck either way. As I say, for basic "this just
> provides a bunch of registers" devices _NATIVE_ is the
> pragmatic answer, since it effectively models the way that the
> same bit of hardware is wired up to the bus differently if it's
> expected to be in a big or little endian system.
> (Any device where you can make byte accesses into the "middle"
> of a 32 bit register probably needs to think more carefully, but those
> are pretty rare.)
> 
> >anything outside hw/<specific architecture>
> > is at least in theory not a system specific device
> 
> This is wrong, by the way. hw/$arch contains:
>  * board models
>  * things we haven't properly separated out into self contained devices
>  * random "not actually a device" things like boot code
> 
> Anything that's really a device goes in its appropriate subdirectory
> (char, video, etc etc), whether it happens to be used only on one
> system or one architecture or not. (For instance all the interrupt
> controllers live in hw/intc though obviously they're hopelessly
> system specific.)
> 
> -- PMM

Thanks for the clarification.

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Peter Maydell]

On 16 September 2013 07:14, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 15, 2013 at 10:41:26PM +0100, Peter Maydell wrote:
>> On 15 September 2013 22:07, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, Sep 15, 2013 at 09:40:37PM +0100, Peter Maydell wrote:
>> >> "native" means "if the device's MMIO callback does 'return 0x12345678;'
>> >> for a 32 bit read then the guest CPU should see 0x12345678". That's
>> >> almost always what you want for simple devices (which may in fact
>> >> only support 32 bit accesses to registers), because it means you don't
>> >> have to fill your device with explicit endianness swaps.
>> >
>> > But this means that you device behaves differently
>> > depending on the endian-ness of the guest system.
>> > So it only makes sense if the device is very
>> > system specific
>>
>> If you mark a device as specifically DEVICE_LITTLE_ENDIAN
>> or DEVICE_BIG_ENDIAN this is *also* very system specific.
>
> No, this just means the device is always wired in
> the same way on all systems. It's the pragmatic
> choice for any bus that supports device plug-in.

No, it means the device is little endian even on a big
endian system. On BE systems that is weird, and the only
reason for it is if it's an external bus with a standard that
specifies which endianness it is. That's true for ISA and PCI
(and this is where we're using the device's endianness
specification to compensate for the fact we don't have a way
to specify that PCI host bridges will do endianness swapping
on a bigendian system).
But "pluggable bus" is not equivalent to "system specific"
(in either direction).

-- PMM

Re: [Qemu-devel] [PATCH v4 3/3] hw/pci: handle downstream pci master abort

[Marcel Apfelbaum]

On Sun, 2013-09-15 at 20:30 +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 15, 2013 at 07:16:41PM +0300, Marcel Apfelbaum wrote:
> > A MemoryRegion with negative priority was created and
> > it spans over all the pci address space.
> > It "intercepts" the accesses to unassigned pci
> > address space and will follow the pci spec:
> >  1. returns -1 on read
> >  2. does nothing on write
> > 
> > Note: setting the RECEIVED MASTER ABORT bit in the STATUS register
> >       of the device that initiated the transaction will be
> >       implemented in another series
> 
> Fine though I'd like to see how it all works
> together before applying.
> 
> > Note: This implementation handles only the reads/writes to
> >       the pci address space that are done by the cpu.(downstream)
> 
> Strange, I don't see where does the limitation come from.
> Looks like any read returns -1 - what did I miss.
Devices using IOMMU have a different way to get their
memory regions. I am not sure that master abort memory
region covers the scenario.
Anyway, the above statement was for the prev implementation,
I will remove it in the next version.

Thanks,
Marcel

> 
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > Changes from v3:
> >  - Addresses Michael S. Tsirkin comments
> >    - Changed the name of the new Memory region to master_abort_mem
> >    - Made master abort priority INT_MIN instead of -1
> >  - Removed handling of RECEIVED MASTER ABORT BIT; it will be taken
> >    care in a different series
> > 
> > Changes from v1:
> >  - "pci-unassigned-mem" MemoryRegion resides now in PCIBus and not on
> >     various Host Bridges
> >  - "pci-unassgined-mem" does not have a ".valid.accept" field and
> >     implements read write methods
> > 
> >  hw/pci/pci.c             | 27 +++++++++++++++++++++++++++
> >  include/hw/pci/pci_bus.h |  1 +
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index d00682e..9b12375 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -283,6 +283,24 @@ const char *pci_root_bus_path(PCIDevice *dev)
> >      return rootbus->qbus.name;
> >  }
> >  
> > +static uint64_t master_abort_mem_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +   return -1ULL;
> > +}
> > +
> > +static void master_abort_mem_write(void *opaque, hwaddr addr, uint64_t val,
> > +                                   unsigned size)
> > +{
> > +}
> > +
> > +static const MemoryRegionOps master_abort_mem_ops = {
> > +    .read = master_abort_mem_read,
> > +    .write = master_abort_mem_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> 
> Please make it little endian.
> DEVICE_NATIVE_ENDIAN is almost always a bug.
> 
> > +#define MASTER_ABORT_MEM_PRIORITY INT_MIN
> > +
> >  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> >                           const char *name,
> >                           MemoryRegion *address_space_mem,
> > @@ -294,6 +312,15 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> >      bus->address_space_mem = address_space_mem;
> >      bus->address_space_io = address_space_io;
> >  
> > +
> > +    memory_region_init_io(&bus->master_abort_mem, OBJECT(bus),
> > +                          &master_abort_mem_ops, bus, "pci-master-abort",
> > +                          memory_region_size(bus->address_space_mem));
> > +    memory_region_add_subregion_overlap(bus->address_space_mem,
> > +                                        bus->address_space_mem->addr,
> > +                                        &bus->master_abort_mem,
> > +                                        MASTER_ABORT_MEM_PRIORITY);
> > +
> >      /* host bridge */
> >      QLIST_INIT(&bus->child);
> >  
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 9df1788..2ad5edb 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -23,6 +23,7 @@ struct PCIBus {
> >      PCIDevice *parent_dev;
> >      MemoryRegion *address_space_mem;
> >      MemoryRegion *address_space_io;
> > +    MemoryRegion master_abort_mem;
> >  
> >      QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
> >      QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */
> > -- 
> > 1.8.3.1