[Qemu-devel] [PATCH RFC v3 0/2] pci: complete master abort protocol

[Qemu-devel] [PATCH RFC v3 0/2] pci: complete master abort protocol

[Marcel Apfelbaum]

Note: The series is incomplete, for review only

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.

The series deals also with the other aspect of the master abort scenario:
Upon completion the master has to raise RECEIVED MASTER ABORT BIT in
initiator's STATUS register.

Implementation:
 - Allowed the MemoryRegion priority to be negative so a subregion will be
   visible on all the addresses not covered by the parent MemoryRegion
   or other 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:
For the moment the code assumes that all the reads/writes to
pci address space are done by the cpu.

Changes from v2:
 - minor: changed nr of patches int 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 (2):
  memory: allow MemoryRegion's priority field to accept negative values
  hw/pci: handle unassigned pci addresses

 hw/pci/pci.c             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/exec/memory.h    |  6 +++---
 include/hw/pci/pci_bus.h |  1 +
 memory.c                 |  2 +-
 4 files changed, 51 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC v3 1/2] 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>
---
 include/exec/memory.h | 6 +++---
 memory.c              | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index ebe0d24..6995087 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;
@@ -193,7 +193,7 @@ struct MemoryListener {
     void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection *section,
                                hwaddr addr, hwaddr len);
     /* Lower = earlier (during add), later (during del) */
-    unsigned priority;
+    int priority;
     AddressSpace *address_space_filter;
     QTAILQ_ENTRY(MemoryListener) 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/memory.c b/memory.c
index 5a10fd0..984a3dc 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;
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Mon, Sep 09, 2013 at 02:21:35PM +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.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Seems harmless enough.

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


> ---
>  include/exec/memory.h | 6 +++---
>  memory.c              | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ebe0d24..6995087 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;
> @@ -193,7 +193,7 @@ struct MemoryListener {
>      void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection *section,
>                                 hwaddr addr, hwaddr len);
>      /* Lower = earlier (during add), later (during del) */
> -    unsigned priority;
> +    int priority;
>      AddressSpace *address_space_filter;
>      QTAILQ_ENTRY(MemoryListener) 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/memory.c b/memory.c
> index 5a10fd0..984a3dc 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;
> -- 
> 1.8.3.1

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

[Peter Maydell]

On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 09, 2013 at 02:21:35PM +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.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>
> Seems harmless enough.
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

No, the idea is good but this version is just broken.
See the comments I made on the previous version which
Marcel ignored :-(

-- PMM

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

[Michael S. Tsirkin]

On Mon, Sep 09, 2013 at 12:45:12PM +0100, Peter Maydell wrote:
> On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Sep 09, 2013 at 02:21:35PM +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.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >
> > Seems harmless enough.
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> No, the idea is good but this version is just broken.
> See the comments I made on the previous version which
> Marcel ignored :-(
> 
> -- PMM

You are right, I missed the bugs.
Good catch, thanks.

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

[Michael S. Tsirkin]

On Mon, Sep 09, 2013 at 02:48:55PM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 09, 2013 at 12:45:12PM +0100, Peter Maydell wrote:
> > On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Sep 09, 2013 at 02:21:35PM +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.
> > >>
> > >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > >
> > > Seems harmless enough.
> > >
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > No, the idea is good but this version is just broken.
> > See the comments I made on the previous version which
> > Marcel ignored :-(
> > 
> > -- PMM
> 
> You are right, I missed the bugs.
> Good catch, thanks.
> 

So I guess it's only an Acked-by: Michael S. Tsirkin <mst@redhat.com>
at this point :)

[Qemu-devel] [PATCH RFC v3 2/2] hw/pci: handle unassigned pci addresses

[Marcel Apfelbaum]

Created a MemoryRegion with negative priority that
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
 3. sets the RECEIVED MASTER ABORT bit in the STATUS register
    of the device that initiated the transaction

Note: This implementation assumes that all the reads/writes to
the pci address space are done by the cpu.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
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             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci_bus.h |  1 +
 2 files changed, 47 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d00682e..b6a8026 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -283,6 +283,43 @@ const char *pci_root_bus_path(PCIDevice *dev)
     return rootbus->qbus.name;
 }
 
+static void unassigned_mem_access(PCIBus *bus)
+{
+    /* FIXME assumption: memory access to the pci address
+     * space is always initiated by the host bridge
+     * (device 0 on the bus) */
+    PCIDevice *d = bus->devices[0];
+    if (!d) {
+        return;
+    }
+
+    pci_word_test_and_set_mask(d->config + PCI_STATUS,
+                               PCI_STATUS_REC_MASTER_ABORT);
+}
+
+static uint64_t unassigned_mem_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PCIBus *bus = opaque;
+    unassigned_mem_access(bus);
+
+    return -1ULL;
+}
+
+static void unassigned_mem_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+    PCIBus *bus = opaque;
+    unassigned_mem_access(bus);
+}
+
+static const MemoryRegionOps unassigned_mem_ops = {
+    .read = unassigned_mem_read,
+    .write = unassigned_mem_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+#define UNASSIGNED_MEM_PRIORITY -1
+
 static void pci_bus_init(PCIBus *bus, DeviceState *parent,
                          const char *name,
                          MemoryRegion *address_space_mem,
@@ -294,6 +331,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->unassigned_mem, OBJECT(bus),
+                          &unassigned_mem_ops, bus, "pci-unassigned",
+                          memory_region_size(bus->address_space_mem));
+    memory_region_add_subregion_overlap(bus->address_space_mem,
+                                        bus->address_space_mem->addr,
+                                        &bus->unassigned_mem,
+                                        UNASSIGNED_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..4cc25a3 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 unassigned_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 RFC v3 2/2] hw/pci: handle unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 09, 2013 at 02:21:36PM +0300, Marcel Apfelbaum wrote:
> Created a MemoryRegion with negative priority that
> 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
>  3. sets the RECEIVED MASTER ABORT bit in the STATUS register
>     of the device that initiated the transaction
> 
> Note: This implementation assumes that all the reads/writes to
> the pci address space are done by the cpu.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> 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             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci_bus.h |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index d00682e..b6a8026 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -283,6 +283,43 @@ const char *pci_root_bus_path(PCIDevice *dev)
>      return rootbus->qbus.name;
>  }
>  
> +static void unassigned_mem_access(PCIBus *bus)
> +{
> +    /* FIXME assumption: memory access to the pci address
> +     * space is always initiated by the host bridge
> +     * (device 0 on the bus) */
> +    PCIDevice *d = bus->devices[0];
> +    if (!d) {
> +        return;
> +    }
> +
> +    pci_word_test_and_set_mask(d->config + PCI_STATUS,
> +                               PCI_STATUS_REC_MASTER_ABORT);
> +}
> +
> +static uint64_t unassigned_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIBus *bus = opaque;
> +    unassigned_mem_access(bus);
> +
> +    return -1ULL;
> +}
> +
> +static void unassigned_mem_write(void *opaque, hwaddr addr, uint64_t val,
> +                                unsigned size)
> +{
> +    PCIBus *bus = opaque;
> +    unassigned_mem_access(bus);
> +}
> +
> +static const MemoryRegionOps unassigned_mem_ops = {
> +    .read = unassigned_mem_read,
> +    .write = unassigned_mem_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +#define UNASSIGNED_MEM_PRIORITY -1
> +

This really should be "lowest available priority" correct?

So how about making it INT_MIN then?

>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                           const char *name,
>                           MemoryRegion *address_space_mem,
> @@ -294,6 +331,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->unassigned_mem, OBJECT(bus),
> +                          &unassigned_mem_ops, bus, "pci-unassigned",
> +                          memory_region_size(bus->address_space_mem));
> +    memory_region_add_subregion_overlap(bus->address_space_mem,
> +                                        bus->address_space_mem->addr,
> +                                        &bus->unassigned_mem,
> +                                        UNASSIGNED_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..4cc25a3 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 unassigned_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 RFC v3 2/2] hw/pci: handle unassigned pci addresses

[Marcel Apfelbaum]

On Sun, 2013-09-15 at 00:08 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 09, 2013 at 02:21:36PM +0300, Marcel Apfelbaum wrote:
> > Created a MemoryRegion with negative priority that
> > 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
> >  3. sets the RECEIVED MASTER ABORT bit in the STATUS register
> >     of the device that initiated the transaction
> > 
> > Note: This implementation assumes that all the reads/writes to
> > the pci address space are done by the cpu.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > 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             | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci_bus.h |  1 +
> >  2 files changed, 47 insertions(+)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index d00682e..b6a8026 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -283,6 +283,43 @@ const char *pci_root_bus_path(PCIDevice *dev)
> >      return rootbus->qbus.name;
> >  }
> >  
> > +static void unassigned_mem_access(PCIBus *bus)
> > +{
> > +    /* FIXME assumption: memory access to the pci address
> > +     * space is always initiated by the host bridge
> > +     * (device 0 on the bus) */
> > +    PCIDevice *d = bus->devices[0];
> > +    if (!d) {
> > +        return;
> > +    }
> > +
> > +    pci_word_test_and_set_mask(d->config + PCI_STATUS,
> > +                               PCI_STATUS_REC_MASTER_ABORT);
> > +}
> > +
> > +static uint64_t unassigned_mem_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    PCIBus *bus = opaque;
> > +    unassigned_mem_access(bus);
> > +
> > +    return -1ULL;
> > +}
> > +
> > +static void unassigned_mem_write(void *opaque, hwaddr addr, uint64_t val,
> > +                                unsigned size)
> > +{
> > +    PCIBus *bus = opaque;
> > +    unassigned_mem_access(bus);
> > +}
> > +
> > +static const MemoryRegionOps unassigned_mem_ops = {
> > +    .read = unassigned_mem_read,
> > +    .write = unassigned_mem_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +#define UNASSIGNED_MEM_PRIORITY -1
> > +
> 
> This really should be "lowest available priority" correct?
Yes, it should
> 
> So how about making it INT_MIN then?
Seems right, thanks
Marcel

> 
> >  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> >                           const char *name,
> >                           MemoryRegion *address_space_mem,
> > @@ -294,6 +331,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->unassigned_mem, OBJECT(bus),
> > +                          &unassigned_mem_ops, bus, "pci-unassigned",
> > +                          memory_region_size(bus->address_space_mem));
> > +    memory_region_add_subregion_overlap(bus->address_space_mem,
> > +                                        bus->address_space_mem->addr,
> > +                                        &bus->unassigned_mem,
> > +                                        UNASSIGNED_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..4cc25a3 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 unassigned_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