[Qemu-devel] [PATCH for 1.6 0/2] Fix unassigned memory and I/O access handling

[Qemu-devel] [PATCH for 1.6 0/2] Fix unassigned memory and I/O access handling

[Jan Kiszka]

This just binds both patches properly together and fixes up the return
value size of unassigned_io_read as suggested. Please merge before the
1.6 release.

Jan Kiszka (2):
  memory: Provide separate handling of unassigned io ports accesses
  Revert "memory: Return -1 again on reads from unsigned regions"

 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 memory.c              |    2 +-
 4 files changed, 21 insertions(+), 2 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Jan Kiszka]

Accesses to unassigned io ports shall return -1 on read and be ignored
on write. Ensure these properties via dedicated ops, decoupling us from
the memory core's handling of unassigned accesses.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                |    3 ++-
 include/exec/ioport.h |    2 ++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/exec.c b/exec.c
index 3ca9381..9ed598f 100644
--- a/exec.c
+++ b/exec.c
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);
     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index bdd4e96..84f7f85 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -45,6 +45,8 @@ typedef struct MemoryRegionPortio {
 
 #define PORTIO_END_OF_LIST() { }
 
+extern const MemoryRegionOps unassigned_io_ops;
+
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
 void cpu_outl(pio_addr_t addr, uint32_t val);
diff --git a/ioport.c b/ioport.c
index 79b7f1a..707cce8 100644
--- a/ioport.c
+++ b/ioport.c
@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 
+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1ULL;
+}
+
+static void unassigned_io_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+}
+
+const MemoryRegionOps unassigned_io_ops = {
+    .read = unassigned_io_read,
+    .write = unassigned_io_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
-- 
1.7.3.4

[Qemu-devel] [PATCH for 1.6 2/2] Revert "memory: Return -1 again on reads from unsigned regions"

[Jan Kiszka]

This reverts commit 9b8c69243585a32d14b9bb9fcd52c37b0b5a1b71.

The commit was wrong: We only return -1 on invalid accesses, not on
valid but unbacked ones. This broke various corner cases.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 memory.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/memory.c b/memory.c
index 886f838..5a10fd0 100644
--- a/memory.c
+++ b/memory.c
@@ -872,7 +872,7 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
     if (current_cpu != NULL) {
         cpu_unassigned_access(current_cpu, addr, false, false, 0, size);
     }
-    return -1ULL;
+    return 0;
 }
 
 static void unassigned_mem_write(void *opaque, hwaddr addr,
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH for 1.6 2/2] Revert "memory: Return -1 again on reads from unsigned regions"

[Andreas Färber]

Am 09.08.2013 14:01, schrieb Jan Kiszka:
> This reverts commit 9b8c69243585a32d14b9bb9fcd52c37b0b5a1b71.
> 
> The commit was wrong: We only return -1 on invalid accesses, not on
> valid but unbacked ones. This broke various corner cases.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Tested-by: Andreas Färber <andreas.faerber@web.de>

Restores previous PReP behavior.

Thanks,
Andreas

Re: [Qemu-devel] [PATCH for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> Accesses to unassigned io ports shall return -1 on read and be ignored
> on write. Ensure these properties via dedicated ops, decoupling us from
> the memory core's handling of unassigned accesses.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Breaks the build (linux-user):

  LINK  xtensa-softmmu/qemu-system-xtensa
  CC    alpha-linux-user/exec.o
In file included from /home/aliguori/git/qemu/include/hw/hw.h:11:0,
                 from /home/aliguori/git/qemu/exec.c:30:
/home/aliguori/git/qemu/include/exec/ioport.h:48:1: error: unknown type name ‘MemoryRegionOps’
make[1]: *** [exec.o] Error 1
make: *** [subdir-alpha-linux-user] Error 2

Regards,

Anthony Liguori

> ---
>  exec.c                |    3 ++-
>  include/exec/ioport.h |    2 ++
>  ioport.c              |   16 ++++++++++++++++
>  3 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 3ca9381..9ed598f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1820,7 +1820,8 @@ static void memory_map_init(void)
>      address_space_init(&address_space_memory, system_memory, "memory");
>  
>      system_io = g_malloc(sizeof(*system_io));
> -    memory_region_init(system_io, NULL, "io", 65536);
> +    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
> +                          65536);
>      address_space_init(&address_space_io, system_io, "I/O");
>  
>      memory_listener_register(&core_memory_listener, &address_space_memory);
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index bdd4e96..84f7f85 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -45,6 +45,8 @@ typedef struct MemoryRegionPortio {
>  
>  #define PORTIO_END_OF_LIST() { }
>  
> +extern const MemoryRegionOps unassigned_io_ops;
> +
>  void cpu_outb(pio_addr_t addr, uint8_t val);
>  void cpu_outw(pio_addr_t addr, uint16_t val);
>  void cpu_outl(pio_addr_t addr, uint32_t val);
> diff --git a/ioport.c b/ioport.c
> index 79b7f1a..707cce8 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
>      MemoryRegionPortio ports[];
>  } MemoryRegionPortioList;
>  
> +static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return -1ULL;
> +}
> +
> +static void unassigned_io_write(void *opaque, hwaddr addr, uint64_t val,
> +                                unsigned size)
> +{
> +}
> +
> +const MemoryRegionOps unassigned_io_ops = {
> +    .read = unassigned_io_read,
> +    .write = unassigned_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  void cpu_outb(pio_addr_t addr, uint8_t val)
>  {
>      LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
> -- 
> 1.7.3.4

[Qemu-devel] [PATCH v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Jan Kiszka]

Accesses to unassigned io ports shall return -1 on read and be ignored
on write. Ensure these properties via dedicated ops, decoupling us from
the memory core's handling of unassigned accesses.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - avoid breakage in ioport.h for user build

 exec.c                |    3 ++-
 include/exec/ioport.h |    4 ++++
 ioport.c              |   16 ++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/exec.c b/exec.c
index 3ca9381..9ed598f 100644
--- a/exec.c
+++ b/exec.c
@@ -1820,7 +1820,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init(system_io, NULL, "io", 65536);
+    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
+                          65536);
     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index bdd4e96..b3848be 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -45,6 +45,10 @@ typedef struct MemoryRegionPortio {
 
 #define PORTIO_END_OF_LIST() { }
 
+#ifndef CONFIG_USER_ONLY
+extern const MemoryRegionOps unassigned_io_ops;
+#endif
+
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
 void cpu_outl(pio_addr_t addr, uint32_t val);
diff --git a/ioport.c b/ioport.c
index 79b7f1a..707cce8 100644
--- a/ioport.c
+++ b/ioport.c
@@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 
+static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1ULL;
+}
+
+static void unassigned_io_write(void *opaque, hwaddr addr, uint64_t val,
+                                unsigned size)
+{
+}
+
+const MemoryRegionOps unassigned_io_ops = {
+    .read = unassigned_io_read,
+    .write = unassigned_io_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Andreas Färber]

Am 12.08.2013 17:29, schrieb Jan Kiszka:
> Accesses to unassigned io ports shall return -1 on read and be ignored
> on write. Ensure these properties via dedicated ops, decoupling us from
> the memory core's handling of unassigned accesses.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v2:
>  - avoid breakage in ioport.h for user build

Looks OK, but if you want Anthony to apply this for rc3 then please
repost as a full, top-level series.

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 v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Anthony Liguori]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> Accesses to unassigned io ports shall return -1 on read and be ignored
> on write. Ensure these properties via dedicated ops, decoupling us from
> the memory core's handling of unassigned accesses.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Please top-post.

Regards,

Anthony Liguori

> ---
>
> Changes in v2:
>  - avoid breakage in ioport.h for user build
>
>  exec.c                |    3 ++-
>  include/exec/ioport.h |    4 ++++
>  ioport.c              |   16 ++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 3ca9381..9ed598f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1820,7 +1820,8 @@ static void memory_map_init(void)
>      address_space_init(&address_space_memory, system_memory, "memory");
>  
>      system_io = g_malloc(sizeof(*system_io));
> -    memory_region_init(system_io, NULL, "io", 65536);
> +    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
> +                          65536);
>      address_space_init(&address_space_io, system_io, "I/O");
>  
>      memory_listener_register(&core_memory_listener, &address_space_memory);
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index bdd4e96..b3848be 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -45,6 +45,10 @@ typedef struct MemoryRegionPortio {
>  
>  #define PORTIO_END_OF_LIST() { }
>  
> +#ifndef CONFIG_USER_ONLY
> +extern const MemoryRegionOps unassigned_io_ops;
> +#endif
> +
>  void cpu_outb(pio_addr_t addr, uint8_t val);
>  void cpu_outw(pio_addr_t addr, uint16_t val);
>  void cpu_outl(pio_addr_t addr, uint32_t val);
> diff --git a/ioport.c b/ioport.c
> index 79b7f1a..707cce8 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -44,6 +44,22 @@ typedef struct MemoryRegionPortioList {
>      MemoryRegionPortio ports[];
>  } MemoryRegionPortioList;
>  
> +static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return -1ULL;
> +}
> +
> +static void unassigned_io_write(void *opaque, hwaddr addr, uint64_t val,
> +                                unsigned size)
> +{
> +}
> +
> +const MemoryRegionOps unassigned_io_ops = {
> +    .read = unassigned_io_read,
> +    .write = unassigned_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  void cpu_outb(pio_addr_t addr, uint8_t val)
>  {
>      LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
> -- 
> 1.7.3.4

Re: [Qemu-devel] [PATCH v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Peter Maydell]

On 12 August 2013 16:39, Andreas Färber <afaerber@suse.de> wrote:
> Am 12.08.2013 17:29, schrieb Jan Kiszka:
>> Accesses to unassigned io ports shall return -1 on read and be ignored
>> on write. Ensure these properties via dedicated ops, decoupling us from
>> the memory core's handling of unassigned accesses.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v2:
>>  - avoid breakage in ioport.h for user build
>
> Looks OK, but if you want Anthony to apply this for rc3 then please
> repost as a full, top-level series.

It looks like this never happened, so this bug is still present
in master :-(  [noticed by stsquad since integratorcp is busted too].

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Jan Kiszka]

On 2013-09-02 15:52, Peter Maydell wrote:
> On 12 August 2013 16:39, Andreas Färber <afaerber@suse.de> wrote:
>> Am 12.08.2013 17:29, schrieb Jan Kiszka:
>>> Accesses to unassigned io ports shall return -1 on read and be ignored
>>> on write. Ensure these properties via dedicated ops, decoupling us from
>>> the memory core's handling of unassigned accesses.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - avoid breakage in ioport.h for user build
>>
>> Looks OK, but if you want Anthony to apply this for rc3 then please
>> repost as a full, top-level series.
> 
> It looks like this never happened, so this bug is still present
> in master :-(  [noticed by stsquad since integratorcp is busted too].

I suppose the repost just fell through the cracks at the end of the
release cycle. Let me re-repost the whole thing, CC'ing stable.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Andreas Färber]

Am 02.09.2013 17:56, schrieb Jan Kiszka:
> On 2013-09-02 15:52, Peter Maydell wrote:
>> On 12 August 2013 16:39, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 12.08.2013 17:29, schrieb Jan Kiszka:
>>>> Accesses to unassigned io ports shall return -1 on read and be ignored
>>>> on write. Ensure these properties via dedicated ops, decoupling us from
>>>> the memory core's handling of unassigned accesses.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>  - avoid breakage in ioport.h for user build
>>>
>>> Looks OK, but if you want Anthony to apply this for rc3 then please
>>> repost as a full, top-level series.
>>
>> It looks like this never happened, so this bug is still present
>> in master :-(  [noticed by stsquad since integratorcp is busted too].
> 
> I suppose the repost just fell through the cracks at the end of the
> release cycle. Let me re-repost the whole thing, CC'ing stable.

I vaguely remember you reposting 2/2 top-level only and asking Anthony
to fix his tool instead, which was quite apparently not helpful. ;)

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 v2 for 1.6 1/2] memory: Provide separate handling of unassigned io ports accesses

[Jan Kiszka]

On 2013-09-02 17:58, Andreas Färber wrote:
> Am 02.09.2013 17:56, schrieb Jan Kiszka:
>> On 2013-09-02 15:52, Peter Maydell wrote:
>>> On 12 August 2013 16:39, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 12.08.2013 17:29, schrieb Jan Kiszka:
>>>>> Accesses to unassigned io ports shall return -1 on read and be ignored
>>>>> on write. Ensure these properties via dedicated ops, decoupling us from
>>>>> the memory core's handling of unassigned accesses.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>>  - avoid breakage in ioport.h for user build
>>>>
>>>> Looks OK, but if you want Anthony to apply this for rc3 then please
>>>> repost as a full, top-level series.
>>>
>>> It looks like this never happened, so this bug is still present
>>> in master :-(  [noticed by stsquad since integratorcp is busted too].
>>
>> I suppose the repost just fell through the cracks at the end of the
>> release cycle. Let me re-repost the whole thing, CC'ing stable.
> 
> I vaguely remember you reposting 2/2 top-level only and asking Anthony
> to fix his tool instead, which was quite apparently not helpful. ;)

Well, I wasn't expecting this to happen within the release cycle. But I
would still appreciate having to do such thread-breaking updates in the
future.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux