On Mon, Apr 11, 2016 at 01:04:24PM +0200, Alexander Gordeev wrote:
> This rework is a prerequisite for the forthcoming pci-testdev
> implementation. Basically, only generic port IO accessors are
> needed, but it turned out touching io/smp/mm files is needed.
> This update should likely be more comprehensive and split into
> several commits.
Yes. It should be split into more commits. And this commit message
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++----
> lib/x86/asm/page.h | 1 +
> lib/x86/io.h | 36 ++++++++++++++++++++++------------
> lib/x86/smp.h | 4 ----
> lib/x86/vm.c | 1 -
> lib/x86/vm.h | 12 +-----------
> x86/eventinj.c | 7 +------
> x86/kvmclock.c | 1 +
> 8 files changed, 79 insertions(+), 38 deletions(-)
> create mode 100644 lib/x86/asm/page.h
>
> diff --git a/lib/asm-generic/io.h b/lib/asm-generic/io.h
> index 49283d6eb020..bd78f3586b9e 100644
> --- a/lib/asm-generic/io.h
> +++ b/lib/asm-generic/io.h
> @@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x)
> ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })
> #define cpu_to_be64 be64_to_cpu
>
> +#ifndef mb
> +#define mb() do { } while (0)
> +#endif
> #ifndef rmb
> #define rmb() do { } while (0)
> #endif
> @@ -152,6 +155,48 @@ static inline u64 __bswap64(u64 x)
> #define writeq(b, addr) \
> ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); })
>
> +#ifndef inb
> +static inline uint8_t inb(unsigned long port)
> +{
> + return readb((const volatile void __iomem *)port);
> +}
> +#endif
> +
> +#ifndef inw
> +static inline uint16_t inw(unsigned long port)
> +{
> + return readw((const volatile void __iomem *)port);
> +}
> +#endif
> +
> +#ifndef inl
> +static inline uint32_t inl(unsigned long port)
> +{
> + return readl((const volatile void __iomem *)port);
> +}
> +#endif
> +
> +#ifndef outb
> +static inline void outb(uint8_t value, unsigned long port)
> +{
> + writeb(value, (volatile void __iomem *)port);
> +}
> +#endif
> +
> +#ifndef outw
> +static inline void outw(uint16_t value, unsigned long port)
> +{
> + writew(value, (volatile void __iomem *)port);
> +}
> +#endif
> +
> +#ifndef outl
> +static inline void outl(uint32_t value, unsigned long port)
> +{
> + writel(value, (volatile void __iomem *)port);
> +}
> +#endif
> +
What are all the above for? Are we going to base arch-neutral pci code
on these io-accessors? If so, why?
> #ifndef ioremap
> static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)
> {
> @@ -161,14 +206,16 @@ static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)
> #endif
>
> #ifndef virt_to_phys
> -static inline unsigned long virt_to_phys(volatile void *address)
> +static inline phys_addr_t virt_to_phys(volatile void *virt)
> {
> - return __pa((unsigned long)address);
> + return (phys_addr_t)virt;
> }
> +#endif
>
> -static inline void *phys_to_virt(unsigned long address)
> +#ifndef phys_to_virt
> +static inline void *phys_to_virt(phys_addr_t phys)
> {
> - return __va(address);
> + return (void*)phys;
> }
> #endif
Why change the generic virt_to_phys/phys_to_virt? I think all
architectures will want to define their own, in which case we
don't really need these anyway, but they're here now, and they
match Linux's asm-generic implementations.
>
> diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> new file mode 100644
> index 000000000000..1a8b62711f28
> --- /dev/null
> +++ b/lib/x86/asm/page.h
> @@ -0,0 +1 @@
> +#include <asm-generic/page.h>
> diff --git a/lib/x86/io.h b/lib/x86/io.h
> index 730786411fb3..e8e7b4aff23f 100644
> --- a/lib/x86/io.h
> +++ b/lib/x86/io.h
> @@ -3,43 +3,55 @@
>
> #define __iomem
>
> -static inline unsigned char inb(unsigned short port)
> +#define mb() asm volatile("mfence":::"memory")
> +#define rmb() asm volatile("lfence":::"memory")
> +#define wmb() asm volatile("sfence" ::: "memory")
> +
> +#define inb inb
> +static inline uint8_t inb(unsigned long port)
> {
> unsigned char value;
> - asm volatile("inb %w1, %0" : "=a" (value) : "Nd" (port));
> + asm volatile("inb %w1, %0" : "=a" (value) : "Nd" ((unsigned short)port));
> return value;
> }
>
> -static inline unsigned short inw(unsigned short port)
> +#define inw inw
> +static inline uint16_t inw(unsigned long port)
> {
> unsigned short value;
> - asm volatile("inw %w1, %0" : "=a" (value) : "Nd" (port));
> + asm volatile("inw %w1, %0" : "=a" (value) : "Nd" ((unsigned short)port));
> return value;
> }
>
> -static inline unsigned int inl(unsigned short port)
> +#define inl inl
> +static inline uint32_t inl(unsigned long port)
> {
> unsigned int value;
> - asm volatile("inl %w1, %0" : "=a" (value) : "Nd" (port));
> + asm volatile("inl %w1, %0" : "=a" (value) : "Nd" ((unsigned short)port));
> return value;
> }
>
> -static inline void outb(unsigned char value, unsigned short port)
> +#define outb outb
> +static inline void outb(uint8_t value, unsigned long port)
> {
> - asm volatile("outb %b0, %w1" : : "a"(value), "Nd"(port));
> + asm volatile("outb %b0, %w1" : : "a"(value), "Nd"((unsigned short)port));
> }
>
> -static inline void outw(unsigned short value, unsigned short port)
> +#define outw outw
> +static inline void outw(uint16_t value, unsigned long port)
> {
> - asm volatile("outw %w0, %w1" : : "a"(value), "Nd"(port));
> + asm volatile("outw %w0, %w1" : : "a"(value), "Nd"((unsigned short)port));
> }
>
> -static inline void outl(unsigned int value, unsigned short port)
> +#define outl outl
> +static inline void outl(uint32_t value, unsigned long port)
> {
> - asm volatile("outl %0, %w1" : : "a"(value), "Nd"(port));
> + asm volatile("outl %0, %w1" : : "a"(value), "Nd"((unsigned short)port));
> }
>
> #define ioremap ioremap
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused);
>
> +#include <asm-generic/io.h>
let's avoid doing this until x86 needs something from asm-generic/io.h
> +
> #endif
> diff --git a/lib/x86/smp.h b/lib/x86/smp.h
> index 566018f49ba3..afabac8495f1 100644
> --- a/lib/x86/smp.h
> +++ b/lib/x86/smp.h
> @@ -2,10 +2,6 @@
> #define __SMP_H
> #include <asm/spinlock.h>
>
> -#define mb() asm volatile("mfence":::"memory")
> -#define rmb() asm volatile("lfence":::"memory")
> -#define wmb() asm volatile("sfence" ::: "memory")
> -
> void smp_init(void);
>
> int cpu_count(void);
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 7ce7bbc03eef..f19654993350 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -2,7 +2,6 @@
> #include "vm.h"
> #include "libcflat.h"
>
> -#define PAGE_SIZE 4096ul
> #ifdef __x86_64__
> #define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
> #else
> diff --git a/lib/x86/vm.h b/lib/x86/vm.h
> index 28794d7f26c6..0e15291ee3a4 100644
> --- a/lib/x86/vm.h
> +++ b/lib/x86/vm.h
> @@ -2,8 +2,8 @@
> #define VM_H
>
> #include "processor.h"
> +#include "io.h"
>
> -#define PAGE_SIZE 4096ul
Strange. You've removed two definitions of PAGE_SIZE. I think that
removes them all. How does x86 still compile after this patch?
> #ifdef __x86_64__
> #define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
> #else
> @@ -39,14 +39,4 @@ unsigned long *install_large_page(unsigned long *cr3,unsigned long phys,
> void *virt);
> unsigned long *install_page(unsigned long *cr3, unsigned long phys, void *virt);
>
> -static inline unsigned long virt_to_phys(const void *virt)
> -{
> - return (unsigned long)virt;
> -}
> -
> -static inline void *phys_to_virt(unsigned long phys)
> -{
> - return (void *)phys;
> -}
> -
Uh oh. You've now changed x86's virt_to_phys/phys_to_virt to using
phys_addr_t. The unit tests may not be happy!
> #endif
> diff --git a/x86/eventinj.c b/x86/eventinj.c
> index 57c2a2d0cf28..84dfe71d8e2a 100644
> --- a/x86/eventinj.c
> +++ b/x86/eventinj.c
> @@ -16,11 +16,6 @@ static inline void io_delay(void)
> {
> }
>
> -static inline void outl(int addr, int val)
> -{
> - asm volatile ("outl %1, %w0" : : "d" (addr), "a" (val));
> -}
> -
> void apic_self_ipi(u8 v)
> {
> apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED |
> @@ -32,7 +27,7 @@ void apic_self_nmi(void)
> apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_NMI | APIC_INT_ASSERT, 0);
> }
>
> -#define flush_phys_addr(__s) outl(0xe4, __s)
> +#define flush_phys_addr(__s) outl(__s, 0xe4)
> #define flush_stack() do { \
> int __l; \
> flush_phys_addr(virt_to_phys(&__l)); \
> diff --git a/x86/kvmclock.c b/x86/kvmclock.c
> index 327e60d34213..b0be2a41296d 100644
> --- a/x86/kvmclock.c
> +++ b/x86/kvmclock.c
> @@ -1,4 +1,5 @@
> #include "libcflat.h"
> +#include "io.h"
why add this include?
> #include "smp.h"
> #include "atomic.h"
> #include "processor.h"
> --
> 1.8.3.1
>
I guess I'll see in the next patches what the motivation for this one
is, but at the moment I'd say just completely drop this one.
thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
