Patchwork [23/25] PPC: e500: dt: use target_phys_addr_t for ramsize

login
register
mail settings
Submitter Alexander Graf
Date May 30, 2012, 11 a.m.
Message ID <1338375646-15064-24-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/161953/
State New
Headers show

Comments

Alexander Graf - May 30, 2012, 11 a.m.
We're passing the ram size as uint32_t, capping it to 32 bits atm.
Change to target_phys_addr_t (uint64_t) to make sure we have all
the bits.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppce500_mpc8544ds.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Scott Wood - May 31, 2012, 10:07 p.m.
On 05/30/2012 06:00 AM, Alexander Graf wrote:
> We're passing the ram size as uint32_t, capping it to 32 bits atm.
> Change to target_phys_addr_t (uint64_t) to make sure we have all
> the bits.

Wouldn't ram_addr_t be more appropriate?

-Scott
Alexander Graf - May 31, 2012, 10:18 p.m.
On 01.06.2012, at 00:07, Scott Wood wrote:

> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>> We're passing the ram size as uint32_t, capping it to 32 bits atm.
>> Change to target_phys_addr_t (uint64_t) to make sure we have all
>> the bits.
> 
> Wouldn't ram_addr_t be more appropriate?

I never quite grasped the difference, but wasn't ram_addr_t something for the host?


Alex
Scott Wood - May 31, 2012, 10:24 p.m.
On 05/31/2012 05:18 PM, Alexander Graf wrote:
> 
> On 01.06.2012, at 00:07, Scott Wood wrote:
> 
>> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>>> We're passing the ram size as uint32_t, capping it to 32 bits atm.
>>> Change to target_phys_addr_t (uint64_t) to make sure we have all
>>> the bits.
>>
>> Wouldn't ram_addr_t be more appropriate?
> 
> I never quite grasped the difference, but wasn't ram_addr_t something for the host?

I don't fully understand QEMU's RAM handling, but from what I recall RAM
is treated differently from arbitrary guest physical addresses, with a
QEMU-internal contiguous address space.  Guest RAM needs to be mappable
by QEMU as well as the target, so ram_addr_t is 32-bit on a 32-bit host,
even if target_phys_addr_t is different.

But again, it was a while ago that I looked at this, and I didn't fully
understand it then, so I may be missing or misremembering something.

-Scott
Alexander Graf - May 31, 2012, 10:39 p.m.
On 01.06.2012, at 00:24, Scott Wood wrote:

> On 05/31/2012 05:18 PM, Alexander Graf wrote:
>> 
>> On 01.06.2012, at 00:07, Scott Wood wrote:
>> 
>>> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>>>> We're passing the ram size as uint32_t, capping it to 32 bits atm.
>>>> Change to target_phys_addr_t (uint64_t) to make sure we have all
>>>> the bits.
>>> 
>>> Wouldn't ram_addr_t be more appropriate?
>> 
>> I never quite grasped the difference, but wasn't ram_addr_t something for the host?
> 
> I don't fully understand QEMU's RAM handling, but from what I recall RAM
> is treated differently from arbitrary guest physical addresses, with a
> QEMU-internal contiguous address space.  Guest RAM needs to be mappable
> by QEMU as well as the target, so ram_addr_t is 32-bit on a 32-bit host,
> even if target_phys_addr_t is different.
> 
> But again, it was a while ago that I looked at this, and I didn't fully
> understand it then, so I may be missing or misremembering something.

Right, so target_phys_addr_t is what we want, because that's what describes the guest's physical memory layout the best, which is what we're describing here, right? It should definitely be independent of the host's virtual memory layout.


Alex
Scott Wood - May 31, 2012, 10:52 p.m.
On 05/31/2012 05:39 PM, Alexander Graf wrote:
> 
> On 01.06.2012, at 00:24, Scott Wood wrote:
> 
>> On 05/31/2012 05:18 PM, Alexander Graf wrote:
>>>
>>> On 01.06.2012, at 00:07, Scott Wood wrote:
>>>
>>>> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>>>>> We're passing the ram size as uint32_t, capping it to 32 bits atm.
>>>>> Change to target_phys_addr_t (uint64_t) to make sure we have all
>>>>> the bits.
>>>>
>>>> Wouldn't ram_addr_t be more appropriate?
>>>
>>> I never quite grasped the difference, but wasn't ram_addr_t something for the host?
>>
>> I don't fully understand QEMU's RAM handling, but from what I recall RAM
>> is treated differently from arbitrary guest physical addresses, with a
>> QEMU-internal contiguous address space.  Guest RAM needs to be mappable
>> by QEMU as well as the target, so ram_addr_t is 32-bit on a 32-bit host,
>> even if target_phys_addr_t is different.
>>
>> But again, it was a while ago that I looked at this, and I didn't fully
>> understand it then, so I may be missing or misremembering something.
> 
> Right, so target_phys_addr_t is what we want, because that's what
> describes the guest's physical memory layout the best, which is what
> we're describing here, right? It should definitely be independent of
> the host's virtual memory layout.

It's not really the host virtual memory layout -- it's just constrained
by the need to fit there.  QEMU's notion of a "RAM address" is a
different address space than either target physical or host virtual.  We
can't have more target RAM than will fit in ram_addr_t (though it can
start at a target physical address that doesn't fit in ram_addr_t).

If you want to use target_phys_addr_t here, fine -- it's not performance
critical -- but the uint32_t was only really removing a limitation on
64-bit hosts, and semanticly target_phys_addr_t doesn't seem as correct
as ram_addr_t.

The same applies to initrd_size, which currently uses target_phys_addr_t.

If we were discussing the address of RAM (as seen by the target), rather
than the size of it, then target_phys_addr_t would be correct (and we
already use it for that).

From HACKING:
"Use target_phys_addr_t for guest physical addresses except pcibus_t
for PCI addresses.  In addition, ram_addr_t is a QEMU internal address
space that maps guest RAM physical addresses into an intermediate
address space that can map to host virtual address spaces.  Generally
speaking, the size of guest memory can always fit into ram_addr_t but
it would not be correct to store an actual guest physical address in a
ram_addr_t."

-Scott
Alexander Graf - May 31, 2012, 11:10 p.m.
On 01.06.2012, at 00:52, Scott Wood wrote:

> On 05/31/2012 05:39 PM, Alexander Graf wrote:
>> 
>> On 01.06.2012, at 00:24, Scott Wood wrote:
>> 
>>> On 05/31/2012 05:18 PM, Alexander Graf wrote:
>>>> 
>>>> On 01.06.2012, at 00:07, Scott Wood wrote:
>>>> 
>>>>> On 05/30/2012 06:00 AM, Alexander Graf wrote:
>>>>>> We're passing the ram size as uint32_t, capping it to 32 bits atm.
>>>>>> Change to target_phys_addr_t (uint64_t) to make sure we have all
>>>>>> the bits.
>>>>> 
>>>>> Wouldn't ram_addr_t be more appropriate?
>>>> 
>>>> I never quite grasped the difference, but wasn't ram_addr_t something for the host?
>>> 
>>> I don't fully understand QEMU's RAM handling, but from what I recall RAM
>>> is treated differently from arbitrary guest physical addresses, with a
>>> QEMU-internal contiguous address space.  Guest RAM needs to be mappable
>>> by QEMU as well as the target, so ram_addr_t is 32-bit on a 32-bit host,
>>> even if target_phys_addr_t is different.
>>> 
>>> But again, it was a while ago that I looked at this, and I didn't fully
>>> understand it then, so I may be missing or misremembering something.
>> 
>> Right, so target_phys_addr_t is what we want, because that's what
>> describes the guest's physical memory layout the best, which is what
>> we're describing here, right? It should definitely be independent of
>> the host's virtual memory layout.
> 
> It's not really the host virtual memory layout -- it's just constrained
> by the need to fit there.  QEMU's notion of a "RAM address" is a
> different address space than either target physical or host virtual.  We
> can't have more target RAM than will fit in ram_addr_t (though it can
> start at a target physical address that doesn't fit in ram_addr_t).
> 
> If you want to use target_phys_addr_t here, fine -- it's not performance
> critical -- but the uint32_t was only really removing a limitation on
> 64-bit hosts, and semanticly target_phys_addr_t doesn't seem as correct
> as ram_addr_t.
> 
> The same applies to initrd_size, which currently uses target_phys_addr_t.
> 
> If we were discussing the address of RAM (as seen by the target), rather
> than the size of it, then target_phys_addr_t would be correct (and we
> already use it for that).
> 
> From HACKING:
> "Use target_phys_addr_t for guest physical addresses except pcibus_t
> for PCI addresses.  In addition, ram_addr_t is a QEMU internal address
> space that maps guest RAM physical addresses into an intermediate
> address space that can map to host virtual address spaces.  Generally
> speaking, the size of guest memory can always fit into ram_addr_t but
> it would not be correct to store an actual guest physical address in a
> ram_addr_t."

So if I understand this correctly, the offset into the guest physical memory layout would be target_phys_addr_t, while the ram size would be ram_addr_t? Jeez - can't we just use u64 for everything and call it a day? :)


Alex

Patch

diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index 32fbdd3..9c41f50 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -85,7 +85,7 @@  static void pci_map_create(void *fdt, uint32_t *pci_map, uint32_t mpic)
 
 static int mpc8544_load_device_tree(CPUPPCState *env,
                                     target_phys_addr_t addr,
-                                    uint32_t ramsize,
+                                    target_phys_addr_t ramsize,
                                     target_phys_addr_t initrd_base,
                                     target_phys_addr_t initrd_size,
                                     const char *kernel_cmdline)