Re: [PATCH] sh4: Remove bad memory alias 'sh_pci.isa'

[Peter Maydell]

On Mon, 17 Feb 2020 at 20:38, Guenter Roeck <address@hidden> 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 <address@hidden>
> ---
>  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