hw/s390x/ipl.h | 11 +++++++---- target/s390x/diag.c | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-)
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/ipl.h | 11 +++++++----
target/s390x/diag.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -261,15 +261,18 @@ static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
return true;
}
-static inline bool iplb_valid(IplParameterBlock *iplb)
+static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
{
switch (iplb->pbt) {
case S390_IPL_TYPE_FCP:
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
+ return (subcode == DIAG308_SET &&
+ be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
case S390_IPL_TYPE_CCW:
- return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
+ return (subcode == DIAG308_SET &&
+ be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
case S390_IPL_TYPE_PV:
- if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
+ if (subcode != DIAG308_PV_SET ||
+ be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
return false;
}
if (!ipl_valid_pv_header(iplb)) {
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index b1ca81633b83bbdc..d4f33db5c23c818d 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
- if (!iplb_valid(iplb)) {
+ if (!iplb_valid(iplb, subcode)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
--
2.25.1
On Fri, 13 Mar 2020 05:52:32 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/ipl.h | 11 +++++++----
> target/s390x/diag.c | 2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -261,15 +261,18 @@ static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
> return true;
> }
>
> -static inline bool iplb_valid(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
> {
> switch (iplb->pbt) {
> case S390_IPL_TYPE_FCP:
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
> + return (subcode == DIAG308_SET &&
> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
> case S390_IPL_TYPE_CCW:
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
> + return (subcode == DIAG308_SET &&
> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
> case S390_IPL_TYPE_PV:
> - if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
> + if (subcode != DIAG308_PV_SET ||
> + be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
> return false;
I'm not sure I like passing the subcode here...
> }
> if (!ipl_valid_pv_header(iplb)) {
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b1ca81633b83bbdc..d4f33db5c23c818d 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>
> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>
> - if (!iplb_valid(iplb)) {
> + if (!iplb_valid(iplb, subcode)) {
> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> goto out;
> }
...because you're basically checking whether you either have a valid
normal iplb, or a valid pv iplb, with the two being mutually exclusive,
IIUC. So what about introducing iplb_valid_pv and calling that for the
pv case? Would be a bit nicer to read, I think, and also matches what
you do for the STORE case.
On 3/16/20 3:27 PM, Cornelia Huck wrote:
> On Fri, 13 Mar 2020 05:52:32 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/ipl.h | 11 +++++++----
>> target/s390x/diag.c | 2 +-
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -261,15 +261,18 @@ static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
>> return true;
>> }
>>
>> -static inline bool iplb_valid(IplParameterBlock *iplb)
>> +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
>> {
>> switch (iplb->pbt) {
>> case S390_IPL_TYPE_FCP:
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
>> + return (subcode == DIAG308_SET &&
>> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
>> case S390_IPL_TYPE_CCW:
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
>> + return (subcode == DIAG308_SET &&
>> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
>> case S390_IPL_TYPE_PV:
>> - if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> + if (subcode != DIAG308_PV_SET ||
>> + be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> return false;
>
> I'm not sure I like passing the subcode here...
I could move this to diag.c and call it iplb_valid_for_subcode()
>
>> }
>> if (!ipl_valid_pv_header(iplb)) {
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index b1ca81633b83bbdc..d4f33db5c23c818d 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>
>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>
>> - if (!iplb_valid(iplb)) {
>> + if (!iplb_valid(iplb, subcode)) {
>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>> goto out;
>> }
>
> ...because you're basically checking whether you either have a valid
> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> IIUC. So what about introducing iplb_valid_pv and calling that for the
> pv case? Would be a bit nicer to read, I think, and also matches what
> you do for the STORE case.
>
The idea was to get rid of all of these ifs and elses and only have one
iplb_valid function. Your suggestion would defeat hiding that complexity
behind this function.
On Mon, 16 Mar 2020 15:47:41 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> On 3/16/20 3:27 PM, Cornelia Huck wrote:
> > On Fri, 13 Mar 2020 05:52:32 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >> hw/s390x/ipl.h | 11 +++++++----
> >> target/s390x/diag.c | 2 +-
> >> 2 files changed, 8 insertions(+), 5 deletions(-)
> >> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> >>
> >> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> >>
> >> - if (!iplb_valid(iplb)) {
> >> + if (!iplb_valid(iplb, subcode)) {
> >> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> >> goto out;
> >> }
> >
> > ...because you're basically checking whether you either have a valid
> > normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> > IIUC. So what about introducing iplb_valid_pv and calling that for the
> > pv case? Would be a bit nicer to read, I think, and also matches what
> > you do for the STORE case.
> >
>
> The idea was to get rid of all of these ifs and elses and only have one
> iplb_valid function. Your suggestion would defeat hiding that complexity
> behind this function.
I'd argue that this is a complexity we should not hide; for non-pv, we
can have several formats, for pv, only one, and we cannot use a pv iplb
in a non-pv context and vice versa.
On 16.03.20 15:54, Cornelia Huck wrote:
> On Mon, 16 Mar 2020 15:47:41 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> On 3/16/20 3:27 PM, Cornelia Huck wrote:
>>> On Fri, 13 Mar 2020 05:52:32 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>> hw/s390x/ipl.h | 11 +++++++----
>>>> target/s390x/diag.c | 2 +-
>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>
>
>>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>>>
>>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>>>
>>>> - if (!iplb_valid(iplb)) {
>>>> + if (!iplb_valid(iplb, subcode)) {
>>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>>> goto out;
>>>> }
>>>
>>> ...because you're basically checking whether you either have a valid
>>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
>>> IIUC. So what about introducing iplb_valid_pv and calling that for the
>>> pv case? Would be a bit nicer to read, I think, and also matches what
>>> you do for the STORE case.
>>>
>>
>> The idea was to get rid of all of these ifs and elses and only have one
>> iplb_valid function. Your suggestion would defeat hiding that complexity
>> behind this function.
>
> I'd argue that this is a complexity we should not hide; for non-pv, we
> can have several formats, for pv, only one, and we cannot use a pv iplb
> in a non-pv context and vice versa.
So you suggest to split these case statements?
case DIAG308_STORE:
case DIAG308_PV_STORE:
On Mon, 16 Mar 2020 16:04:00 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> On 16.03.20 15:54, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 15:47:41 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >
> >> On 3/16/20 3:27 PM, Cornelia Huck wrote:
> >>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>
> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>> ---
> >>>> hw/s390x/ipl.h | 11 +++++++----
> >>>> target/s390x/diag.c | 2 +-
> >>>> 2 files changed, 8 insertions(+), 5 deletions(-)
> >
> >
> >>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> >>>>
> >>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> >>>>
> >>>> - if (!iplb_valid(iplb)) {
> >>>> + if (!iplb_valid(iplb, subcode)) {
> >>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> >>>> goto out;
> >>>> }
> >>>
> >>> ...because you're basically checking whether you either have a valid
> >>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> >>> IIUC. So what about introducing iplb_valid_pv and calling that for the
> >>> pv case? Would be a bit nicer to read, I think, and also matches what
> >>> you do for the STORE case.
> >>>
> >>
> >> The idea was to get rid of all of these ifs and elses and only have one
> >> iplb_valid function. Your suggestion would defeat hiding that complexity
> >> behind this function.
> >
> > I'd argue that this is a complexity we should not hide; for non-pv, we
> > can have several formats, for pv, only one, and we cannot use a pv iplb
> > in a non-pv context and vice versa.
>
> So you suggest to split these case statements?
> case DIAG308_STORE:
> case DIAG308_PV_STORE:
Why? Those cases are already done in the way I suggest for these here
as well (i.e. keep common checks, just split the iplb handling.)
On 16.03.20 18:57, Cornelia Huck wrote:
> On Mon, 16 Mar 2020 16:04:00 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 16.03.20 15:54, Cornelia Huck wrote:
>>> On Mon, 16 Mar 2020 15:47:41 +0100
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> On 3/16/20 3:27 PM, Cornelia Huck wrote:
>>>>> On Fri, 13 Mar 2020 05:52:32 -0400
>>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>> hw/s390x/ipl.h | 11 +++++++----
>>>>>> target/s390x/diag.c | 2 +-
>>>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>>
>>>>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>>>>>
>>>>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>>>>>
>>>>>> - if (!iplb_valid(iplb)) {
>>>>>> + if (!iplb_valid(iplb, subcode)) {
>>>>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>>>>> goto out;
>>>>>> }
>>>>>
>>>>> ...because you're basically checking whether you either have a valid
>>>>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
>>>>> IIUC. So what about introducing iplb_valid_pv and calling that for the
>>>>> pv case? Would be a bit nicer to read, I think, and also matches what
>>>>> you do for the STORE case.
>>>>>
>>>>
>>>> The idea was to get rid of all of these ifs and elses and only have one
>>>> iplb_valid function. Your suggestion would defeat hiding that complexity
>>>> behind this function.
>>>
>>> I'd argue that this is a complexity we should not hide; for non-pv, we
>>> can have several formats, for pv, only one, and we cannot use a pv iplb
>>> in a non-pv context and vice versa.
>>
>> So you suggest to split these case statements?
>> case DIAG308_STORE:
>> case DIAG308_PV_STORE:
>
> Why? Those cases are already done in the way I suggest for these here
> as well (i.e. keep common checks, just split the iplb handling.)
This was more of a question. I was not sure what your suggestion was.
On Mon, 16 Mar 2020 20:42:33 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> On 16.03.20 18:57, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 16:04:00 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >
> >> On 16.03.20 15:54, Cornelia Huck wrote:
> >>> On Mon, 16 Mar 2020 15:47:41 +0100
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>
> >>>> On 3/16/20 3:27 PM, Cornelia Huck wrote:
> >>>>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>>>
> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>>>> ---
> >>>>>> hw/s390x/ipl.h | 11 +++++++----
> >>>>>> target/s390x/diag.c | 2 +-
> >>>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>>
> >>>>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> >>>>>>
> >>>>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> >>>>>>
> >>>>>> - if (!iplb_valid(iplb)) {
> >>>>>> + if (!iplb_valid(iplb, subcode)) {
> >>>>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> >>>>>> goto out;
> >>>>>> }
> >>>>>
> >>>>> ...because you're basically checking whether you either have a valid
> >>>>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> >>>>> IIUC. So what about introducing iplb_valid_pv and calling that for the
> >>>>> pv case? Would be a bit nicer to read, I think, and also matches what
> >>>>> you do for the STORE case.
> >>>>>
> >>>>
> >>>> The idea was to get rid of all of these ifs and elses and only have one
> >>>> iplb_valid function. Your suggestion would defeat hiding that complexity
> >>>> behind this function.
> >>>
> >>> I'd argue that this is a complexity we should not hide; for non-pv, we
> >>> can have several formats, for pv, only one, and we cannot use a pv iplb
> >>> in a non-pv context and vice versa.
> >>
> >> So you suggest to split these case statements?
> >> case DIAG308_STORE:
> >> case DIAG308_PV_STORE:
> >
> > Why? Those cases are already done in the way I suggest for these here
> > as well (i.e. keep common checks, just split the iplb handling.)
>
> This was more of a question. I was not sure what your suggestion was.
Sorry if I wasn't clear enough.
For the store case, you have
if (subcode == DIAG308_PV_STORE) {
iplb = s390_ipl_get_iplb_pv();
} else {
iplb = s390_ipl_get_iplb();
}
with the rest of the handling being identical. My suggestion was to use
something like
valid = subcode == DIAG308_PV_SET ? iplb_valid_pv(iplb) : iplb_valid(iplb);
if (!valid) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
On 3/16/20 3:54 PM, Cornelia Huck wrote:
> On Mon, 16 Mar 2020 15:47:41 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> On 3/16/20 3:27 PM, Cornelia Huck wrote:
>>> On Fri, 13 Mar 2020 05:52:32 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>> hw/s390x/ipl.h | 11 +++++++----
>>>> target/s390x/diag.c | 2 +-
>>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>
>
>>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>>>
>>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>>>
>>>> - if (!iplb_valid(iplb)) {
>>>> + if (!iplb_valid(iplb, subcode)) {
>>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>>> goto out;
>>>> }
>>>
>>> ...because you're basically checking whether you either have a valid
>>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
>>> IIUC. So what about introducing iplb_valid_pv and calling that for the
>>> pv case? Would be a bit nicer to read, I think, and also matches what
>>> you do for the STORE case.
>>>
>>S390_IPL_TYPE_CCW
>> The idea was to get rid of all of these ifs and elses and only have one
>> iplb_valid function. Your suggestion would defeat hiding that complexity
>> behind this function.
>
> I'd argue that this is a complexity we should not hide; for non-pv, we
> can have several formats, for pv, only one, and we cannot use a pv iplb
> in a non-pv context and vice versa.
>
Ok, then please let me split this out into a new function within diag.c.
Something like:
static bool diag308_pbt_subcode_validity(uint8_t pbt, uint64_t subcode)
{
if (subcode == DIAG308_SET) {
return (pbt == S390_IPL_TYPE_FCP || pbt == S390_IPL_TYPE_CCW)
} else if (subcode == DIAG308_PV_SET && pbt == S390_IPL_TYPE_PV) {
return true;
}
return false;
}
On Mon, 16 Mar 2020 16:05:03 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> On 3/16/20 3:54 PM, Cornelia Huck wrote:
> > On Mon, 16 Mar 2020 15:47:41 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >
> >> On 3/16/20 3:27 PM, Cornelia Huck wrote:
> >>> On Fri, 13 Mar 2020 05:52:32 -0400
> >>> Janosch Frank <frankja@linux.ibm.com> wrote:
> >>>
> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>> ---
> >>>> hw/s390x/ipl.h | 11 +++++++----
> >>>> target/s390x/diag.c | 2 +-
> >>>> 2 files changed, 8 insertions(+), 5 deletions(-)
> >
> >
> >>>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> >>>>
> >>>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> >>>>
> >>>> - if (!iplb_valid(iplb)) {
> >>>> + if (!iplb_valid(iplb, subcode)) {
> >>>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> >>>> goto out;
> >>>> }
> >>>
> >>> ...because you're basically checking whether you either have a valid
> >>> normal iplb, or a valid pv iplb, with the two being mutually exclusive,
> >>> IIUC. So what about introducing iplb_valid_pv and calling that for the
> >>> pv case? Would be a bit nicer to read, I think, and also matches what
> >>> you do for the STORE case.
> >>>
> >>S390_IPL_TYPE_CCW
> >> The idea was to get rid of all of these ifs and elses and only have one
> >> iplb_valid function. Your suggestion would defeat hiding that complexity
> >> behind this function.
> >
> > I'd argue that this is a complexity we should not hide; for non-pv, we
> > can have several formats, for pv, only one, and we cannot use a pv iplb
> > in a non-pv context and vice versa.
> >
>
> Ok, then please let me split this out into a new function within diag.c.
> Something like:
>
> static bool diag308_pbt_subcode_validity(uint8_t pbt, uint64_t subcode)
> {
> if (subcode == DIAG308_SET) {
> return (pbt == S390_IPL_TYPE_FCP || pbt == S390_IPL_TYPE_CCW)
> } else if (subcode == DIAG308_PV_SET && pbt == S390_IPL_TYPE_PV) {
> return true;
> }
>
> return false;
> }
>
Sorry, you now managed to confuse me... where is that supposed to be
called?
On 13.03.20 10:52, Janosch Frank wrote:
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
looks sane. You will merge this?
> ---
> hw/s390x/ipl.h | 11 +++++++----
> target/s390x/diag.c | 2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -261,15 +261,18 @@ static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
> return true;
> }
>
> -static inline bool iplb_valid(IplParameterBlock *iplb)
> +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
> {
> switch (iplb->pbt) {
> case S390_IPL_TYPE_FCP:
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
> + return (subcode == DIAG308_SET &&
> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
> case S390_IPL_TYPE_CCW:
> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
> + return (subcode == DIAG308_SET &&
> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
> case S390_IPL_TYPE_PV:
> - if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
> + if (subcode != DIAG308_PV_SET ||
> + be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
> return false;
> }
> if (!ipl_valid_pv_header(iplb)) {
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index b1ca81633b83bbdc..d4f33db5c23c818d 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>
> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>
> - if (!iplb_valid(iplb)) {
> + if (!iplb_valid(iplb, subcode)) {
> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> goto out;
> }
>
On 3/13/20 3:30 PM, Christian Borntraeger wrote:
>
>
> On 13.03.20 10:52, Janosch Frank wrote:
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
> looks sane. You will merge this?
Yes, just did.
>
>> ---
>> hw/s390x/ipl.h | 11 +++++++----
>> target/s390x/diag.c | 2 +-
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 95e3183c9cccf8b6..f799f7cfcf4763b1 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -261,15 +261,18 @@ static inline bool ipl_valid_pv_header(IplParameterBlock *iplb)
>> return true;
>> }
>>
>> -static inline bool iplb_valid(IplParameterBlock *iplb)
>> +static inline bool iplb_valid(IplParameterBlock *iplb, uint64_t subcode)
>> {
>> switch (iplb->pbt) {
>> case S390_IPL_TYPE_FCP:
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN;
>> + return (subcode == DIAG308_SET &&
>> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN);
>> case S390_IPL_TYPE_CCW:
>> - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN;
>> + return (subcode == DIAG308_SET &&
>> + be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN);
>> case S390_IPL_TYPE_PV:
>> - if (be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> + if (subcode != DIAG308_PV_SET ||
>> + be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN) {
>> return false;
>> }
>> if (!ipl_valid_pv_header(iplb)) {
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index b1ca81633b83bbdc..d4f33db5c23c818d 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -118,7 +118,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>
>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>>
>> - if (!iplb_valid(iplb)) {
>> + if (!iplb_valid(iplb, subcode)) {
>> env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>> goto out;
>> }
>>
>
>
© 2016 - 2024 Red Hat, Inc.