On Fri, Apr 22, 2016 at 05:54:12PM +0200, Andrew Jones wrote:
> 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?
I am not sure I got your question, but I will answer as if I did :)
We need in[bwl]/out[bwl] to access devices' port IO space. Out of
all archs in kvm-unit-tests it is only x86 that uses dedicated
instructions and overrides the generic ones. All others do it
as Linux IO generics do - using read[bwl]/write[bwl].
> > #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.
Right. In my new version I preserved these once by just adding
missing __va/__pa to x86 in "x86: Cleanup low-level arch code"
series.
> >
> > 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.
Yes, sorry I did not make it even clearer. This patch is to
enable following changes. I hoped to gather some ideas on
proper approach, but it is completely superceded by the
series "x86: Cleanup low-level arch code".
> 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
