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

[Mark Langsdorf]

On 03/10/2012 09:27 AM, Peter Maydell wrote:
> 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 :-)
>>
>> 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.

I'm reluctant to continue this thread, but I feel obliged to ask:

what types of changes should I make to allow the highbank model to put
with more than 2GB of emulated DRAM?

I've fired off 3 versions of a patch that answers that question, some
of which I've liked more than others. I'm willing to do a reasonable
amount of refactoring the general QEMU image loading code, but I don't
want to do that until I have a sense that the maintainers agree on the
general solution and that I'm working toward their understand.

Thanks for thinking this over.

--Mark Langsdorf
Calxeda, Inc.