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

[Qemu-devel] [PATCH RFC 0/3] 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. (already implemented in quemu)

The series deals with the other aspect of the master abort scenario:
Upon completion the master has to raise MASTER ABORT BIT in i
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 MeomoryRegion's ops callbacks emulate the master abort scenario.

Note:
For the moment the code assumes that all the reads/writes on
pci address space are done by the cpu.

Marcel Apfelbaum (3):
  memory: allow MemoryRegion's priority field to accept negative values
  hw/pci: add MemoryRegion ops for unassigned pci addresses
  hw/pci-host: catch acesses to unassigned pci addresses

 hw/pci-host/piix.c        |  8 ++++++++
 hw/pci-host/q35.c         | 19 ++++++++++++++++---
 hw/pci/pci.c              | 18 ++++++++++++++++++
 include/exec/memory.h     |  6 +++---
 include/hw/pci-host/q35.h |  1 +
 include/hw/pci/pci.h      |  3 +++
 memory.c                  |  2 +-
 7 files changed, 50 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 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>
---
 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 886f838..dfb3ae6 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 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Peter Maydell]

On 2 September 2013 15:13, 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.

This comment is not exactly accurate. Allowing priority to
be signed is just a convenience: you can achieve exactly
the same effect by specifying some positive priority for
everything you map into the region and having the background
region be priority zero. (If you care at all about priorities
then everything being mapped into the region should be
happening under the control of your code anyway.)


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

This is unrelated to MemoryRegion priorities -- it controls the
order in which listener callbacks are called. Don't try to change
both at once (and you only need the MR priorities anyway.)

>      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 886f838..dfb3ae6 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;

This isn't a complete set of changes. For instance
memory_region_set_address() has a local variable 'priority'
which should be signed now.

sysbus_mmio_map_common() and sysbus_mmio_map_overlap()
have priority arguments which need to change to match.

thanks
-- PMM

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

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:38 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, 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.
> 
> This comment is not exactly accurate. Allowing priority to
> be signed is just a convenience: you can achieve exactly
> the same effect by specifying some positive priority for
> everything you map into the region and having the background
> region be priority zero. (If you care at all about priorities
> then everything being mapped into the region should be
> happening under the control of your code anyway.)
It seems (to me) that having the default priority 0 and
expanding on both directions depending on what you want to achieve
is an elegant solution. That being said, what you proposed would work too. 

> 
> > 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;
> 
> This is unrelated to MemoryRegion priorities -- it controls the
> order in which listener callbacks are called. Don't try to change
> both at once (and you only need the MR priorities anyway.)
I found that one :). I didn't updated the patches, thanks anyway
Marcel
> 
> >      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 886f838..dfb3ae6 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;
> 
> This isn't a complete set of changes. For instance
> memory_region_set_address() has a local variable 'priority'
> which should be signed now.
> 
> sysbus_mmio_map_common() and sysbus_mmio_map_overlap()
> have priority arguments which need to change to match.
> 
> thanks
> -- PMM
> 

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

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:38 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, 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.
> 
> This comment is not exactly accurate. Allowing priority to
> be signed is just a convenience: you can achieve exactly
> the same effect by specifying some positive priority for
> everything you map into the region and having the background
> region be priority zero. (If you care at all about priorities
> then everything being mapped into the region should be
> happening under the control of your code anyway.)
> 
> 
> > 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;
> 
> This is unrelated to MemoryRegion priorities -- it controls the
> order in which listener callbacks are called. Don't try to change
> both at once (and you only need the MR priorities anyway.)
> 
> >      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 886f838..dfb3ae6 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;
> 
> This isn't a complete set of changes. For instance
> memory_region_set_address() has a local variable 'priority'
> which should be signed now.
> 
> sysbus_mmio_map_common() and sysbus_mmio_map_overlap()
> have priority arguments which need to change to match.
Missed that :(
Making necessary changes and send a new version
Thanks,
Marcel

> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH RFC 2/3] hw/pci: add MemoryRegion ops for unassigned pci addresses

[Marcel Apfelbaum]

The MemoryRegions assigned with this ops shall "intercept"
the accesses to unassigned pci address space and the
associated callback will set MASTER ABORT bit in the
STATUS register of the device that initiated the
transaction as defined in PCI spec.

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>
---
 hw/pci/pci.c         | 18 ++++++++++++++++++
 include/hw/pci/pci.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c004f5..f0289fc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2229,6 +2229,24 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
     return dev->bus->address_space_io;
 }
 
+static bool pci_unassigned_mem_accepts(void *opaque, hwaddr addr,
+                                       unsigned size, bool is_write)
+{
+    PCIDevice *d = opaque;
+
+    /* FIXME assumption: the cpu initiated the pci transaction
+     * and not another pci device */
+    pci_word_test_and_set_mask(d->config + PCI_STATUS,
+                               PCI_STATUS_REC_MASTER_ABORT);
+
+    return false;
+}
+
+const MemoryRegionOps pci_unassigned_mem_ops = {
+    .valid.accepts = pci_unassigned_mem_accepts,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 static void pci_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ccec2ba..854681c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -329,6 +329,9 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
 MemoryRegion *pci_address_space(PCIDevice *dev);
 MemoryRegion *pci_address_space_io(PCIDevice *dev);
 
+#define PCI_UNASSIGNED_MEM_PRIORITY -1
+extern const MemoryRegionOps pci_unassigned_mem_ops;
+
 typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 2/3] hw/pci: add MemoryRegion ops for unassigned pci addresses

[Peter Maydell]

On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> +const MemoryRegionOps pci_unassigned_mem_ops = {
> +    .valid.accepts = pci_unassigned_mem_accepts,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};

This is wrong -- you want reads and writes to result in
your PCI-spec-defined behaviour, but if you provide
an accepts callback and it returns false then you get
"machine dependent behaviour such as a machine check
exception". What you want is to provide .read and .write
callbacks which behave as the PCI spec mandates.

Also this should probably be static, not global, and you
should put it in the same patch as the one which actually
creates the memory region.

-- PMM

Re: [Qemu-devel] [PATCH RFC 2/3] hw/pci: add MemoryRegion ops for unassigned pci addresses

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:42 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > +const MemoryRegionOps pci_unassigned_mem_ops = {
> > +    .valid.accepts = pci_unassigned_mem_accepts,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> 
> This is wrong -- you want reads and writes to result in
> your PCI-spec-defined behaviour, but if you provide
> an accepts callback and it returns false then you get
> "machine dependent behaviour such as a machine check
> exception". What you want is to provide .read and .write
> callbacks which behave as the PCI spec mandates.
That makes sense. It was already a bug.Thanks!

> 
> Also this should probably be static, not global, and you
If it will be implemented in the "pci core layer", sure
Marcel

> should put it in the same patch as the one which actually
> creates the memory region.
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 2/3] hw/pci: add MemoryRegion ops for unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 02, 2013 at 05:13:08PM +0300, Marcel Apfelbaum wrote:
> The MemoryRegions assigned with this ops shall "intercept"
> the accesses to unassigned pci address space and the
> associated callback will set MASTER ABORT bit in the
> STATUS register of the device that initiated the
> transaction as defined in PCI spec.
> 
> 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>
> ---
>  hw/pci/pci.c         | 18 ++++++++++++++++++
>  include/hw/pci/pci.h |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4c004f5..f0289fc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2229,6 +2229,24 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>      return dev->bus->address_space_io;
>  }
>  
> +static bool pci_unassigned_mem_accepts(void *opaque, hwaddr addr,
> +                                       unsigned size, bool is_write)
> +{
> +    PCIDevice *d = opaque;
> +
> +    /* FIXME assumption: the cpu initiated the pci transaction
> +     * and not another pci device */

/*
 * Multiline
 * comments
 */


> +    pci_word_test_and_set_mask(d->config + PCI_STATUS,
> +                               PCI_STATUS_REC_MASTER_ABORT);
> +
> +    return false;
> +}
> +
> +const MemoryRegionOps pci_unassigned_mem_ops = {
> +    .valid.accepts = pci_unassigned_mem_accepts,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  static void pci_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ccec2ba..854681c 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -329,6 +329,9 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
>  MemoryRegion *pci_address_space(PCIDevice *dev);
>  MemoryRegion *pci_address_space_io(PCIDevice *dev);
>  
> +#define PCI_UNASSIGNED_MEM_PRIORITY -1
> +extern const MemoryRegionOps pci_unassigned_mem_ops;
> +
>  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
>  typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH RFC 2/3] hw/pci: add MemoryRegion ops for unassigned pci addresses

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 17:48 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 02, 2013 at 05:13:08PM +0300, Marcel Apfelbaum wrote:
> > The MemoryRegions assigned with this ops shall "intercept"
> > the accesses to unassigned pci address space and the
> > associated callback will set MASTER ABORT bit in the
> > STATUS register of the device that initiated the
> > transaction as defined in PCI spec.
> > 
> > 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>
> > ---
> >  hw/pci/pci.c         | 18 ++++++++++++++++++
> >  include/hw/pci/pci.h |  3 +++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 4c004f5..f0289fc 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2229,6 +2229,24 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
> >      return dev->bus->address_space_io;
> >  }
> >  
> > +static bool pci_unassigned_mem_accepts(void *opaque, hwaddr addr,
> > +                                       unsigned size, bool is_write)
> > +{
> > +    PCIDevice *d = opaque;
> > +
> > +    /* FIXME assumption: the cpu initiated the pci transaction
> > +     * and not another pci device */
> 
> /*
>  * Multiline
>  * comments
>  */
Got it. Thanks!
Marcel
> 
> 
> > +    pci_word_test_and_set_mask(d->config + PCI_STATUS,
> > +                               PCI_STATUS_REC_MASTER_ABORT);
> > +
> > +    return false;
> > +}
> > +
> > +const MemoryRegionOps pci_unassigned_mem_ops = {
> > +    .valid.accepts = pci_unassigned_mem_accepts,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> >  static void pci_device_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *k = DEVICE_CLASS(klass);
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index ccec2ba..854681c 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -329,6 +329,9 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
> >  MemoryRegion *pci_address_space(PCIDevice *dev);
> >  MemoryRegion *pci_address_space_io(PCIDevice *dev);
> >  
> > +#define PCI_UNASSIGNED_MEM_PRIORITY -1
> > +extern const MemoryRegionOps pci_unassigned_mem_ops;
> > +
> >  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> >  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> >  typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> > -- 
> > 1.8.3.1
> 

[Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Marcel Apfelbaum]

Added a memory region that has negative priority and
extends over all the pci adddress space. This region will
"catch" all the accesses to the unassigned pci
addresses and it will be possible to emulate the
master abort scenario (When no device on the bus claims
the transaction).

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci-host/piix.c        |  8 ++++++++
 hw/pci-host/q35.c         | 19 ++++++++++++++++---
 include/hw/pci-host/q35.h |  1 +
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index dc1718f..27b04a6 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -104,6 +104,7 @@ struct PCII440FXState {
     MemoryRegion *ram_memory;
     MemoryRegion pci_hole;
     MemoryRegion pci_hole_64bit;
+    MemoryRegion pci_unassigned_memory;
     PAMMemoryRegion pam_regions[13];
     MemoryRegion smram_region;
     uint8_t smm_enabled;
@@ -347,6 +348,13 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
         i440fx->pci_info.w32.begin = 0xe0000000;
     }
 
+    memory_region_init_io(&f->pci_unassigned_memory, OBJECT(d),
+                          &pci_unassigned_mem_ops, d,
+                          "pci-hole-unassigned", pci_hole_size);
+    memory_region_add_subregion_overlap(f->pci_address_space, pci_hole_start,
+                                        &f->pci_unassigned_memory,
+                                        PCI_UNASSIGNED_MEM_PRIORITY);
+
     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
                              pci_hole_start, pci_hole_size);
     memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 12314d8..ee4a4f9 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -320,13 +320,26 @@ static int mch_init(PCIDevice *d)
 {
     int i;
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
+    hwaddr pci_hole_start;
+    uint64_t pci_hole_size;
+
+    pci_hole_start = mch->below_4g_mem_size;
+    pci_hole_size = 0x100000000ULL - pci_hole_start;
+
+    memory_region_init_io(&mch->pci_unassigned_memory, OBJECT(d),
+                          &pci_unassigned_mem_ops, d,
+                          "pci-hole-unassigned", pci_hole_size);
+    memory_region_add_subregion_overlap(mch->pci_address_space, pci_hole_start,
+                                        &mch->pci_unassigned_memory,
+                                        PCI_UNASSIGNED_MEM_PRIORITY);
+
 
     /* setup pci memory regions */
     memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
                              mch->pci_address_space,
-                             mch->below_4g_mem_size,
-                             0x100000000ULL - mch->below_4g_mem_size);
-    memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
+                             pci_hole_start,
+                             pci_hole_size);
+    memory_region_add_subregion(mch->system_memory, pci_hole_start,
                                 &mch->pci_hole);
 
     pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 6eb7ab6..c42fba3 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,6 +55,7 @@ typedef struct MCHPCIState {
     MemoryRegion smram_region;
     MemoryRegion pci_hole;
     MemoryRegion pci_hole_64bit;
+    MemoryRegion pci_unassigned_memory;
     PcPciInfo pci_info;
     uint8_t smm_enabled;
     ram_addr_t below_4g_mem_size;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Peter Maydell]

On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> Added a memory region that has negative priority and
> extends over all the pci adddress space. This region will
> "catch" all the accesses to the unassigned pci
> addresses and it will be possible to emulate the
> master abort scenario (When no device on the bus claims
> the transaction).
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/pci-host/piix.c        |  8 ++++++++
>  hw/pci-host/q35.c         | 19 ++++++++++++++++---
>  include/hw/pci-host/q35.h |  1 +

This is happening at the wrong layer -- you want this memory
region to be created and managed in the PCI core code so that
we get correct PCI-spec behaviour for all our PCI controllers,
not just the two x86 ones you've changed here.

-- PMM

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > Added a memory region that has negative priority and
> > extends over all the pci adddress space. This region will
> > "catch" all the accesses to the unassigned pci
> > addresses and it will be possible to emulate the
> > master abort scenario (When no device on the bus claims
> > the transaction).
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/pci-host/piix.c        |  8 ++++++++
> >  hw/pci-host/q35.c         | 19 ++++++++++++++++---
> >  include/hw/pci-host/q35.h |  1 +
> 
> This is happening at the wrong layer -- you want this memory
> region to be created and managed in the PCI core code so that
> we get correct PCI-spec behaviour for all our PCI controllers,
> not just the two x86 ones you've changed here.pci_address_space
I saw that the memory regions are part of the Host state and
duplicated for each host type(like pci_address_space). 
Question, why are not pci_address_space and pci_hole present
in a core layer?

I followed the existing code; from what you are saying
I understand that also the existing memory regions 
like the one mentioned above should be moved in
the core layer, right?
Marcel 

> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Andreas Färber]

Am 02.09.2013 17:42, schrieb Marcel Apfelbaum:
> On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
>> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>>> Added a memory region that has negative priority and
>>> extends over all the pci adddress space. This region will
>>> "catch" all the accesses to the unassigned pci
>>> addresses and it will be possible to emulate the
>>> master abort scenario (When no device on the bus claims
>>> the transaction).
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>> ---
>>>  hw/pci-host/piix.c        |  8 ++++++++
>>>  hw/pci-host/q35.c         | 19 ++++++++++++++++---
>>>  include/hw/pci-host/q35.h |  1 +
>>
>> This is happening at the wrong layer -- you want this memory
>> region to be created and managed in the PCI core code so that
>> we get correct PCI-spec behaviour for all our PCI controllers,
>> not just the two x86 ones you've changed here.pci_address_space
> I saw that the memory regions are part of the Host state and
> duplicated for each host type(like pci_address_space). 
> Question, why are not pci_address_space and pci_hole present
> in a core layer?

I would say, because that core layer didn't exist before I added it not
too long ago. My focus was on fixing bugs at the time and getting PReP
Raven PHB into shape for QOM.

> I followed the existing code; from what you are saying
> I understand that also the existing memory regions 
> like the one mentioned above should be moved in
> the core layer, right?

Consider it all Work In Progress :) and feel free to move more fields
and code there as you guys see fit.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Peter Maydell]

On 2 September 2013 16:42, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
>> This is happening at the wrong layer -- you want this memory
>> region to be created and managed in the PCI core code so that
>> we get correct PCI-spec behaviour for all our PCI controllers,
>> not just the two x86 ones you've changed here.pci_address_space
> I saw that the memory regions are part of the Host state and
> duplicated for each host type(like pci_address_space).
> Question, why are not pci_address_space and pci_hole present
> in a core layer?
>
> I followed the existing code; from what you are saying
> I understand that also the existing memory regions
> like the one mentioned above should be moved in
> the core layer, right?

Ideally, yes, I think so. However that's not particularly
a requirement for the changes you're trying to make here:
at the moment what happens is that the pci controller
creates the PCI memory and io memory regions (or cheats
by reusing the system memory space[*]), passes them to
the PCI core code (via pci_bus_new) and then they're
the PCI code's responsibility to manage. So in the PCI
code you can ignore where they came from when you're
deciding how to manage these containers (and in this case
what you do is just create your default region and map
it in to the container at a suitable priority).

[*] I'm pretty sure this is a bug in all platforms that do it.

-- PMM

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Peter Maydell]

On 2 September 2013 16:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 September 2013 16:42, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>> Question, why are not pci_address_space and pci_hole present
>> in a core layer?
>>
>> I followed the existing code; from what you are saying
>> I understand that also the existing memory regions
>> like the one mentioned above should be moved in
>> the core layer, right?
>
> Ideally, yes, I think so.

...but I didn't notice you mentioned pci_hole there. That
is a PCI-controller specific concept (there's no such thing
for the versatilepb PCI controller, for instance) and so it
should remain at the PCI controller level, not the PCI
core code.

-- PMM

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 02, 2013 at 04:53:50PM +0100, Peter Maydell wrote:
> On 2 September 2013 16:42, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
> >> This is happening at the wrong layer -- you want this memory
> >> region to be created and managed in the PCI core code so that
> >> we get correct PCI-spec behaviour for all our PCI controllers,
> >> not just the two x86 ones you've changed here.pci_address_space
> > I saw that the memory regions are part of the Host state and
> > duplicated for each host type(like pci_address_space).
> > Question, why are not pci_address_space and pci_hole present
> > in a core layer?
> >
> > I followed the existing code; from what you are saying
> > I understand that also the existing memory regions
> > like the one mentioned above should be moved in
> > the core layer, right?
> 
> Ideally, yes, I think so. However that's not particularly
> a requirement for the changes you're trying to make here:
> at the moment what happens is that the pci controller
> creates the PCI memory and io memory regions (or cheats
> by reusing the system memory space[*]), passes them to
> the PCI core code (via pci_bus_new) and then they're
> the PCI code's responsibility to manage. So in the PCI
> code you can ignore where they came from when you're
> deciding how to manage these containers (and in this case
> what you do is just create your default region and map
> it in to the container at a suitable priority).
> 
> [*] I'm pretty sure this is a bug in all platforms that do it.
> 
> -- PMM

Well as usual this cheat originated with PIIX.
AFAIK PIIX actually has a shared bus for memory and PCI
so this is not a bug there, I think.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Peter Maydell]

On 2 September 2013 17:00, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 02, 2013 at 04:53:50PM +0100, Peter Maydell wrote:
>> at the moment what happens is that the pci controller
>> creates the PCI memory and io memory regions (or cheats
>> by reusing the system memory space[*]),

>> [*] I'm pretty sure this is a bug in all platforms that do it.

> Well as usual this cheat originated with PIIX.
> AFAIK PIIX actually has a shared bus for memory and PCI
> so this is not a bug there, I think.

It will be when you introduce this "return -1 for unassigned
addresses", though, since you only want that to happen
for PCI accesses, not system memory accesses, right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 02, 2013 at 05:05:10PM +0100, Peter Maydell wrote:
> On 2 September 2013 17:00, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Sep 02, 2013 at 04:53:50PM +0100, Peter Maydell wrote:
> >> at the moment what happens is that the pci controller
> >> creates the PCI memory and io memory regions (or cheats
> >> by reusing the system memory space[*]),
> 
> >> [*] I'm pretty sure this is a bug in all platforms that do it.
> 
> > Well as usual this cheat originated with PIIX.
> > AFAIK PIIX actually has a shared bus for memory and PCI
> > so this is not a bug there, I think.
> 
> It will be when you introduce this "return -1 for unassigned
> addresses", though, since you only want that to happen
> for PCI accesses, not system memory accesses, right?
> 
> thanks
> -- PMM

What happens with PIIX is that everything that is not
in system memory is PCI.
So there's no such thing as "unassigned system memory
address": all unassigned addresses are PCI addresses.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 16:53 +0100, Peter Maydell wrote:
> On 2 September 2013 16:42, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
> >> This is happening at the wrong layer -- you want this memory
> >> region to be created and managed in the PCI core code so that
> >> we get correct PCI-spec behaviour for all our PCI controllers,
> >> not just the two x86 ones you've changed here.pci_address_space
> > I saw that the memory regions are part of the Host state and
> > duplicated for each host type(like pci_address_space).
> > Question, why are not pci_address_space and pci_hole present
> > in a core layer?
> >
> > I followed the existing code; from what you are saying
> > I understand that also the existing memory regions
> > like the one mentioned above should be moved in
> > the core layer, right?
> 
> Ideally, yes, I think so. However that's not particularly
> a requirement for the changes you're trying to make here:
> at the moment what happens is that the pci controller
> creates the PCI memory and io memory regions (or cheats
> by reusing the system memory space[*]), passes them to
> the PCI core code (via pci_bus_new) and then they're
> the PCI code's responsibility to manage. So in the PCI
> code you can ignore where they came from when you're
> deciding how to manage these containers (and in this case
> what you do is just create your default region and map
> it in to the container at a suitable priority).
Thanks, I am going to follow this path
Marcel

> [*] I'm pretty sure this is a bug in all platforms that do it.
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 3/3] hw/pci-host: catch acesses to unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 02, 2013 at 06:42:33PM +0300, Marcel Apfelbaum wrote:
> On Mon, 2013-09-02 at 15:39 +0100, Peter Maydell wrote:
> > On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > > Added a memory region that has negative priority and
> > > extends over all the pci adddress space. This region will
> > > "catch" all the accesses to the unassigned pci
> > > addresses and it will be possible to emulate the
> > > master abort scenario (When no device on the bus claims
> > > the transaction).
> > >
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > >  hw/pci-host/piix.c        |  8 ++++++++
> > >  hw/pci-host/q35.c         | 19 ++++++++++++++++---
> > >  include/hw/pci-host/q35.h |  1 +
> > 
> > This is happening at the wrong layer -- you want this memory
> > region to be created and managed in the PCI core code so that
> > we get correct PCI-spec behaviour for all our PCI controllers,
> > not just the two x86 ones you've changed here.pci_address_space
> I saw that the memory regions are part of the Host state and
> duplicated for each host type(like pci_address_space). 
> Question, why are not pci_address_space and pci_hole present
> in a core layer?


I think we can move them out to core.

> 
> I followed the existing code; from what you are saying
> I understand that also the existing memory regions 
> like the one mentioned above should be moved in
> the core layer, right?
> Marcel 

pci hole is a PC thing.

> > 
> > -- PMM
> > 
> 

Re: [Qemu-devel] [PATCH RFC 0/3] pci: complete master abort protocol

[Peter Maydell]

On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> 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. (already implemented in quemu)

...but only erroneously and by breaking a pile of other boards.

> Note:
> For the moment the code assumes that all the reads/writes on
> pci address space are done by the cpu.

This is a bogus assumption in the presence of bus mastering
PCI devices, which aren't exactly uncommon.

-- PMM

Re: [Qemu-devel] [PATCH RFC 0/3] pci: complete master abort protocol

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:30 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > 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. (already implemented in quemu)
> 
> ...but only erroneously and by breaking a pile of other boards.
:(
Let's find a way to fix it... Please suggest
> 
> > Note:
> > For the moment the code assumes that all the reads/writes on
> > pci address space are done by the cpu.
> 
> This is a bogus assumption in the presence of bus mastering
> PCI devices, which aren't exactly uncommon.
As I said, the series is work in progress. Tracking down
the real initiator of a read/write to the pci address space
seems to be difficult. I am looking into it and opened
to suggestions.
Marcel
 
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 0/3] pci: complete master abort protocol

[Peter Maydell]

On 2 September 2013 15:39, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Mon, 2013-09-02 at 15:30 +0100, Peter Maydell wrote:
>> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>> > 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. (already implemented in quemu)
>>
>> ...but only erroneously and by breaking a pile of other boards.
> :(
> Let's find a way to fix it... Please suggest

We need to apply Jan's two patches which revert the breakage
and give us back 1.5 behaviour for unassigned memory.
Then you can work on this patchset and we can commit it
when it works OK.

-- PMM

Re: [Qemu-devel] [PATCH RFC 0/3] pci: complete master abort protocol

[Marcel Apfelbaum]

On Mon, 2013-09-02 at 15:43 +0100, Peter Maydell wrote:
> On 2 September 2013 15:39, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > On Mon, 2013-09-02 at 15:30 +0100, Peter Maydell wrote:
> >> On 2 September 2013 15:13, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> >> > 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. (already implemented in quemu)
> >>
> >> ...but only erroneously and by breaking a pile of other boards.
> > :(
> > Let's find a way to fix it... Please suggest
> 
> We need to apply Jan's two patches which revert the breakage
> and give us back 1.5 behaviour for unassigned memory.
> Then you can work on this patchset and we can commit it
> when it works OK.
I am going to have a look on those patches, thanks
Marcel

> 
> -- PMM
>