[v1] target/s390x: Prohibit target_ulong and TARGET_PAGE_SIZE uses

[PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Philippe Mathieu-Daudé 1 month ago

Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
in CHSC instruction") the access is no more tied to the page
size. Define CHSC_MAX_REQ_LEN to better express the relation
with the length of the Channel Subsystem Call request.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/ioinst.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 2320dd4c122..d07773d14a3 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -398,6 +398,7 @@ typedef struct ChscResp {
     char data[];
 } QEMU_PACKED ChscResp;
 
+#define CHSC_MAX_REQ_LEN  0x1000
 #define CHSC_MIN_RESP_LEN 0x0008
 
 #define CHSC_SCPD 0x0002
@@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
     uint16_t len;
     uint16_t command;
     CPUS390XState *env = &cpu->env;
-    uint8_t buf[TARGET_PAGE_SIZE];
+    uint8_t buf[CHSC_MAX_REQ_LEN];
 
     trace_ioinst("chsc");
     reg = (ipb >> 20) & 0x00f;
@@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
         s390_program_interrupt(env, PGM_OPERAND, ra);
         return;
     }
-    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
+    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
     res = (void *)((char *)req + len);
     command = be16_to_cpu(req->command);
     trace_ioinst_chsc_cmd(command, len);
-- 
2.52.0

Re: [PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Thomas Huth 1 month ago

On 07/01/2026 14.08, Philippe Mathieu-Daudé wrote:
> Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
> in CHSC instruction") the access is no more tied to the page
> size. Define CHSC_MAX_REQ_LEN to better express the relation
> with the length of the Channel Subsystem Call request.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/ioinst.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 2320dd4c122..d07773d14a3 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -398,6 +398,7 @@ typedef struct ChscResp {
>       char data[];
>   } QEMU_PACKED ChscResp;
>   
> +#define CHSC_MAX_REQ_LEN  0x1000
>   #define CHSC_MIN_RESP_LEN 0x0008
>   
>   #define CHSC_SCPD 0x0002
> @@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>       uint16_t len;
>       uint16_t command;
>       CPUS390XState *env = &cpu->env;
> -    uint8_t buf[TARGET_PAGE_SIZE];
> +    uint8_t buf[CHSC_MAX_REQ_LEN];
>   
>       trace_ioinst("chsc");
>       reg = (ipb >> 20) & 0x00f;
> @@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>           s390_program_interrupt(env, PGM_OPERAND, ra);
>           return;
>       }
> -    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
> +    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
>       res = (void *)((char *)req + len);
>       command = be16_to_cpu(req->command);
>       trace_ioinst_chsc_cmd(command, len);

Sorry, I don't quite get it, there are still dozends of others 
TARGET_PAGE_SIZE usages in target/s390x/, what's the advantage of removing 
that one here?

  Thomas

Re: [PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Philippe Mathieu-Daudé 1 month ago

On 8/1/26 08:53, Thomas Huth wrote:
> On 07/01/2026 14.08, Philippe Mathieu-Daudé wrote:
>> Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
>> in CHSC instruction") the access is no more tied to the page
>> size. Define CHSC_MAX_REQ_LEN to better express the relation
>> with the length of the Channel Subsystem Call request.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/s390x/ioinst.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>> index 2320dd4c122..d07773d14a3 100644
>> --- a/target/s390x/ioinst.c
>> +++ b/target/s390x/ioinst.c
>> @@ -398,6 +398,7 @@ typedef struct ChscResp {
>>       char data[];
>>   } QEMU_PACKED ChscResp;
>> +#define CHSC_MAX_REQ_LEN  0x1000
>>   #define CHSC_MIN_RESP_LEN 0x0008
>>   #define CHSC_SCPD 0x0002
>> @@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>> ipb, uintptr_t ra)
>>       uint16_t len;
>>       uint16_t command;
>>       CPUS390XState *env = &cpu->env;
>> -    uint8_t buf[TARGET_PAGE_SIZE];
>> +    uint8_t buf[CHSC_MAX_REQ_LEN];
>>       trace_ioinst("chsc");
>>       reg = (ipb >> 20) & 0x00f;
>> @@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>> ipb, uintptr_t ra)
>>           s390_program_interrupt(env, PGM_OPERAND, ra);
>>           return;
>>       }
>> -    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
>> +    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
>>       res = (void *)((char *)req + len);
>>       command = be16_to_cpu(req->command);
>>       trace_ioinst_chsc_cmd(command, len);
> 
> Sorry, I don't quite get it, there are still dozends of others 
> TARGET_PAGE_SIZE usages in target/s390x/, what's the advantage of 
> removing that one here?

The other ones are also used by user emulation so need more care. This
series addresses all system-only files. I neglected to mention that in
the cover :(

But if you think this is better to review all target/s390x/ conversion
in one go, I can wait until I finish the rest.

Re: [PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Philippe Mathieu-Daudé 4 days, 20 hours ago

On 8/1/26 12:46, Philippe Mathieu-Daudé wrote:
> On 8/1/26 08:53, Thomas Huth wrote:
>> On 07/01/2026 14.08, Philippe Mathieu-Daudé wrote:
>>> Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
>>> in CHSC instruction") the access is no more tied to the page
>>> size. Define CHSC_MAX_REQ_LEN to better express the relation
>>> with the length of the Channel Subsystem Call request.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   target/s390x/ioinst.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>>> index 2320dd4c122..d07773d14a3 100644
>>> --- a/target/s390x/ioinst.c
>>> +++ b/target/s390x/ioinst.c
>>> @@ -398,6 +398,7 @@ typedef struct ChscResp {
>>>       char data[];
>>>   } QEMU_PACKED ChscResp;
>>> +#define CHSC_MAX_REQ_LEN  0x1000
>>>   #define CHSC_MIN_RESP_LEN 0x0008
>>>   #define CHSC_SCPD 0x0002
>>> @@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>>> ipb, uintptr_t ra)
>>>       uint16_t len;
>>>       uint16_t command;
>>>       CPUS390XState *env = &cpu->env;
>>> -    uint8_t buf[TARGET_PAGE_SIZE];
>>> +    uint8_t buf[CHSC_MAX_REQ_LEN];
>>>       trace_ioinst("chsc");
>>>       reg = (ipb >> 20) & 0x00f;
>>> @@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>>> ipb, uintptr_t ra)
>>>           s390_program_interrupt(env, PGM_OPERAND, ra);
>>>           return;
>>>       }
>>> -    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
>>> +    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
>>>       res = (void *)((char *)req + len);
>>>       command = be16_to_cpu(req->command);
>>>       trace_ioinst_chsc_cmd(command, len);
>>
>> Sorry, I don't quite get it, there are still dozends of others 
>> TARGET_PAGE_SIZE usages in target/s390x/, what's the advantage of 
>> removing that one here?

The other ones are really related to the s390x page size.

This one seems to use TARGET_PAGE_SIZE as an optimization.

> 
> The other ones are also used by user emulation so need more care. This
> series addresses all system-only files. I neglected to mention that in
> the cover :(
> 
> But if you think this is better to review all target/s390x/ conversion
> in one go, I can wait until I finish the rest.

Re: [PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Philippe Mathieu-Daudé 4 days, 20 hours ago

On 4/2/26 19:04, Philippe Mathieu-Daudé wrote:
> On 8/1/26 12:46, Philippe Mathieu-Daudé wrote:
>> On 8/1/26 08:53, Thomas Huth wrote:
>>> On 07/01/2026 14.08, Philippe Mathieu-Daudé wrote:
>>>> Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
>>>> in CHSC instruction") the access is no more tied to the page
>>>> size. Define CHSC_MAX_REQ_LEN to better express the relation
>>>> with the length of the Channel Subsystem Call request.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   target/s390x/ioinst.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>>>> index 2320dd4c122..d07773d14a3 100644
>>>> --- a/target/s390x/ioinst.c
>>>> +++ b/target/s390x/ioinst.c
>>>> @@ -398,6 +398,7 @@ typedef struct ChscResp {
>>>>       char data[];
>>>>   } QEMU_PACKED ChscResp;
>>>> +#define CHSC_MAX_REQ_LEN  0x1000
>>>>   #define CHSC_MIN_RESP_LEN 0x0008
>>>>   #define CHSC_SCPD 0x0002
>>>> @@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>>>> ipb, uintptr_t ra)
>>>>       uint16_t len;
>>>>       uint16_t command;
>>>>       CPUS390XState *env = &cpu->env;
>>>> -    uint8_t buf[TARGET_PAGE_SIZE];
>>>> +    uint8_t buf[CHSC_MAX_REQ_LEN];
>>>>       trace_ioinst("chsc");
>>>>       reg = (ipb >> 20) & 0x00f;
>>>> @@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t 
>>>> ipb, uintptr_t ra)
>>>>           s390_program_interrupt(env, PGM_OPERAND, ra);
>>>>           return;
>>>>       }
>>>> -    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
>>>> +    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
>>>>       res = (void *)((char *)req + len);
>>>>       command = be16_to_cpu(req->command);
>>>>       trace_ioinst_chsc_cmd(command, len);
>>>
>>> Sorry, I don't quite get it, there are still dozends of others 
>>> TARGET_PAGE_SIZE usages in target/s390x/, what's the advantage of 
>>> removing that one here?
> 
> The other ones are really related to the s390x page size.
> 
> This one seems to use TARGET_PAGE_SIZE as an optimization.

Also, making this file common, we get the page size at runtime,
and that triggers:

../../target/s390x/ioinst.c:663:17: error: variable length array used 
[-Werror,-Wvla]
   663 |     uint8_t buf[TARGET_PAGE_SIZE];
       |                 ^~~~~~~~~~~~~~~~
include/exec/target_page.h:42:30: note: expanded from macro 
'TARGET_PAGE_SIZE'
    42 | # define TARGET_PAGE_SIZE    (-(int)TARGET_PAGE_MASK)
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~
include/exec/target_page.h:40:49: note: expanded from macro 
'TARGET_PAGE_MASK'
    40 | #  define TARGET_PAGE_MASK   ((TARGET_PAGE_TYPE)target_page.mask)
       |                                                 ^
include/exec/target_page.h:32:29: note: declared here
    32 | extern const TargetPageBits target_page;
       |                             ^
1 error generated.

I'll drop this patch and let someone with proper s390x knowledge post
a proper fix when this will become a blocking issue, but today it isn't.

> 
>>
>> The other ones are also used by user emulation so need more care. This
>> series addresses all system-only files. I neglected to mention that in
>> the cover :(
>>
>> But if you think this is better to review all target/s390x/ conversion
>> in one go, I can wait until I finish the rest.
>

Re: [PATCH 13/18] target/s390x: Introduce CHSC_MAX_REQ_LEN definition

Posted by Thomas Huth 4 days, 6 hours ago

On 04/02/2026 19.08, Philippe Mathieu-Daudé wrote:
> On 4/2/26 19:04, Philippe Mathieu-Daudé wrote:
>> On 8/1/26 12:46, Philippe Mathieu-Daudé wrote:
>>> On 8/1/26 08:53, Thomas Huth wrote:
>>>> On 07/01/2026 14.08, Philippe Mathieu-Daudé wrote:
>>>>> Since commit 166f1bb7968 ("s390x/ioinst: Rework memory access
>>>>> in CHSC instruction") the access is no more tied to the page
>>>>> size. Define CHSC_MAX_REQ_LEN to better express the relation
>>>>> with the length of the Channel Subsystem Call request.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>   target/s390x/ioinst.c | 5 +++--
>>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>>>>> index 2320dd4c122..d07773d14a3 100644
>>>>> --- a/target/s390x/ioinst.c
>>>>> +++ b/target/s390x/ioinst.c
>>>>> @@ -398,6 +398,7 @@ typedef struct ChscResp {
>>>>>       char data[];
>>>>>   } QEMU_PACKED ChscResp;
>>>>> +#define CHSC_MAX_REQ_LEN  0x1000
>>>>>   #define CHSC_MIN_RESP_LEN 0x0008
>>>>>   #define CHSC_SCPD 0x0002
>>>>> @@ -660,7 +661,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, 
>>>>> uintptr_t ra)
>>>>>       uint16_t len;
>>>>>       uint16_t command;
>>>>>       CPUS390XState *env = &cpu->env;
>>>>> -    uint8_t buf[TARGET_PAGE_SIZE];
>>>>> +    uint8_t buf[CHSC_MAX_REQ_LEN];
>>>>>       trace_ioinst("chsc");
>>>>>       reg = (ipb >> 20) & 0x00f;
>>>>> @@ -690,7 +691,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, 
>>>>> uintptr_t ra)
>>>>>           s390_program_interrupt(env, PGM_OPERAND, ra);
>>>>>           return;
>>>>>       }
>>>>> -    memset((char *)req + len, 0, TARGET_PAGE_SIZE - len);
>>>>> +    memset((char *)req + len, 0, CHSC_MAX_REQ_LEN - len);
>>>>>       res = (void *)((char *)req + len);
>>>>>       command = be16_to_cpu(req->command);
>>>>>       trace_ioinst_chsc_cmd(command, len);
>>>>
>>>> Sorry, I don't quite get it, there are still dozends of others 
>>>> TARGET_PAGE_SIZE usages in target/s390x/, what's the advantage of 
>>>> removing that one here?
>>
>> The other ones are really related to the s390x page size.
>>
>> This one seems to use TARGET_PAGE_SIZE as an optimization.
> 
> Also, making this file common, we get the page size at runtime,
> and that triggers:
> 
> ../../target/s390x/ioinst.c:663:17: error: variable length array used [- 
> Werror,-Wvla]
>    663 |     uint8_t buf[TARGET_PAGE_SIZE];
>        |                 ^~~~~~~~~~~~~~~~

Using such big arrays on the stack is not good style anyway - unless this is 
a hot path, which I think it is not. So maybe you could simply post a patch 
that turns this into a "g_autofree uint8_t *buf = gmalloc(TARGET_PAGE_SIZE)" 
instead?

  Thomas