* gen_pool_add broken with LPAE based systems
@ 2013-03-14 23:05 Laura Abbott
2013-03-19 21:54 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Laura Abbott @ 2013-03-14 23:05 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD, Andrew Morton, Benjamin Gaignard
Cc: Linux Kernel Mailing List, linux-arm-kernel
Hi,
We use genalloc for managing certain pools of physical memory. genalloc
currently uses unsigned long for virtual addresses and phys_addr_t for
physical addresses. Our ARM LPAE systems have 64-bit physical addresses
but unsigned long is still 32 bits. Using gen_pool_add breaks with
addresses > 4G because gen_pool_add treats the address passed in as the
virtual address. gen_pool allocates internally based on the 32 bit
virtual address as well so everything is broken if we want to be able to
manage the full address space after 4G. I see a couple of options:
1) Change gen_pool_add to use physical addresses and allocate based on
physical addresses instead of virtual addresses
2) Change the virtual address to be a 64 bit type or something
selectable to a 64 bit type.
3) Allow a flag per pool to select whether the allocator is virtual or
physical and switch between those.
4) Split the APIs into virtual <-> physical and physical only and have
separate types for each.
Any of these suggestions seem reasonable or is there another option to
consider?
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: gen_pool_add broken with LPAE based systems
2013-03-14 23:05 gen_pool_add broken with LPAE based systems Laura Abbott
@ 2013-03-19 21:54 ` Andrew Morton
2013-03-19 22:49 ` Laura Abbott
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2013-03-19 21:54 UTC (permalink / raw)
To: Laura Abbott
Cc: Jean-Christophe PLAGNIOL-VILLARD, Benjamin Gaignard,
Linux Kernel Mailing List, linux-arm-kernel
On Thu, 14 Mar 2013 16:05:27 -0700 Laura Abbott <lauraa@codeaurora.org> wrote:
> Hi,
>
> We use genalloc for managing certain pools of physical memory. genalloc
> currently uses unsigned long for virtual addresses and phys_addr_t for
> physical addresses. Our ARM LPAE systems have 64-bit physical addresses
> but unsigned long is still 32 bits. Using gen_pool_add breaks with
> addresses > 4G because gen_pool_add treats the address passed in as the
> virtual address. gen_pool allocates internally based on the 32 bit
> virtual address as well so everything is broken if we want to be able to
> manage the full address space after 4G. I see a couple of options:
The above only makes sense if ARM LPAE has 64-bit (actually >= 33-bit)
virtual addresses. If so, I don't understand how ARM LPAE can work at
all - the core MM assumes that addresses-fit-in-ulongs in eleventy
trillion places.
I think we need a better description of the problem, please.
> 1) Change gen_pool_add to use physical addresses and allocate based on
> physical addresses instead of virtual addresses
> 2) Change the virtual address to be a 64 bit type or something
> selectable to a 64 bit type.
> 3) Allow a flag per pool to select whether the allocator is virtual or
> physical and switch between those.
> 4) Split the APIs into virtual <-> physical and physical only and have
> separate types for each.
>
> Any of these suggestions seem reasonable or is there another option to
> consider?
2) sounds least intrusive but I can't think with my head spinning so fast.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: gen_pool_add broken with LPAE based systems
2013-03-19 21:54 ` Andrew Morton
@ 2013-03-19 22:49 ` Laura Abbott
2013-03-19 23:00 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Laura Abbott @ 2013-03-19 22:49 UTC (permalink / raw)
To: Andrew Morton
Cc: Benjamin Gaignard, Jean-Christophe PLAGNIOL-VILLARD,
Linux Kernel Mailing List, linux-arm-kernel
On 3/19/2013 2:54 PM, Andrew Morton wrote:
> On Thu, 14 Mar 2013 16:05:27 -0700 Laura Abbott <lauraa@codeaurora.org> wrote:
>
>> Hi,
>>
>> We use genalloc for managing certain pools of physical memory. genalloc
>> currently uses unsigned long for virtual addresses and phys_addr_t for
>> physical addresses. Our ARM LPAE systems have 64-bit physical addresses
>> but unsigned long is still 32 bits. Using gen_pool_add breaks with
>> addresses > 4G because gen_pool_add treats the address passed in as the
>> virtual address. gen_pool allocates internally based on the 32 bit
>> virtual address as well so everything is broken if we want to be able to
>> manage the full address space after 4G. I see a couple of options:
>
> The above only makes sense if ARM LPAE has 64-bit (actually >= 33-bit)
> virtual addresses. If so, I don't understand how ARM LPAE can work at
> all - the core MM assumes that addresses-fit-in-ulongs in eleventy
> trillion places.
>
> I think we need a better description of the problem, please.
>
Sorry, let me clarify. ARM LPAE still has 32 bit virtual addresses.
Change 3c8f370ded3483b27f1218ff0051fcf0c7a2facd (lib/genalloc.c: add
support for specifying the physical address) added support for using
genalloc to know about both physical addresses and virtual addresses.
Allocation in gen_pool is still based on the virtual address though.
The problem is we've been using genalloc to allocate physical addresses,
not virtual ones so allocating and returning an unsigned long breaks
with sizeof(phys_addr_t) > sizeof(unsigned long). It looks like genalloc
was added and extended with virtual addresses in mind but apart from the
address size limitation right now it should be able to work just fine
for physical addresses.
There seem to be a few other clients scattered about who are using
genalloc for physical addresses as well (although all are 32 bit systems
right now)
A better subject would be 'genalloc broken on LPAE systems when used to
allocate physical addresses instead of virtual addresses'
>> 1) Change gen_pool_add to use physical addresses and allocate based on
>> physical addresses instead of virtual addresses
>> 2) Change the virtual address to be a 64 bit type or something
>> selectable to a 64 bit type.
>> 3) Allow a flag per pool to select whether the allocator is virtual or
>> physical and switch between those.
>> 4) Split the APIs into virtual <-> physical and physical only and have
>> separate types for each.
>>
>> Any of these suggestions seem reasonable or is there another option to
>> consider?
>
> 2) sounds least intrusive but I can't think with my head spinning so fast.
>
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: gen_pool_add broken with LPAE based systems
2013-03-19 22:49 ` Laura Abbott
@ 2013-03-19 23:00 ` Andrew Morton
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2013-03-19 23:00 UTC (permalink / raw)
To: Laura Abbott
Cc: Benjamin Gaignard, Jean-Christophe PLAGNIOL-VILLARD,
Linux Kernel Mailing List, linux-arm-kernel
On Tue, 19 Mar 2013 15:49:24 -0700 Laura Abbott <lauraa@codeaurora.org> wrote:
> On 3/19/2013 2:54 PM, Andrew Morton wrote:
> > On Thu, 14 Mar 2013 16:05:27 -0700 Laura Abbott <lauraa@codeaurora.org> wrote:
> >
> >> Hi,
> >>
> >> We use genalloc for managing certain pools of physical memory. genalloc
> >> currently uses unsigned long for virtual addresses and phys_addr_t for
> >> physical addresses. Our ARM LPAE systems have 64-bit physical addresses
> >> but unsigned long is still 32 bits. Using gen_pool_add breaks with
> >> addresses > 4G because gen_pool_add treats the address passed in as the
> >> virtual address. gen_pool allocates internally based on the 32 bit
> >> virtual address as well so everything is broken if we want to be able to
> >> manage the full address space after 4G. I see a couple of options:
> >
> > The above only makes sense if ARM LPAE has 64-bit (actually >= 33-bit)
> > virtual addresses. If so, I don't understand how ARM LPAE can work at
> > all - the core MM assumes that addresses-fit-in-ulongs in eleventy
> > trillion places.
> >
> > I think we need a better description of the problem, please.
> >
>
> Sorry, let me clarify. ARM LPAE still has 32 bit virtual addresses.
>
> Change 3c8f370ded3483b27f1218ff0051fcf0c7a2facd (lib/genalloc.c: add
> support for specifying the physical address) added support for using
> genalloc to know about both physical addresses and virtual addresses.
> Allocation in gen_pool is still based on the virtual address though.
>
> The problem is we've been using genalloc to allocate physical addresses,
> not virtual ones so allocating and returning an unsigned long breaks
> with sizeof(phys_addr_t) > sizeof(unsigned long). It looks like genalloc
> was added and extended with virtual addresses in mind but apart from the
> address size limitation right now it should be able to work just fine
> for physical addresses.
>
> There seem to be a few other clients scattered about who are using
> genalloc for physical addresses as well (although all are 32 bit systems
> right now)
I see. So genpool has never worked properly for this application?
> A better subject would be 'genalloc broken on LPAE systems when used to
> allocate physical addresses instead of virtual addresses'
I'd say "extend genpool so we can use physical addresses instead of
virtual addresses" ;)
> >> 1) Change gen_pool_add to use physical addresses and allocate based on
> >> physical addresses instead of virtual addresses
> >> 2) Change the virtual address to be a 64 bit type or something
> >> selectable to a 64 bit type.
> >> 3) Allow a flag per pool to select whether the allocator is virtual or
> >> physical and switch between those.
> >> 4) Split the APIs into virtual <-> physical and physical only and have
> >> separate types for each.
> >>
> >> Any of these suggestions seem reasonable or is there another option to
> >> consider?
> >
> > 2) sounds least intrusive but I can't think with my head spinning so fast.
I suppose using a bare u64 for `addr' would fix things up.
I think it's rather regrettable that genpool.c contains terms like
"addr", "phys" and "virt" at all. It's in lib/ and it's a
general-purpose container thing. It should know whether it's operating
on addresses or bananas or whatever. A better layering would be to
weed all that out of there, implement a truly general-purpose container
and then add convenience wrappers around that for each particular
application.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-03-19 23:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-14 23:05 gen_pool_add broken with LPAE based systems Laura Abbott
2013-03-19 21:54 ` Andrew Morton
2013-03-19 22:49 ` Laura Abbott
2013-03-19 23:00 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).