| Message ID | 1373951995-9866-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto: > In the past, IO space could not be mapped into the memory address space > so we introduced a workaround for that. Nowadays it does not look > necessary so we can remove the workaround and make sPAPR PCI > configuration simplier. > > This also removes all byte swappings as it is not PHB's to take care > of endiannes - devices should do convertion if they want to. And almost > every PCI device which uses IO ports does that by registering IO ports > region with memory_region_init_io() (Intel e1000, RTL8139, virtio). > > However VGA uses MemoryRegionPortio which does not support endiannes > but it still expects the convertion to be done. For this case only, > this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show > that other devices are not affected by this change. x86 systems should > not suffer either as having LITTLE_ENDIAN there has no effect. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > This removes bugs at least from SPAPR so any further fixes should be equal > for all platforms and hopefully will break all platform altogether but not > just PPC64-pSeries :) > > Did I miss anything here? No, I don't think so. The patch looks good. Paolo > > --- > hw/ppc/spapr_pci.c | 52 +++------------------------------------------------- > ioport.c | 1 + > 2 files changed, 4 insertions(+), 49 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index ca588aa..8d76290 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level) > qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level); > } > > -static uint64_t spapr_io_read(void *opaque, hwaddr addr, > - unsigned size) > -{ > - switch (size) { > - case 1: > - return cpu_inb(addr); > - case 2: > - return cpu_inw(addr); > - case 4: > - return cpu_inl(addr); > - } > - g_assert_not_reached(); > -} > - > -static void spapr_io_write(void *opaque, hwaddr addr, > - uint64_t data, unsigned size) > -{ > - switch (size) { > - case 1: > - cpu_outb(addr, data); > - return; > - case 2: > - cpu_outw(addr, data); > - return; > - case 4: > - cpu_outl(addr, data); > - return; > - } > - g_assert_not_reached(); > -} > - > -static const MemoryRegionOps spapr_io_ops = { > - .endianness = DEVICE_LITTLE_ENDIAN, > - .read = spapr_io_read, > - .write = spapr_io_write > -}; > - > /* > * MSI/MSIX memory region implementation. > * The handler handles both MSI and MSIX. > @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s) > memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr, > &sphb->memwindow); > > - /* On ppc, we only have MMIO no specific IO space from the CPU > - * perspective. In theory we ought to be able to embed the PCI IO > - * memory region direction in the system memory space. However, > - * if any of the IO BAR subregions use the old_portio mechanism, > - * that won't be processed properly unless accessed from the > - * system io address space. This hack to bounce things via > - * system_io works around the problem until all the users of > - * old_portion are updated */ > + /* Initialize IO regions */ > sprintf(namebuf, "%s.io", sphb->dtbusname); > memory_region_init(&sphb->iospace, OBJECT(sphb), > namebuf, SPAPR_PCI_IO_WIN_SIZE); > - /* FIXME: fix to support multiple PHBs */ > - memory_region_add_subregion(get_system_io(), 0, &sphb->iospace); > > sprintf(namebuf, "%s.io-alias", sphb->dtbusname); > - memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb, > - namebuf, SPAPR_PCI_IO_WIN_SIZE); > + memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), > + namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE); > memory_region_add_subregion(get_system_memory(), sphb->io_win_addr, > &sphb->iowindow); > > diff --git a/ioport.c b/ioport.c > index 89b17d6..79b7f1a 100644 > --- a/ioport.c > +++ b/ioport.c > @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data, > static const MemoryRegionOps portio_ops = { > .read = portio_read, > .write = portio_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > .valid.unaligned = true, > .impl.unaligned = true, > }; >
Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>: > Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto: >> In the past, IO space could not be mapped into the memory address space >> so we introduced a workaround for that. Nowadays it does not look >> necessary so we can remove the workaround and make sPAPR PCI >> configuration simplier. >> >> This also removes all byte swappings as it is not PHB's to take care >> of endiannes - devices should do convertion if they want to. And almost >> every PCI device which uses IO ports does that by registering IO ports >> region with memory_region_init_io() (Intel e1000, RTL8139, virtio). >> >> However VGA uses MemoryRegionPortio which does not support endiannes >> but it still expects the convertion to be done. For this case only, >> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show >> that other devices are not affected by this change. x86 systems should >> not suffer either as having LITTLE_ENDIAN there has no effect. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> >> This removes bugs at least from SPAPR so any further fixes should be equal >> for all platforms and hopefully will break all platform altogether but not >> just PPC64-pSeries :) >> >> Did I miss anything here? > > No, I don't think so. The patch looks good. ... and will break all Mac targets again, no? Not to speak of non-ppc devices. Alex > > Paolo > >> >> --- >> hw/ppc/spapr_pci.c | 52 +++------------------------------------------------- >> ioport.c | 1 + >> 2 files changed, 4 insertions(+), 49 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index ca588aa..8d76290 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level) >> qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level); >> } >> >> -static uint64_t spapr_io_read(void *opaque, hwaddr addr, >> - unsigned size) >> -{ >> - switch (size) { >> - case 1: >> - return cpu_inb(addr); >> - case 2: >> - return cpu_inw(addr); >> - case 4: >> - return cpu_inl(addr); >> - } >> - g_assert_not_reached(); >> -} >> - >> -static void spapr_io_write(void *opaque, hwaddr addr, >> - uint64_t data, unsigned size) >> -{ >> - switch (size) { >> - case 1: >> - cpu_outb(addr, data); >> - return; >> - case 2: >> - cpu_outw(addr, data); >> - return; >> - case 4: >> - cpu_outl(addr, data); >> - return; >> - } >> - g_assert_not_reached(); >> -} >> - >> -static const MemoryRegionOps spapr_io_ops = { >> - .endianness = DEVICE_LITTLE_ENDIAN, >> - .read = spapr_io_read, >> - .write = spapr_io_write >> -}; >> - >> /* >> * MSI/MSIX memory region implementation. >> * The handler handles both MSI and MSIX. >> @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s) >> memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr, >> &sphb->memwindow); >> >> - /* On ppc, we only have MMIO no specific IO space from the CPU >> - * perspective. In theory we ought to be able to embed the PCI IO >> - * memory region direction in the system memory space. However, >> - * if any of the IO BAR subregions use the old_portio mechanism, >> - * that won't be processed properly unless accessed from the >> - * system io address space. This hack to bounce things via >> - * system_io works around the problem until all the users of >> - * old_portion are updated */ >> + /* Initialize IO regions */ >> sprintf(namebuf, "%s.io", sphb->dtbusname); >> memory_region_init(&sphb->iospace, OBJECT(sphb), >> namebuf, SPAPR_PCI_IO_WIN_SIZE); >> - /* FIXME: fix to support multiple PHBs */ >> - memory_region_add_subregion(get_system_io(), 0, &sphb->iospace); >> >> sprintf(namebuf, "%s.io-alias", sphb->dtbusname); >> - memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb, >> - namebuf, SPAPR_PCI_IO_WIN_SIZE); >> + memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), >> + namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE); >> memory_region_add_subregion(get_system_memory(), sphb->io_win_addr, >> &sphb->iowindow); >> >> diff --git a/ioport.c b/ioport.c >> index 89b17d6..79b7f1a 100644 >> --- a/ioport.c >> +++ b/ioport.c >> @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data, >> static const MemoryRegionOps portio_ops = { >> .read = portio_read, >> .write = portio_write, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> .valid.unaligned = true, >> .impl.unaligned = true, >> }; >
On 07/16/2013 06:32 PM, Alexander Graf wrote: > > > Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>: > >> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto: >>> In the past, IO space could not be mapped into the memory address space >>> so we introduced a workaround for that. Nowadays it does not look >>> necessary so we can remove the workaround and make sPAPR PCI >>> configuration simplier. >>> >>> This also removes all byte swappings as it is not PHB's to take care >>> of endiannes - devices should do convertion if they want to. And almost >>> every PCI device which uses IO ports does that by registering IO ports >>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio). >>> >>> However VGA uses MemoryRegionPortio which does not support endiannes >>> but it still expects the convertion to be done. For this case only, >>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show >>> that other devices are not affected by this change. x86 systems should >>> not suffer either as having LITTLE_ENDIAN there has no effect. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> >>> This removes bugs at least from SPAPR so any further fixes should be equal >>> for all platforms and hopefully will break all platform altogether but not >>> just PPC64-pSeries :) >>> >>> Did I miss anything here? >> >> No, I don't think so. The patch looks good. > > ... and will break all Mac targets again, no? Not to speak of non-ppc devices. Like what? I do not mind/argue/discuss, may be it breaks, I am just looking for good examples to test.
Am 16.07.2013 um 10:37 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>: > On 07/16/2013 06:32 PM, Alexander Graf wrote: >> >> >> Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonzini@redhat.com>: >> >>> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto: >>>> In the past, IO space could not be mapped into the memory address space >>>> so we introduced a workaround for that. Nowadays it does not look >>>> necessary so we can remove the workaround and make sPAPR PCI >>>> configuration simplier. >>>> >>>> This also removes all byte swappings as it is not PHB's to take care >>>> of endiannes - devices should do convertion if they want to. And almost >>>> every PCI device which uses IO ports does that by registering IO ports >>>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio). >>>> >>>> However VGA uses MemoryRegionPortio which does not support endiannes >>>> but it still expects the convertion to be done. For this case only, >>>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show >>>> that other devices are not affected by this change. x86 systems should >>>> not suffer either as having LITTLE_ENDIAN there has no effect. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> >>>> This removes bugs at least from SPAPR so any further fixes should be equal >>>> for all platforms and hopefully will break all platform altogether but not >>>> just PPC64-pSeries :) >>>> >>>> Did I miss anything here? >>> >>> No, I don't think so. The patch looks good. >> >> ... and will break all Mac targets again, no? Not to speak of non-ppc devices. > > > Like what? I do not mind/argue/discuss, may be it breaks, I am just looking > for good examples to test. How about you start with $ qemu-system-ppc Then? It shouldn't show a prompt with your patch ;) Alex > > > > -- > Alexey
On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote: > >> Did I miss anything here? > > > > No, I don't think so. The patch looks good. > > ... and will break all Mac targets again, no? Not to speak of non-ppc > devices. Do you know why it breaks ? Can you suggest what's wrong and how to fix it maybe ? Cheers, Ben,
Am 16.07.2013 um 10:45 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>: > On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote: >>>> Did I miss anything here? >>> >>> No, I don't think so. The patch looks good. >> >> ... and will break all Mac targets again, no? Not to speak of non-ppc >> devices. > > Do you know why it breaks ? Can you suggest what's wrong and how to fix > it maybe ? Sure. Get rid of cpu_inX and cpu_outX completely and verify that the conversion is doing the "right thing" for each conversion. Some of them are really tricky - like MIPS which accesses the RTC in native endian (big) in current code. So this patch would definitely break that one too. I'm actually surprised we have to mark the ioport region little endian for sPAPR. We already did have it converted to a direct memory api alias a while ago and IIRC the only problem back then were the old_portio callbacks. I wonder what changed in between. Alex > > Cheers, > Ben, > >
On 07/16/2013 07:01 PM, Alexander Graf wrote: > > > Am 16.07.2013 um 10:45 schrieb Benjamin Herrenschmidt > <benh@kernel.crashing.org>: > >> On Tue, 2013-07-16 at 10:32 +0200, Alexander Graf wrote: >>>>> Did I miss anything here? >>>> >>>> No, I don't think so. The patch looks good. >>> >>> ... and will break all Mac targets again, no? Not to speak of >>> non-ppc devices. >> >> Do you know why it breaks ? Can you suggest what's wrong and how to >> fix it maybe ? > > Sure. Get rid of cpu_inX and cpu_outX completely and verify that the > conversion is doing the "right thing" for each conversion. Some of them > are really tricky - like MIPS which accesses the RTC in native endian > (big) in current code. So this patch would definitely break that one > too. > > I'm actually surprised we have to mark the ioport region little endian > for sPAPR. It is not for spapr, it is for devices which expect some endianness and do not do convertions. > We already did have it converted to a direct memory api alias > a while ago and IIRC the only problem back then were the old_portio > callbacks. I wonder what changed in between. ? We have had the IO memory region hack I am trying to remove for a long time.
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index ca588aa..8d76290 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level) qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level); } -static uint64_t spapr_io_read(void *opaque, hwaddr addr, - unsigned size) -{ - switch (size) { - case 1: - return cpu_inb(addr); - case 2: - return cpu_inw(addr); - case 4: - return cpu_inl(addr); - } - g_assert_not_reached(); -} - -static void spapr_io_write(void *opaque, hwaddr addr, - uint64_t data, unsigned size) -{ - switch (size) { - case 1: - cpu_outb(addr, data); - return; - case 2: - cpu_outw(addr, data); - return; - case 4: - cpu_outl(addr, data); - return; - } - g_assert_not_reached(); -} - -static const MemoryRegionOps spapr_io_ops = { - .endianness = DEVICE_LITTLE_ENDIAN, - .read = spapr_io_read, - .write = spapr_io_write -}; - /* * MSI/MSIX memory region implementation. * The handler handles both MSI and MSIX. @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s) memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr, &sphb->memwindow); - /* On ppc, we only have MMIO no specific IO space from the CPU - * perspective. In theory we ought to be able to embed the PCI IO - * memory region direction in the system memory space. However, - * if any of the IO BAR subregions use the old_portio mechanism, - * that won't be processed properly unless accessed from the - * system io address space. This hack to bounce things via - * system_io works around the problem until all the users of - * old_portion are updated */ + /* Initialize IO regions */ sprintf(namebuf, "%s.io", sphb->dtbusname); memory_region_init(&sphb->iospace, OBJECT(sphb), namebuf, SPAPR_PCI_IO_WIN_SIZE); - /* FIXME: fix to support multiple PHBs */ - memory_region_add_subregion(get_system_io(), 0, &sphb->iospace); sprintf(namebuf, "%s.io-alias", sphb->dtbusname); - memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops, sphb, - namebuf, SPAPR_PCI_IO_WIN_SIZE); + memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), + namebuf, &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE); memory_region_add_subregion(get_system_memory(), sphb->io_win_addr, &sphb->iowindow); diff --git a/ioport.c b/ioport.c index 89b17d6..79b7f1a 100644 --- a/ioport.c +++ b/ioport.c @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data, static const MemoryRegionOps portio_ops = { .read = portio_read, .write = portio_write, + .endianness = DEVICE_LITTLE_ENDIAN, .valid.unaligned = true, .impl.unaligned = true, };
In the past, IO space could not be mapped into the memory address space so we introduced a workaround for that. Nowadays it does not look necessary so we can remove the workaround and make sPAPR PCI configuration simplier. This also removes all byte swappings as it is not PHB's to take care of endiannes - devices should do convertion if they want to. And almost every PCI device which uses IO ports does that by registering IO ports region with memory_region_init_io() (Intel e1000, RTL8139, virtio). However VGA uses MemoryRegionPortio which does not support endiannes but it still expects the convertion to be done. For this case only, this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show that other devices are not affected by this change. x86 systems should not suffer either as having LITTLE_ENDIAN there has no effect. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- This removes bugs at least from SPAPR so any further fixes should be equal for all platforms and hopefully will break all platform altogether but not just PPC64-pSeries :) Did I miss anything here? --- hw/ppc/spapr_pci.c | 52 +++------------------------------------------------- ioport.c | 1 + 2 files changed, 4 insertions(+), 49 deletions(-)