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

[Thomas Huth]

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.

> +    return 0;
> +}

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

>   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