Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions

[Christian Borntraeger <borntraeger@de.ibm.com>]

Yes we will re-split the sclp patches.

besides that, some comments:

On 12/06/12 11:58, Alexander Graf wrote:
>> +#include "hw/s390-sclp.h"
>>   
> 
> No need for hw/.

will fix. 


>> +void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb)
>> +{
>> +    if (!sccb) {
>> +        return;
>> +    }
>> +
>> +    if (kvm_enabled()) {
>> +#ifdef CONFIG_KVM
>>   
> 
> You shouldn't know about CONFIG_KVM in hw/. So we have to generalize
> this code.

Ok, Maybe an exported interface for sending interrupts to the guest 
under target-s390/  that hides the kvm/tcg thing.


ice_call(CPUS390XState *env, struct kvm_run *run,
>>      r = sclp_service_call(env, sccb, code);
>>      if (r) {
>>          setcc(env, 3);
>> +    } else {
>> +        setcc(env, 0);
>>   
> 
> This one looks like an actual fix that is not part of the cleanup?

Yes it is. Separate patch?

> 
>>      }
>>  
>>      return 0;
>> diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
>> index 7b72473..74bd9ad 100644
>> --- a/target-s390x/op_helper.c
>> +++ b/target-s390x/op_helper.c
>> @@ -31,6 +31,7 @@
>>  
>>  #if !defined (CONFIG_USER_ONLY)
>>  #include "sysemu.h"
>> +#include "hw/s390-sclp.h"
>>   
> 
> #include in hw/ from target-XXX is a no-go. It means our abstraction
> layer is broken.

Disagree here. The sclp is a processor that helps the CPU and there is a 
tight link. This is similar to a PIC/APIC etc which are also under hw AND
included from target-386/ - among others:

cborntra@br96egxr:/space/qemu$ egrep "include.*hw"  target-*/* | wc -l
39


[...9

>> -            if (kvm_enabled()) {
>> -#ifdef CONFIG_KVM
>> -                kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE,
>> -                                            sccb & ~3, 0, 1);
>> -#endif
>> -            } else {
>> -                env->psw.addr += 4;
>> -                ext_interrupt(env, EXT_SERVICE, sccb & ~3, 0);
>> -            }
>> +            r = sclp_read_info(env, &work_sccb);
>>   
> 
> Maybe we should have a list of callbacks that hw/ code can register for?
> Like the spapr hcalls.

We will have a look if thats a way to go.