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

[Alexander Graf <agraf@suse.de>]

On 06/12/2012 02:24 PM, Christian Borntraeger wrote:
> 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.

Yeah, or have KVM hook into the tcg interrupt dispatch loop at 
cpu_exec.c:cpu_exec(). Not sure which way is easier.

>
>
> 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?

Yes, please :).

>
>>>       }
>>>
>>>       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:

Which is exactly why Anthony is suggesting for years now to pull the 
APIC code into target-i386.

To me, the SCLP interface is similar to PIO, MMIO, SPAPR hypercalls, you 
name it. We can certainly have sclp awareness in target-s390x, but 
please don't just blindly include headers from hw/. Split the few bits 
of information that we need in target-s390x into a separate header 
(clean) or target-s390x/cpu.h (hacky, but ok for now) and rather include 
that from hw/.


Alex