* [PATCH 0/1] s390x:clp: implementing CLP immediate commands
@ 2021-09-17 12:06 Pierre Morel
2021-09-17 12:06 ` [PATCH 1/1] " Pierre Morel
0 siblings, 1 reply; 5+ messages in thread
From: Pierre Morel @ 2021-09-17 12:06 UTC (permalink / raw)
To: qemu-s390x
Cc: thuth, mjrosato, david, farman, cohuck, richard.henderson,
qemu-devel, pasic, borntraeger
CLP immediate commands allow to query the Logical Processor
available on the machine and to check for a specific one.
I do not see any reason to not implement it.
The check for an immediate command is done on bit 0 of the RRF i3
field so let's implement a new clp_immediate_cmd() to handle CLP
immediate commands.
AFAIK only the basi processor, with Logical Processor Selector 0,
LPS==0, and the PCI processor, LPS==2, are currently available on
QEMU.
Pierre Morel (1):
s390x:clp: implementing CLP immediate commands
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(+)
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] s390x:clp: implementing CLP immediate commands
2021-09-17 12:06 [PATCH 0/1] s390x:clp: implementing CLP immediate commands Pierre Morel
@ 2021-09-17 12:06 ` Pierre Morel
2021-09-17 13:55 ` Matthew Rosato
2021-10-12 7:27 ` Thomas Huth
0 siblings, 2 replies; 5+ messages in thread
From: Pierre Morel @ 2021-09-17 12:06 UTC (permalink / raw)
To: qemu-s390x
Cc: thuth, mjrosato, david, farman, cohuck, richard.henderson,
qemu-devel, pasic, borntraeger
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);
+ return 0;
+}
+
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] s390x:clp: implementing CLP immediate commands
2021-09-17 12:06 ` [PATCH 1/1] " Pierre Morel
@ 2021-09-17 13:55 ` Matthew Rosato
2021-10-12 7:27 ` Thomas Huth
1 sibling, 0 replies; 5+ messages in thread
From: Matthew Rosato @ 2021-09-17 13:55 UTC (permalink / raw)
To: Pierre Morel, qemu-s390x
Cc: thuth, david, farman, cohuck, richard.henderson, qemu-devel,
pasic, borntraeger
On 9/17/21 8:06 AM, 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 */
So at first look the code itself seems sane and I have no problem with
implementing immediate clp, but tbh I find it a little weird to be
handling base LP stuff directly in the PCI code.
With this patch, you're introducing immediate commands implicitly
targeted at the base LP and circumstantially handling in them in PCI
code, even when !S390_FEAT_ZPCI. That seems wrong, we should ideally
only be getting to PCI code for CLP related to PCI, no?
Granted, it's true that the existing kvm_clp_service_call always calls
clp_service_call in PCI code today (if FEAT_ZPCI), but this function
only handles PCI-related clp commands and bails out on any other command
(really, it should probably be renamed zpci_clp_service_call and
kvm_clp_service_call should only be calling it for normal clp with LPS==2)
I guess the trouble is there's no obvious other home to handle anything
targeted at the base LP because we don't bother today?
> + 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);
> + return 0;
> +}
> +
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] s390x:clp: implementing CLP immediate commands
2021-09-17 12:06 ` [PATCH 1/1] " Pierre Morel
2021-09-17 13:55 ` Matthew Rosato
@ 2021-10-12 7:27 ` Thomas Huth
2021-10-12 14:26 ` Pierre Morel
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Huth @ 2021-10-12 7:27 UTC (permalink / raw)
To: Pierre Morel, qemu-s390x
Cc: farman, mjrosato, david, cohuck, richard.henderson, qemu-devel,
pasic, borntraeger
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] s390x:clp: implementing CLP immediate commands
2021-10-12 7:27 ` Thomas Huth
@ 2021-10-12 14:26 ` Pierre Morel
0 siblings, 0 replies; 5+ messages in thread
From: Pierre Morel @ 2021-10-12 14:26 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x
Cc: farman, mjrosato, david, cohuck, richard.henderson, qemu-devel,
pasic, borntraeger
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-12 14:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 12:06 [PATCH 0/1] s390x:clp: implementing CLP immediate commands Pierre Morel
2021-09-17 12:06 ` [PATCH 1/1] " Pierre Morel
2021-09-17 13:55 ` Matthew Rosato
2021-10-12 7:27 ` Thomas Huth
2021-10-12 14:26 ` Pierre Morel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.