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

[Guenter Roeck]

On Tue, Feb 18, 2020 at 04:33:49PM +0000, Peter Maydell wrote:
> 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.
> 

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