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

["Andreas Färber" <afaerber@suse.de>]

Am 09.03.2012 20:04, schrieb Alexander Graf:
> 
> On 09.03.2012, at 19:47, Andreas Färber wrote:
> 
>> Am 09.03.2012 18:11, schrieb Peter Maydell:
>>> On 9 March 2012 14:28, Andreas Färber <afaerber@suse.de> wrote:
>>>> No, please. We're describing sizes, not addresses. target_phys_addr_t
>>>> thus is semantically wrong here. The RAM size is unsigned long IIRC (it
>>>> is limited by the host's available memory). If you subtract something
>>>> from a size it remains a size. I had therefore suggested size_t before.
>>>> I expect sizeof(size_t) >= sizeof(unsigned long).
>>>
>>> We're discussing target sizes. size_t might be smaller than
>>> target_phys_addr_t, so it's also semantically wrong. We don't
>>> have a target_size_t, though, and I think "use an address
>>> related type for an offset" is less bad than "use a host
>>> sized type for a guest sized value".
>>
>> That is a moot point. There is no such thing as a "target size". The
>> size is defined by the guest OS, not by the architecture. And it doesn't
>> matter if the guest OS's size is defined larger than the host's because
>> we process those files on the host and they must fit into host memory.
>>
>> So unsigned long would be perfectly fine if ignoring oddball win64.
>>
>>> Compare the way we use target_phys_addr_t for the offset arguments
>>> to MemoryRegion read/write functions.
>>
>> Nobody here has been arguing against using target_phys_addr_t for guest
>> memory *offsets*.
>>
>> Actually, the size (1, 2, 4) is an unsigned int there. :) Fine with me.
>>
>> And due to very similar signed overflow issues with int64_t, 128-bit
>> integer support was introduced as a workaround. ;)
>>
>>
>> Anyway, POSIX:2008 says this about sys/types.h:
>>
>> off_t
>> 	Used for file sizes.
>> size_t
>> 	Used for sizes of objects.
>> ssize_t
>> 	Used for a count of bytes or an error indication.
>>
>> So off_t seems right for get_image_size() although a bit
>> counter-intuitive. However later is said, "off_t shall be signed integer
>> types". So on a 32-bit host that does not necessarily help for the case
>> at hands. (Since get_image_size() gets its value as an off_t though, it
>> doesn't matter there and improves the 64-bit host case.)
>>
>> By comparison, "size_t shall be an unsigned integer type" and "The
>> implementation shall support one or more programming environments in
>> which the widths of [...] size_t, ssize_t, [...] are no greater than the
>> width of type long".
>>
>> So I still think that size_t is our best bet for potentially large sizes
>> here. Comparison with off_t would cause warnings though...
> 
> You're on the wrong track here really :). Size, max_size and friends are all offsets into the guest's address space, so that's the type they should have. We don't care about host POSIX whatever semantics here - it's a question of how big can guest address space grow.

No, we're talking about different use cases here. You *assume* the size
depends on guest RAM. I don't.

And no, max_size is not an offset. start + max_size is. The difference
is that this expression may overflow to zero in cases I care about.

For both PReP and Macs the size depends solely on the physical address
space and we know the max size ahead of time.

For your use case you can easily provide a separate function as a
wrapper, taking target_phys_addr_t start, end if you like (you're right,
that's possible) and calculating the off_t/int/... max_sz there and pass
it on to the existing API. Anything bigger we just cannot load as a blob.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg