Re: [Qemu-devel] [PATCH] use an unsigned long for the max_sz parameter in load_image_targphys

[Peter Maydell]

On 10 March 2012 14:08, Andreas Färber <address@hidden> wrote:
> Am 10.03.2012 14:51, schrieb Peter Maydell:
>> "Length of a block of memory on the guest" is what I meant.
>> What you need is "an integer type large enough to hold the
>> difference between two guest pointer values". The size of
>> that type should depend only on the guest config, not on the
>> host, so 'unsigned long', 'size_t', 'off_t' etc are all wrong.
>
> Your view is very ARM-centric.

I don't understand this remark. Nothing about the above explanation
is ARM-centric -- it's just pointing out that the guest and the
host are two separate things and maximum widths of size and
pointer types aren't necessarily the same. Assuming they were the
same would be an x86-64-ism :-)

> In the PowerPC domain for instance, we
> have a number of usages where we hardcode a size of, e.g., 1 MB. And
> Alex should know that. I don't want to use target_phys_addr_t for that
> and forcing an end address calculation, as suggested by Alex, would be
> possible but is not as convenient as the current API.
>
> Doing a size check as Mark has demonstrated in ARM code (one place!)
> seems much simpler to me than hurting all targets just because ARM wants
> to pass a possibly stupid value unchecked to the common API.

Where the ARM specific code has particular restrictions then
I'm happy to have the ARM specific code either relax those, or
do explicit checks so we can fail cleanly or whatever.

Putting a "restrict to INT_MAX" in the highbank code is definitely
wrong (not least because passing values to load_image_targphys isn't
the only thing we use that field in arm_boot_info for!)
The ARM boot code needs updating because it shouldn't be
using 'int' for arm_boot_info.ram_size, and using target_phys_addr_t
the same as we do for initrd_size is the obvious thing. I have no
particular objection to having some new target_phys_size_t or whatever,
and I could be persuaded that we should follow the memory API in
using uint64_t for sizes, but it needs to be a type that either follows
guest phys addr restrictions or a fixed-width type which we have decreed
is always large enough, not a type which varies based on host properties.

(arm_boot also needs to fail if the size is > 4GB since at the moment
we do assume it to be 32 bits wide, but that's a different problem.)

> Compare David's off_t patch from March 8th: We'll never get an image
> size larger than off_t's max. Using target_phys_addr_t, uint64_t or
> __int128_t for the max are all moot (academic) because we'll never get
> to their max if it's larger than off_t. Therefore I've been saying, the
> host's limit is the upper realistic limit for image_load_targphys().

You mean this patch?
http://patchwork.ozlabs.org/patch/140064/
get_image_size() is asking a question about the host (ie "what is
the size of this host file?"). It therefore makes perfect sense
for its return type to be one whose size depends on properties of
the host, like off_t. load_image_targphys()'s max_sz parameter is
passing it information which is about the guest (usually "how big is
this block of RAM we're going to try to load this thing into?").
It therefore makes perfect sense for its type to be one whose size
depends on properties of the guest, or alternatively one whose
type is guaranteed to always be at least as large as the worst
case guest we support (so you could use uint64_t, as the memory
region API does for region sizes).

-- PMM