Skip to contentSkip to conversation

Revert "memory: Return -1 again on reads from unsigned regions"

PATCH In review Jan Kiszka 3 participants 6 messages Aug 3, 2013 – Aug 8, 2013 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jan Kiszka

author
#1 lore raw

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

From: Jan Kiszka <[email protected]>

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 <[email protected]>
---
 memory.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
memory.c +1 -1
index ac6f3c6..7a0251d 100644
@@ -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,
Signature · 1 line
1.7.3.4
Reviewed-by on v1 2/2
#2 lore raw
Quoted 8 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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 <[email protected]>

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 <[email protected]>

-- PMM

author
#3 lore raw
Quoted 8 lines · Peter Maydell
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 8 lines
Quoted 1 line
Signed-off-by Jan Kiszka <[email protected]>

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 <[email protected]>

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

Thanks,
Jan

#4 lore raw
In reply to Peter Maydell

Am 08.08.2013 17:23, schrieb Peter Maydell:

Quoted 8 lines · Jan Kiszka
Quoted 7 lines

From: Jan Kiszka <[email protected]>

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.

Quoted 6 lines
Quoted 1 line
Signed-off-by Jan Kiszka <[email protected]>

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

Quoted 5 lines
Reviewed-by Peter Maydell <[email protected]>

-- PMM

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
#5 lore raw
Quoted 5 lines · Andreas Färber

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

#6 lore raw

Am 08.08.2013 18:48, schrieb Peter Maydell:

Quoted 11 lines · Andreas Färber
Quoted 5 lines

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

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