Re: [Qemu-devel] [PATCH 2/6] s390: sclp base support

[Christian Borntraeger]

Andreas,

thanks for having a look. I no other comments arrive today I will resubmit the 
patch 
set with all comments addressed.

On 20/07/12 16:06, Andreas Färber wrote:
>> +#ifdef DEBUG_HELPER
>> +        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, 
>> code);
> 
> sccb is a pointer so %x seems wrong for 64-bit host. %p?

yes its broken.
We will probably just remove that, since it will be changed by followon patch.


>> +typedef struct S390SCLPDevice {
>> +    DeviceState qdev;
> 
> I note that this is probably the first device directly derived from
> DeviceState. This should be possible now after having merged the QOM
> QBus series. Test case to check is "info qdm" from the monitor.

info qdm seems to work. what test did you have in mind?

[...]
>>  //#define DEBUG_S390
>>  
>> @@ -61,6 +62,7 @@
>>  #define MAX_BLK_DEVS                    10
>>  
>>  static VirtIOS390Bus *s390_bus;
>> +SCLPS390Bus *sclp_bus;
> 
> I don't like this so much. We shouldn't be following that example and
> adding global state for each bus there. Can't we add a child property to
> the machine for the hosting device so that we can obtain that via QOM
> path and access the bus from there?

Since Blue already compained Heinz already moved that bus into s390-sclp.c as a
static variable without any export since we only touch that there. We also added
an comment that by architecture definition there is only one sclp per machine, 
so 
this should be fine.


>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -1,3 +1,4 @@
>>  obj-y = s390-virtio-bus.o s390-virtio.o
>> +obj-y += s390-sclp.o
>>  
>>  obj-y := $(addprefix ../,$(obj-y))
> 
> IIUC we have a dependency on the CPU inside the device code. If so,
> please move the s390-sclp.c file into hw/s390x/ and move the obj-y +=
> line below the addprefix line.

Ok.

>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 619b202..8900872 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -21,9 +21,26 @@
>>   */
>>  
>>  #include "cpu.h"
>> +#include "kvm.h"
>>  #include "qemu-common.h"
>>  #include "qemu-timer.h"
>>  
>> +/* service interrupts are floating therefore we must not pass an cpustate */
>> +void s390_sclp_extint(uint32_t parm)
>> +{
>> +    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
>> +    CPUS390XState *env = &dummy_cpu->env;
>> +
>> +    if (kvm_enabled()) {
>> +#ifdef CONFIG_KVM
>> +        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
>> +#endif
>> +    } else {
>> +        env->psw.addr += 4;
>> +        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
>> +    }
>> +}
> 
> Not so happy about this placement, it is not called s390_cpu_, the
> prototype is in cpu.h not cpu-qom.h and it operates only indirectly on
> the CPU. Is there another place for it?

Not yet. I will try to introduce a new one, e.g. target-s390x/interrupt.c
We can then move other code there as well.