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 <address@hidden>

> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
> 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