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

[Alexander Graf]

On 26.09.2012, at 21:25, Christian Borntraeger wrote:

> On 26/09/12 21:01, Alexander Graf wrote:
>> 
>> On 26.09.2012, at 18:06, Christian Borntraeger wrote:
>> 
>>> 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.
>> 
>> Fair enough :).
>> 
>>> 
>>> 
>>> [...]
>>> 
>>>>> +    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
>> 
>> I would still see both fulfilled by having the 2 condition checks in sclp.c. 
>> Why don't we need them for kvm? Does kvm check them in the kernel?
> 
> KVM needs the checks as well. Thats why kvm calls into sclp_service_call 
> (like the tcg) and then evaluates the return value (like tcg). 
> sclp_service_call is the only place that calls into hw/* code. If we move 
> that code into sclp we have two places that call from target to hw/: kvm.c 
> and op_helper.c (now misc_helper.c). But it really doesnt matter, so lets 
> just move that code.

Ah! Yes, please just call into sclp.c from kvm.c :).


Alex