sh4: Remove bad memory alias 'sh_pci.isa'

The memory alias sh_pci.isa is located at address 0x0000 with
a length of 0x40000. This results in the following memory tree.

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 alias overlaps with flash memory. As result, flash memory can
not be accessed. Removing the alias fixes the problem. With this patch,
the memory tree is as follows, and flash is detected as expected.

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-0000000000ffffff (prio 0, i/o): r2d.flash

After this patch has been applied, access to PCI, ATA, and USB is still
working, and no negative impact has ben observed.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/sh4/sh_pci.c | 2 --
 1 file changed, 2 deletions(-)

On Mon, 17 Feb 2020 at 20:38, Guenter Roeck <linux@roeck-us.net> wrote:
>
> The memory alias sh_pci.isa is located at address 0x0000 with
> a length of 0x40000. This results in the following memory tree.
>
> 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 alias overlaps with flash memory. As result, flash memory can
> not be accessed. Removing the alias fixes the problem. With this patch,
> the memory tree is as follows, and flash is detected as expected.
>
> 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-0000000000ffffff (prio 0, i/o): r2d.flash
>
> After this patch has been applied, access to PCI, ATA, and USB is still
> working, and no negative impact has ben observed.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/sh4/sh_pci.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index 71afd23b67..4ced54f1a5 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -143,8 +143,6 @@ static void sh_pci_device_realize(DeviceState *dev, Error **errp)
>                            "sh_pci", 0x224);
>      memory_region_init_alias(&s->memconfig_a7, OBJECT(s), "sh_pci.2",
>                               &s->memconfig_p4, 0, 0x224);
> -    memory_region_init_alias(&s->isa, OBJECT(s), "sh_pci.isa",
> -                             get_system_io(), 0, 0x40000);
>      sysbus_init_mmio(sbd, &s->memconfig_p4);
>      sysbus_init_mmio(sbd, &s->memconfig_a7);
>      s->iobr = 0xfe240000;
> --

This change doesn't look correct. This removes the init of the alias MR,
but we continue to use it in other parts of the code -- we will
add it to the system memory at 0xfe240000 and we will remap it
at different addresses when the guest writes to the 0x1c8 "IO space
base" register.

This alias is for the ISA I/O region bridge; the code initially
maps it at a non-zero address:
    s->iobr = 0xfe240000;
    memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
so it's not entirely clear how it ends up at 0. You could try
sticking a printf into sh_pci_reg_write() to see if we end
up taking that code path to modify the address for it, which
is the most plausible reason for it to be at 0, I think.

I think the problem here is that our implementation of the
IO space base register is simply completely wrong.

Conveniently, the SSH7751R "user's manual: hardware" seems to
still be downloadable from Renesas at
https://www.renesas.com/br/en/document/hw-manual?hwLayerShowFlg=true&prdLayerId=1057&layerName=SH7751R&coronrService=document-prd-search&hwDocUrl=%2Fpt-br%2Fdoc%2Fproducts%2Fmpumcu%2F001%2Fr01uh0457ej0401_sh7751.pdf&hashKey=a503c1946f0bcc913aaf89f48dd15b53
-- hopefully that URL
works for others and not just me.

Section 22.3.7 and in particular figure 22.3 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.

I think the correct fix would be to have the handling of
PCIIOBR call memory_region_set_alias_offset() (thus updating
where this alias window points within the system IO space),
rather than doing the del/add subregion calls.

thanks
-- PMM

On Tue, Feb 18, 2020 at 04:33:49PM +0000, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 20:38, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > The memory alias sh_pci.isa is located at address 0x0000 with
> > a length of 0x40000. This results in the following memory tree.
> >
> > 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 alias overlaps with flash memory. As result, flash memory can
> > not be accessed. Removing the alias fixes the problem. With this patch,
> > the memory tree is as follows, and flash is detected as expected.
> >
> > 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-0000000000ffffff (prio 0, i/o): r2d.flash
> >
> > After this patch has been applied, access to PCI, ATA, and USB is still
> > working, and no negative impact has ben observed.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/sh4/sh_pci.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> > index 71afd23b67..4ced54f1a5 100644
> > --- a/hw/sh4/sh_pci.c
> > +++ b/hw/sh4/sh_pci.c
> > @@ -143,8 +143,6 @@ static void sh_pci_device_realize(DeviceState *dev, Error **errp)
> >                            "sh_pci", 0x224);
> >      memory_region_init_alias(&s->memconfig_a7, OBJECT(s), "sh_pci.2",
> >                               &s->memconfig_p4, 0, 0x224);
> > -    memory_region_init_alias(&s->isa, OBJECT(s), "sh_pci.isa",
> > -                             get_system_io(), 0, 0x40000);
> >      sysbus_init_mmio(sbd, &s->memconfig_p4);
> >      sysbus_init_mmio(sbd, &s->memconfig_a7);
> >      s->iobr = 0xfe240000;
> > --
> 
> This change doesn't look correct. This removes the init of the alias MR,
> but we continue to use it in other parts of the code -- we will
> add it to the system memory at 0xfe240000 and we will remap it
> at different addresses when the guest writes to the 0x1c8 "IO space
> base" register.
> 
> This alias is for the ISA I/O region bridge; the code initially
> maps it at a non-zero address:
>     s->iobr = 0xfe240000;
>     memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
> so it's not entirely clear how it ends up at 0. You could try
> sticking a printf into sh_pci_reg_write() to see if we end
> up taking that code path to modify the address for it, which
> is the most plausible reason for it to be at 0, I think.
> 

Yes, that is what happens.

###### sh_pci_reg_write(addr=0x1c8, val=0x0, size=4)

> I think the problem here is that our implementation of the
> IO space base register is simply completely wrong.
> 
> Conveniently, the SSH7751R "user's manual: hardware" seems to
> still be downloadable from Renesas at
> https://www.renesas.com/br/en/document/hw-manual?hwLayerShowFlg=true&prdLayerId=1057&layerName=SH7751R&coronrService=document-prd-search&hwDocUrl=%2Fpt-br%2Fdoc%2Fproducts%2Fmpumcu%2F001%2Fr01uh0457ej0401_sh7751.pdf&hashKey=a503c1946f0bcc913aaf89f48dd15b53
> -- hopefully that URL
> works for others and not just me.
> 
> Section 22.3.7 and in particular figure 22.3 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.
> 
> I think the correct fix would be to have the handling of
> PCIIOBR call memory_region_set_alias_offset() (thus updating
> where this alias window points within the system IO space),
> rather than doing the del/add subregion calls.
> 
Like this ?

--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -67,12 +67,7 @@ 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);
-        }
+        memory_region_set_alias_offset(&pcic->isa, val & 0xfffc0000);
         break;

This works for me as well. Please let me know if it is correct (especially
the mask), and I'll resubmit.

Thanks,
Guenter

On Tue, 18 Feb 2020 at 17:49, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Feb 18, 2020 at 04:33:49PM +0000, Peter Maydell wrote:
> > I think the correct fix would be to have the handling of
> > PCIIOBR call memory_region_set_alias_offset() (thus updating
> > where this alias window points within the system IO space),
> > rather than doing the del/add subregion calls.
> >
> Like this ?
>
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -67,12 +67,7 @@ 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);
> -        }
> +        memory_region_set_alias_offset(&pcic->isa, val & 0xfffc0000);
>          break;
>
> This works for me as well. Please let me know if it is correct (especially
> the mask), and I'll resubmit.

The mask and call to set_alias_offset look right, but you have
lost the setting of pcic->iobr, which is necessary so that the
register reads back correctly.

We should also drop the
    s->iobr = 0xfe240000;
in sh_pci_device_realize(), as the register resets to zero,
and just hardcode the 0xfe240000 as the argument to
memory_region_add_subregion() in that function.

(A better place for that add_subregion would be to put it
in the board model, and just have this pci controller
device expose the alias window as a sysbus mmio region,
the same way we do with the a7 and p4 windows,
but that's a separate cleanup from this bugfix.)

Incidentally, the device is missing a reset method, but
I guess Linux is programming the whole controller from
scratch and not relying on any of the registers having
known values out of reset.

thanks
-- PMM