diff mbox series

[kvm-unit-tests,v2,3/8] pci-testdev: ioremap regions

Message ID 20210420190002.383444-4-drjones@redhat.com (mailing list archive)
State New
Headers show
Series arm/arm64: Prepare for target-efi | expand

Commit Message

Andrew Jones April 20, 2021, 6:59 p.m. UTC
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(-)

Comments

Andre Przywara April 26, 2021, 3:03 p.m. UTC | #1
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);
Andrew Jones April 26, 2021, 4:25 p.m. UTC | #2
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 mbox series

Patch

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);