[v2] sh4: Fix PCI ISA IO memory subregion

Commit Message

Guenter Roeck Feb. 18, 2020, 8:10 p.m. UTC

Booting the r2d machine from flash fails because flash is not discovered.
Looking at the flattened memory tree, we see the following.

FlatView #1
 AS "memory", root: system
 AS "cpu-memory-0", root: system
 AS "sh_pci_host", root: bus master container
 Root memory region: system
  0000000000000000-000000000000ffff (prio 0, i/o): io
  0000000000010000-0000000000ffffff (prio 0, i/o): r2d.flash @0000000000010000

The overlapping memory region is sh_pci.isa, ie the ISA I/O region bridge.
This region is initially assigned to address 0xfe240000, but overwritten
with a write into the PCIIOBR register. This write is expected to adjust
the PCI memory window, but not to change the region's base adddress.

Peter Maydell provided the following detailed explanation.

"Section 22.3.7 and in particular figure 22.3 (of "SSH7751R user's manual:
hardware") are clear about how this is supposed to work: there is a window
at 0xfe240000 in the system register space for PCI I/O space. When the CPU
makes an access into that area, the PCI controller calculates the PCI
address to use by combining bits 0..17 of the system address with the
bits 31..18 value that the guest has put into the PCIIOBR. That is, writing
to the PCIIOBR changes which section of the IO address space is visible in
the 0xfe240000 window. Instead what QEMU's implementation does is move the
window to whatever value the guest writes to the PCIIOBR register -- so if
the guest writes 0 we put the window at 0 in system address space."

Fix the problem by calling memory_region_set_alias_offset() instead of
removing and re-adding the PCI ISA subregion on writes into PCIIOBR.
At the same time, in sh_pci_device_realize(), don't set iobr since
it is overwritten later anyway. Instead, pass the base address to
memory_region_add_subregion() directly.

Many thanks to Peter Maydell for the detailed problem analysis, and for
providing suggestions on how to fix the problem.

Signed-off-by: Guenter Roeck <[e-mail]>
---
v2: Complete rework based on Peter's analysis. Don't remove the
    'sh_pci.isa' alias, which was perfectly fine.
    Instead, fix the underlying problem.
    Rename subject from "'sh4: Remove bad memory alias 'sh_pci.isa'"

 hw/sh4/sh_pci.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Peter Maydell Feb. 20, 2020, 3:06 p.m. UTC | #1

On Tue, 18 Feb 2020 at 20:10, Guenter Roeck <[e-mail]> wrote:
>
> Booting the r2d machine from flash fails because flash is not discovered.
> Looking at the flattened memory tree, we see the following.
>
> FlatView #1
>  AS "memory", root: system
>  AS "cpu-memory-0", root: system
>  AS "sh_pci_host", root: bus master container
>  Root memory region: system
>   0000000000000000-000000000000ffff (prio 0, i/o): io
>   0000000000010000-0000000000ffffff (prio 0, i/o): r2d.flash @0000000000010000
>
> The overlapping memory region is sh_pci.isa, ie the ISA I/O region bridge.
> This region is initially assigned to address 0xfe240000, but overwritten
> with a write into the PCIIOBR register. This write is expected to adjust
> the PCI memory window, but not to change the region's base adddress.
>
> Peter Maydell provided the following detailed explanation.
>
> "Section 22.3.7 and in particular figure 22.3 (of "SSH7751R user's manual:
> hardware") are clear about how this is supposed to work: there is a window
> at 0xfe240000 in the system register space for PCI I/O space. When the CPU
> makes an access into that area, the PCI controller calculates the PCI
> address to use by combining bits 0..17 of the system address with the
> bits 31..18 value that the guest has put into the PCIIOBR. That is, writing
> to the PCIIOBR changes which section of the IO address space is visible in
> the 0xfe240000 window. Instead what QEMU's implementation does is move the
> window to whatever value the guest writes to the PCIIOBR register -- so if
> the guest writes 0 we put the window at 0 in system address space."
>
> Fix the problem by calling memory_region_set_alias_offset() instead of
> removing and re-adding the PCI ISA subregion on writes into PCIIOBR.
> At the same time, in sh_pci_device_realize(), don't set iobr since
> it is overwritten later anyway. Instead, pass the base address to
> memory_region_add_subregion() directly.
>
> Many thanks to Peter Maydell for the detailed problem analysis, and for
> providing suggestions on how to fix the problem.
>
> Signed-off-by: Guenter Roeck <[e-mail]>

Reviewed-by: Peter Maydell <[e-mail]>

I'll put this in via target-arm.next, since we don't really
have a more active sh4-specific tree to send it via.

thanks
-- PMM

Guenter Roeck Feb. 20, 2020, 9:26 p.m. UTC | #2

Hi Peter,

On Thu, Feb 20, 2020 at 03:06:05PM +0000, Peter Maydell wrote:
> On Tue, 18 Feb 2020 at 20:10, Guenter Roeck <[e-mail]> wrote:
> 
> I'll put this in via target-arm.next, since we don't really
> have a more active sh4-specific tree to send it via.
> 

Thanks for picking up all my patches, and even more thanks for
your patient reviews and for correcting all my errors.

Guenter

Patch

diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index 71afd23b67..08f2fc1dde 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -67,12 +67,8 @@  static void sh_pci_reg_write (void *p, hwaddr addr, uint64_t val,
         pcic->mbr = val & 0xff000001;
         break;
     case 0x1c8:
-        if ((val & 0xfffc0000) != (pcic->iobr & 0xfffc0000)) {
-            memory_region_del_subregion(get_system_memory(), &pcic->isa);
-            pcic->iobr = val & 0xfffc0001;
-            memory_region_add_subregion(get_system_memory(),
-                                        pcic->iobr & 0xfffc0000, &pcic->isa);
-        }
+        pcic->iobr = val & 0xfffc0001;
+        memory_region_set_alias_offset(&pcic->isa, val & 0xfffc0000);
         break;
     case 0x220:
         pci_data_write(phb->bus, pcic->par, val, 4);
@@ -147,8 +143,7 @@  static void sh_pci_device_realize(DeviceState *dev, Error **errp)
                              get_system_io(), 0, 0x40000);
     sysbus_init_mmio(sbd, &s->memconfig_p4);
     sysbus_init_mmio(sbd, &s->memconfig_a7);
-    s->iobr = 0xfe240000;
-    memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
+    memory_region_add_subregion(get_system_memory(), 0xfe240000, &s->isa);
 
     s->dev = pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "sh_pci_host");
 }