Message ID | 20200313095232.2392-1-frankja@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v9] fixup! Fix subcode/pbt | expand |
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; >> } >> > >
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 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: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 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 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; }
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; }
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(-)