diff mbox

[2/2] Revert "memory: Return -1 again on reads from unsigned regions"

Message ID 51FCBFD2.3030401@web.de
State New
Headers show

Commit Message

Jan Kiszka Aug. 3, 2013, 8:31 a.m. UTC
From: Jan Kiszka <jan.kiszka@siemens.com>

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(-)

Comments

Peter Maydell Aug. 8, 2013, 3:23 p.m. UTC | #1
On 3 August 2013 09:31, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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>

This revert fixes the regression in the 'musicpal' board.

Presumably these two patches should go into 1.6 since it is
a regression fix... cc'ing Anthony.

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

-- PMM
Jan Kiszka Aug. 8, 2013, 3:27 p.m. UTC | #2
On 2013-08-08 17:23, Peter Maydell wrote:
> On 3 August 2013 09:31, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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>
> 
> This revert fixes the regression in the 'musicpal' board.
> 
> Presumably these two patches should go into 1.6 since it is
> a regression fix... cc'ing Anthony.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Yes, for 1.6 definitely. Sorry, forgot the proper CCing/tagging.

Thanks,
Jan
Andreas Färber Aug. 8, 2013, 4:46 p.m. UTC | #3
Am 08.08.2013 17:23, schrieb Peter Maydell:
> On 3 August 2013 09:31, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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>
> 
> This revert fixes the regression in the 'musicpal' board.
> 
> Presumably these two patches should go into 1.6 since it is
> a regression fix... cc'ing Anthony.

Since no one has spoken up with a concrete use case that breaks when
having the PIO region opaque, I agree this seems the best solution we
have for 1.6. But given the musicpal issue you have raised, I would ask
Paolo or Jan to squash these two patches together to avoid a git-bisect
regression.

Thanks,
Andreas

> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM
>
Peter Maydell Aug. 8, 2013, 4:48 p.m. UTC | #4
On 8 August 2013 17:46, Andreas Färber <afaerber@suse.de> wrote:
> Since no one has spoken up with a concrete use case that breaks when
> having the PIO region opaque, I agree this seems the best solution we
> have for 1.6. But given the musicpal issue you have raised, I would ask
> Paolo or Jan to squash these two patches together to avoid a git-bisect
> regression.

Not sure what you have in mind here -- musicpal is an ARM
board, and no ARM system should ever touch the system_io
region at all, so for musicpal patch 1/2 has no effect
and we care only about 2/2.

-- PMM
Andreas Färber Aug. 8, 2013, 4:55 p.m. UTC | #5
Am 08.08.2013 18:48, schrieb Peter Maydell:
> On 8 August 2013 17:46, Andreas Färber <afaerber@suse.de> wrote:
>> Since no one has spoken up with a concrete use case that breaks when
>> having the PIO region opaque, I agree this seems the best solution we
>> have for 1.6. But given the musicpal issue you have raised, I would ask
>> Paolo or Jan to squash these two patches together to avoid a git-bisect
>> regression.
> 
> Not sure what you have in mind here -- musicpal is an ARM
> board, and no ARM system should ever touch the system_io
> region at all, so for musicpal patch 1/2 has no effect
> and we care only about 2/2.

Sorry, mixed up your other reply with this non-threaded series - thought
you were saying 1/2 introduced the regression, but the regression is
already in qemu.git I understand just like for prep.

Andreas
diff mbox

Patch

diff --git a/memory.c b/memory.c
index ac6f3c6..7a0251d 100644
--- a/memory.c
+++ b/memory.c
@@ -873,7 +873,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,