On Fri, Apr 29, 2016 at 08:58:30PM +0200, Alexander Gordeev wrote:
> 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.
I don't think they can wait. The problem is that if somebody
modifies those files to include something that includes
asm/page.h then things will break for them, and they'll have
to fix it. We shouldn't allow that to happen.
>
> > > 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 I said that we strive to explicitly include everything we depend on,
rather than dropping includes since they're already indirectly included.
So you shouldn't have removed it based on what I said.
> 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"?
If patch 3 is only code motion at this time (I think that's the case),
then it should stay only code motion. Then we introduce new defines,
and cleanup the redundant (potentially conflicting defines) in a separate
patch.
>
> 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))
These work for me.
Thanks,
drew
>
> 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
--
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
