Re: [PATCH 1/1] s390x:clp: implementing CLP immediate commands

[Pierre Morel <pmorel@linux.ibm.com>]

On 10/12/21 09:27, Thomas Huth wrote:
> On 17/09/2021 14.06, Pierre Morel wrote:
>> CLP immediate commands allow to query the Logical Processor
>> available on the machine and to check for a specific one.
>>
>> Let's add these commands.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c         | 33 ++++++++++++++++++++++++++++++++
>>   include/hw/s390x/s390-pci-inst.h |  5 +++++
>>   target/s390x/kvm/kvm.c           |  6 ++++++
>>   3 files changed, 44 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 1c8ad91175..9fd0669591 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -156,6 +156,39 @@ out:
>>       return rc;
>>   }
>> +int clp_immediate_cmd(S390CPU *cpu, uint8_t r1, uint8_t r2, uint8_t i3,
>> +                      uintptr_t ra)
>> +{
>> +    CPUS390XState *env = &cpu->env;
>> +
>> +    switch (r2) {
>> +    case 0: /* Command Check */
>> +        switch (i3 & 0x07) {
>> +        case CLP_LPS_PCI: /* PCI */
>> +            if (!s390_has_feat(S390_FEAT_ZPCI)) {
>> +                setcc(cpu, 3);
>> +                return 0;
>> +            }
>> +            /* fallthrough */
>> +        case CLP_LPS_BASE: /* Base LP */
>> +            setcc(cpu, 0);
>> +            return 0;
>> +        }
>> +        setcc(cpu, 3);
>> +        return 0;
>> +    case 1: /* Command Query */
>> +        env->regs[r1] = CLP_QUERY_LP_BASE;
>> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +            env->regs[r1] |= CLP_QUERY_LP_BASE >> CLP_LPS_PCI;
>> +        }
>> +        setcc(cpu, 0);
>> +        return 0;
>> +    }
>> +
>> +    s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> 
> Just a matter of taste, but I'd rather put that into a "default:" case, 
> so that the other cases could "break" instead of returning immediately.

OK, is better.

> 
>> +    return 0;
>> +}
> 
> All return statements return 0. Thus the return value does not really 
> matter, you could also turn this into a "void" function.

Yes.

> 
>>   int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>>   {
>>       ClpReqHdr *reqh;
>> diff --git a/include/hw/s390x/s390-pci-inst.h 
>> b/include/hw/s390x/s390-pci-inst.h
>> index a55c448aad..07721b08da 100644
>> --- a/include/hw/s390x/s390-pci-inst.h
>> +++ b/include/hw/s390x/s390-pci-inst.h
>> @@ -101,6 +101,11 @@ typedef struct ZpciFib {
>>   int pci_dereg_irqs(S390PCIBusDevice *pbdev);
>>   void pci_dereg_ioat(S390PCIIOMMU *iommu);
>>   int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra);
>> +#define CLP_LPS_BASE 0
>> +#define CLP_LPS_PCI  2
>> +#define CLP_QUERY_LP_BASE (1UL << 63)
>> +int clp_immediate_cmd(S390CPU *cpu, uint8_t r1, uint8_t r2, uint8_t i3,
>> +                      uintptr_t ra);
>>   int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, 
>> uintptr_t ra);
>>   int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, 
>> uintptr_t ra);
>>   int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, 
>> uintptr_t ra);
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 0a5f2aced2..af1316372d 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -1345,7 +1345,13 @@ static uint64_t get_base_disp_rsy(S390CPU *cpu, 
>> struct kvm_run *run,
>>   static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>>   {
>> +    uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>>       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>> +    uint8_t i3 = (run->s390_sieic.ipb & 0xff000000) >> 24;
>> +
>> +    if (i3 & 0x80) {
>> +        return clp_immediate_cmd(cpu, r1, r2, i3, RA_IGNORED);
>> +    }
>>       if (s390_has_feat(S390_FEAT_ZPCI)) {
>>           return clp_service_call(cpu, r2, RA_IGNORED);
>>
> 
> Could you please adjust the TCG part accordingly, too?
> 
>   Thomas
> 
> 

Thanks for the comments,
I adjust TCG too.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen