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

[Anthony Liguori <anthony@codemonkey.ws>]

On 06/12/2012 07:32 AM, Alexander Graf wrote:
> 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.

Indeed :-)

>
> To me, the SCLP interface is similar to PIO, MMIO, SPAPR hypercalls, you name
> it.

Yeah, the SPAPR hypercalls is a good one I think but I don't know enough about 
SCLP yet.  From what's here, it would be pretty easy to model with qemu_irq I think.

We do that for target-i386 for things like the a20 line which is another case 
where random hardware interacts with the cpu in a far too personal fashion.

Regards,

Anthony Liguori

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