| Message ID | 1338375646-15064-24-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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)
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(-)