Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification

[Alexander Graf]

On 17.12.2013, at 16:58, Matthew Rosato <address@hidden> wrote:

> On 12/16/2013 04:25 PM, Alexander Graf wrote:
>> 
>> On 16.12.2013, at 21:51, Matthew Rosato <address@hidden> wrote:
>> 
>>> When machine=...,standby-mem={size} has been specified, convert the value
>>> to bytes and store it for use.
>>> 
>>> Signed-off-by: Matthew Rosato <address@hidden>
>>> ---
>>> target-s390x/kvm.c |   16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>> 
>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>> index 02ac4ba..d4081f4 100644
>>> --- a/target-s390x/kvm.c
>>> +++ b/target-s390x/kvm.c
>>> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>>> 
>>> int kvm_arch_init(KVMState *s)
>>> {
>>> +    int64_t value;
>>> +
>>>    cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>>    if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>>        || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>>>        phys_mem_set_alloc(legacy_s390_alloc);
>>>    }
>>> +
>>> +    value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
>>> +
>>> +    if (value < 0) {
>>> +        fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", 
>>> value);
>>> +        exit(1);
>>> +    }
>>> +
>>> +    if (value != (int64_t)(ram_addr_t)value) {
>>> +        fprintf(stderr, "qemu: standby size too large\n");
>>> +        exit(1);
>>> +    }
>>> +    standby_mem_size = value * 1024 * 1024;
>> 
>> I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it 
>> here?
>> 
> 
> It's actually in megabytes, and this is converting to bytes --  Based on
> your comment, it sounds like qemu_opt_get_size should always be
> returning a byte value.

At least I don't see code that converts it into MB. After all, you should be 
able to say size=1M which with your code probably ends up as 1T, no?

> FWIW, if I adopt Paolo's comments re: patch 1, this behavior goes away
> (and the parsing moves out of s390).  Either way, I'll adjust for v2.

Ok :).


Alex