Re: [Qemu-devel] [PATCH 1/4] s390: sclp base support

[Christian Borntraeger]

On 26/09/12 17:00, Alexander Graf wrote:

>> +/* Provide information about the configuration, CPUs and storage */
>> +static void read_SCP_info(SCCB *sccb)
>> +{
>> +    ReadInfo *read_info = (ReadInfo *) sccb;
>> +    int shift = 0;
>> +
>> +    while ((ram_size >> (20 + shift)) > 65535) {
>> +        shift++;
>> +    }
>> +    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
>> +    read_info->rnsize = 1 << shift;
> 
> Any reason we can't just always expose rnmax2 and rnsize2 instead? This way 
> we're quite limited on the amount of RAM we can support, right?

Well, we have 65535 * 256 * 1MB == 16TB which is ok for the next 2 or 3 years I 
guess.
There are actually some rules that decide about rnmax vs rnmax2 etc, and here
the non-2 are appropriate. This might change for systems > 16TB or systems with 
memory hotplug,
but I would prefer to use those for now. We will add the full logic in case we 
add memory
hotplug.


[...]

>> +    if (be16_to_cpu(work_sccb.h.length) < 8 ||
> 
> sizeof(SCCBHeader)

ok


> 
>> +        be16_to_cpu(work_sccb.h.length) > 4096) {
> 
> SCCB_SIZE

ok


>>  */
>> -int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
>> +int sclp_service_call(uint32_t sccb, uint64_t code)
> 
> Why not move the whole thing into sclp.c? The only thing remaining here are a 
> few sanity checks that would just as well work in generic sclp handling code, 
> right?

The idea was two-fold:
- to have one single place were we cross between target-s390x and hw (review 
feedback from the first series, originally we had all everything in sclp.c)
- to have the checks that the CPU can do over there and the complex things that 
look into the sccb in sclp.c

But we could certainly move that, your take

Christian