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

[Peter Maydell]

On Tue, 18 Feb 2020 at 17:49, Guenter Roeck <address@hidden> 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