| Message ID | 20210420190002.383444-4-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Prepare for target-efi | expand |
On Tue, 20 Apr 2021 20:59:57 +0200 Andrew Jones <drjones@redhat.com> wrote: Hi, > Don't assume the physical addresses used with PCI have already been > identity mapped. > > Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > lib/pci-host-generic.c | 5 ++--- > lib/pci-host-generic.h | 4 ++-- > lib/pci-testdev.c | 4 ++++ > 3 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c > index 818150dc0a66..de93b8feac39 100644 > --- a/lib/pci-host-generic.c > +++ b/lib/pci-host-generic.c > @@ -122,7 +122,7 @@ static struct pci_host_bridge *pci_dt_probe(void) > sizeof(host->addr_space[0]) * nr_addr_spaces); > assert(host != NULL); > > - host->start = base.addr; > + host->start = ioremap(base.addr, base.size); > host->size = base.size; > host->bus = bus; > host->bus_max = bus_max; > @@ -279,8 +279,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr) > > static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn) > { > - return (void __iomem *)(unsigned long) > - host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); > + return (void __iomem *)host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); But host->start's type is now exactly "void __iomem *", so why the cast? And are we OK with doing pointer arithmetic on a void pointer? > } > > u8 pci_config_readb(pcidevaddr_t dev, u8 off) > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h > index fd30e7c74ed8..0ffe6380ec8f 100644 > --- a/lib/pci-host-generic.h > +++ b/lib/pci-host-generic.h > @@ -18,8 +18,8 @@ struct pci_addr_space { > }; > > struct pci_host_bridge { > - phys_addr_t start; > - phys_addr_t size; > + void __iomem *start; > + size_t size; > int bus; > int bus_max; > int nr_addr_spaces; > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c > index 039bb44781c1..4f2e5663b2d6 100644 > --- a/lib/pci-testdev.c > +++ b/lib/pci-testdev.c > @@ -185,7 +185,11 @@ int pci_testdev(void) > mem = ioremap(addr, PAGE_SIZE); > > addr = pci_bar_get_addr(&pci_dev, 1); > +#if defined(__i386__) || defined(__x86_64__) > io = (void *)(unsigned long)addr; > +#else > + io = ioremap(addr, PAGE_SIZE); > +#endif I am bit puzzled: For anything but x86 ioremap() is implemented like the first statement, so why do we differentiate here? Shouldn't either one of the statements be fine, for all architectures? Cheers, Andre > > nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops); > nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);
On Mon, Apr 26, 2021 at 04:03:26PM +0100, Andre Przywara wrote: > On Tue, 20 Apr 2021 20:59:57 +0200 > Andrew Jones <drjones@redhat.com> wrote: > > Hi, > > > Don't assume the physical addresses used with PCI have already been > > identity mapped. > > > > Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > lib/pci-host-generic.c | 5 ++--- > > lib/pci-host-generic.h | 4 ++-- > > lib/pci-testdev.c | 4 ++++ > > 3 files changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c > > index 818150dc0a66..de93b8feac39 100644 > > --- a/lib/pci-host-generic.c > > +++ b/lib/pci-host-generic.c > > @@ -122,7 +122,7 @@ static struct pci_host_bridge *pci_dt_probe(void) > > sizeof(host->addr_space[0]) * nr_addr_spaces); > > assert(host != NULL); > > > > - host->start = base.addr; > > + host->start = ioremap(base.addr, base.size); > > host->size = base.size; > > host->bus = bus; > > host->bus_max = bus_max; > > @@ -279,8 +279,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr) > > > > static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn) > > { > > - return (void __iomem *)(unsigned long) > > - host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); > > + return (void __iomem *)host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); > > But host->start's type is now exactly "void __iomem *", so why the cast? Only because I didn't think to remove it. Will do for v3. > And are we OK with doing pointer arithmetic on a void pointer? I'm pretty sure we have other cases, but if you'd prefer I can create a local char* for the arithmetic and then return it as a void*. (Assuming that's what you're suggesting I do.) > > > } > > > > u8 pci_config_readb(pcidevaddr_t dev, u8 off) > > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h > > index fd30e7c74ed8..0ffe6380ec8f 100644 > > --- a/lib/pci-host-generic.h > > +++ b/lib/pci-host-generic.h > > @@ -18,8 +18,8 @@ struct pci_addr_space { > > }; > > > > struct pci_host_bridge { > > - phys_addr_t start; > > - phys_addr_t size; > > + void __iomem *start; > > + size_t size; > > int bus; > > int bus_max; > > int nr_addr_spaces; > > diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c > > index 039bb44781c1..4f2e5663b2d6 100644 > > --- a/lib/pci-testdev.c > > +++ b/lib/pci-testdev.c > > @@ -185,7 +185,11 @@ int pci_testdev(void) > > mem = ioremap(addr, PAGE_SIZE); > > > > addr = pci_bar_get_addr(&pci_dev, 1); > > +#if defined(__i386__) || defined(__x86_64__) > > io = (void *)(unsigned long)addr; > > +#else > > + io = ioremap(addr, PAGE_SIZE); > > +#endif > > I am bit puzzled: For anything but x86 ioremap() is implemented like the > first statement, so why do we differentiate here? Shouldn't either one > of the statements be fine, for all architectures? The addresses in this context are pio. So x86 should use them verbatim, but other architectures that don't have pio will need to avoid them or remap them and use them with fake pio instructions (e.g. inb/outb wrappers for readb/writeb). Thanks, drew
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c index 818150dc0a66..de93b8feac39 100644 --- a/lib/pci-host-generic.c +++ b/lib/pci-host-generic.c @@ -122,7 +122,7 @@ static struct pci_host_bridge *pci_dt_probe(void) sizeof(host->addr_space[0]) * nr_addr_spaces); assert(host != NULL); - host->start = base.addr; + host->start = ioremap(base.addr, base.size); host->size = base.size; host->bus = bus; host->bus_max = bus_max; @@ -279,8 +279,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr) static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn) { - return (void __iomem *)(unsigned long) - host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); + return (void __iomem *)host->start + (devfn << PCI_ECAM_DEVFN_SHIFT); } u8 pci_config_readb(pcidevaddr_t dev, u8 off) diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h index fd30e7c74ed8..0ffe6380ec8f 100644 --- a/lib/pci-host-generic.h +++ b/lib/pci-host-generic.h @@ -18,8 +18,8 @@ struct pci_addr_space { }; struct pci_host_bridge { - phys_addr_t start; - phys_addr_t size; + void __iomem *start; + size_t size; int bus; int bus_max; int nr_addr_spaces; diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c index 039bb44781c1..4f2e5663b2d6 100644 --- a/lib/pci-testdev.c +++ b/lib/pci-testdev.c @@ -185,7 +185,11 @@ int pci_testdev(void) mem = ioremap(addr, PAGE_SIZE); addr = pci_bar_get_addr(&pci_dev, 1); +#if defined(__i386__) || defined(__x86_64__) io = (void *)(unsigned long)addr; +#else + io = ioremap(addr, PAGE_SIZE); +#endif nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops); nr_tests += pci_testdev_all(io, &pci_testdev_io_ops);