| Message ID | 20200217203734.18703-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh4: Remove bad memory alias 'sh_pci.isa' | expand |
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
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;
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(-)