Re: [PATCH v3 4/8] s390/sclp: read sccb from mem based on sccb length
On Thu, 18 Jun 2020 18:22:54 -0400 Collin Walling wrote: > The header of the SCCB contains the actual length of the SCCB. Instead > of using a static 4K size, let's allow for a variable size determined > by the value set in the header. The proper checks are already in place > to ensure the SCCB length is sufficent to store a full response, and > that the length does not cross any explicitly-set boundaries. > > Signed-off-by: Collin Walling > Reviewed-by: Thomas Huth > --- > hw/s390x/sclp.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > Reviewed-by: Cornelia Huck
Re: [PATCH v3 4/8] s390/sclp: read sccb from mem based on sccb length
On 6/19/20 12:22 AM, Collin Walling wrote:
> The header of the SCCB contains the actual length of the SCCB. Instead
> of using a static 4K size, let's allow for a variable size determined
> by the value set in the header. The proper checks are already in place
> to ensure the SCCB length is sufficent to store a full response, and
> that the length does not cross any explicitly-set boundaries.
>
> Signed-off-by: Collin Walling
> Reviewed-by: Thomas Huth
I wonder if this brings a measureable performance penalty with it for
protected guests. It's another ioctl to move the remaining bytes from
KVM to QEMU. On the other hand it's only sclp...
Reviewed-by: Janosch Frank
> ---
> hw/s390x/sclp.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 0710138f91..772b7b3b01 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -256,9 +256,8 @@ int sclp_service_call_protected(CPUS390XState *env,
> uint64_t sccb,
> SCLPDevice *sclp = get_sclp_device();
> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> SCCB work_sccb;
> -hwaddr sccb_len = sizeof(SCCB);
>
> -s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb, sccb_len);
> +s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb,
> sizeof(SCCBHeader));
>
> if (!sclp_command_code_valid(code)) {
> work_sccb.h.response_code =
> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> @@ -269,6 +268,9 @@ int sclp_service_call_protected(CPUS390XState *env,
> uint64_t sccb,
> goto out_write;
> }
>
> +s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb,
> + be16_to_cpu(work_sccb.h.length));
> +
> sclp_c->execute(sclp, _sccb, code);
> out_write:
> s390_cpu_pv_mem_write(env_archcpu(env), 0, _sccb,
> @@ -283,8 +285,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> SCCB work_sccb;
>
> -hwaddr sccb_len = sizeof(SCCB);
> -
> /* first some basic checks on program checks */
> if (env->psw.mask & PSW_MASK_PSTATE) {
> return -PGM_PRIVILEGED;
> @@ -302,7 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> * from playing dirty tricks by modifying the memory content after
> * the host has checked the values
> */
> -cpu_physical_memory_read(sccb, _sccb, sccb_len);
> +cpu_physical_memory_read(sccb, _sccb, sizeof(SCCBHeader));
>
> /* Valid sccb sizes */
> if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
> @@ -318,6 +318,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
> uint32_t code)
> goto out_write;
> }
>
> +/* the header contains the actual length of the sccb */
> +cpu_physical_memory_read(sccb, _sccb,
> be16_to_cpu(work_sccb.h.length));
> +
> sclp_c->execute(sclp, _sccb, code);
> out_write:
> cpu_physical_memory_write(sccb, _sccb,
>
signature.asc
Description: OpenPGP digital signature
[PATCH v3 4/8] s390/sclp: read sccb from mem based on sccb length
The header of the SCCB contains the actual length of the SCCB. Instead
of using a static 4K size, let's allow for a variable size determined
by the value set in the header. The proper checks are already in place
to ensure the SCCB length is sufficent to store a full response, and
that the length does not cross any explicitly-set boundaries.
Signed-off-by: Collin Walling
Reviewed-by: Thomas Huth
---
hw/s390x/sclp.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0710138f91..772b7b3b01 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -256,9 +256,8 @@ int sclp_service_call_protected(CPUS390XState *env,
uint64_t sccb,
SCLPDevice *sclp = get_sclp_device();
SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
SCCB work_sccb;
-hwaddr sccb_len = sizeof(SCCB);
-s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb, sccb_len);
+s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb, sizeof(SCCBHeader));
if (!sclp_command_code_valid(code)) {
work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
@@ -269,6 +268,9 @@ int sclp_service_call_protected(CPUS390XState *env,
uint64_t sccb,
goto out_write;
}
+s390_cpu_pv_mem_read(env_archcpu(env), 0, _sccb,
+ be16_to_cpu(work_sccb.h.length));
+
sclp_c->execute(sclp, _sccb, code);
out_write:
s390_cpu_pv_mem_write(env_archcpu(env), 0, _sccb,
@@ -283,8 +285,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
SCCB work_sccb;
-hwaddr sccb_len = sizeof(SCCB);
-
/* first some basic checks on program checks */
if (env->psw.mask & PSW_MASK_PSTATE) {
return -PGM_PRIVILEGED;
@@ -302,7 +302,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
* from playing dirty tricks by modifying the memory content after
* the host has checked the values
*/
-cpu_physical_memory_read(sccb, _sccb, sccb_len);
+cpu_physical_memory_read(sccb, _sccb, sizeof(SCCBHeader));
/* Valid sccb sizes */
if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
@@ -318,6 +318,9 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb,
uint32_t code)
goto out_write;
}
+/* the header contains the actual length of the sccb */
+cpu_physical_memory_read(sccb, _sccb,
be16_to_cpu(work_sccb.h.length));
+
sclp_c->execute(sclp, _sccb, code);
out_write:
cpu_physical_memory_write(sccb, _sccb,
--
2.21.3
