Re: [kvm-unit-tests PATCH v3 09/10] io/x86: Factor out ioremap()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 29, 2016 at 04:48:52PM +0200, Andrew Jones wrote:
> > diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> > index 54f6471..db834ba 100644
> > --- a/lib/x86/asm/page.h
> > +++ b/lib/x86/asm/page.h
> > @@ -7,6 +7,7 @@
> >  #else
> >  #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
> >  #endif
> > +#define PAGE_MASK   (~(PAGE_SIZE-1))
> 
> Hmm... I see two x86 unit tests have their own PAGE_MASK
> definitions (x86/vmx.h, x86/access.c). I agree it should
> be in asm/page.h instead, but looks like more cleaning is
> in order then...

Yes, I am in a rush to get PCI stuff done and thought those
two could wait.

> > diff --git a/lib/x86/io.c b/lib/x86/io.c
> > index d396d42..9a84e6b 100644
> > --- a/lib/x86/io.c
> > +++ b/lib/x86/io.c
> > @@ -1,6 +1,6 @@
> >  #include "libcflat.h"
> > +#include "vm.h"
> >  #include "smp.h"
> > -#include "asm/io.h"
> 
> Why remove this include? io.c uses stuff from asm/io.h. I wouldn't
> remove it just because vm.h also includes it. Also, I guess io.c
> should include asm/page.h now.

I simply took your words on header inclusion too literally and removed it :)
But given that vm.h in x86 code is so ubiquitous and already includes
the two headers it did not seem completely unfounded :)

Will re-include both.

> >  #ifndef USE_SERIAL
> >  #define USE_SERIAL
> >  #endif
> > @@ -81,3 +81,16 @@ void exit(int code)
> >          asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
> >  #endif
> >  }
> > +
> > +void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
> > +{
> > +	phys_addr_t base = phys_addr & PAGE_MASK;
> > +	phys_addr_t off = phys_addr - base;
> > +	ulong nr = (off + size + (PAGE_SIZE - 1)) / PAGE_SIZE;
> 
> nit: I prefer
>  nr = ALIGN(off + size, PAGE_SIZE) >> PAGE_SHIFT
> 
> hmm, x86 doesn't have a PAGE_SHIFT yet. Should probably add
> that along with PAGE_MASK to asm/page.h

Would you prefer to add it in this patch or in patch 3
"x86: Introduce lib/x86/asm/page.h"?

Also what about doing it the way it normally done?

#define PAGE_SHIFT	12
#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK	(~(PAGE_SIZE-1))

Thanks!
--
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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux