| Message ID | 20211211191135.1764649-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm gicv3 ITS: Various bug fixes and refactorings | expand |
On 12/11/21 11:11 AM, Peter Maydell wrote: > In extract_table_params() we process each GITS_BASER<n> register. If > the register's Valid bit is not set, this means there is no > in-guest-memory table and so we should not try to interpret the other > fields in the register. This was incorrectly coded as a 'return' > rather than a 'break', so instead of looping round to process the > next GITS_BASER<n> we would stop entirely, treating any later tables > as being not valid also. > > This has no real guest-visible effects because (since we don't have > GITS_TYPER.HCC != 0) the guest must in any case set up all the > GITS_BASER<n> to point to valid tables, so this only happens in an > odd misbehaving-guest corner case. > > Fix the check to 'break', so that we leave the case statement and > loop back around to the next GITS_BASER<n>. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I suspect this was an accidental result from a refactoring at > some point in the development of the ITS code. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Peter Maydell <peter.maydell@linaro.org> writes: > In extract_table_params() we process each GITS_BASER<n> register. If > the register's Valid bit is not set, this means there is no > in-guest-memory table and so we should not try to interpret the other > fields in the register. This was incorrectly coded as a 'return' > rather than a 'break', so instead of looping round to process the > next GITS_BASER<n> we would stop entirely, treating any later tables > as being not valid also. > > This has no real guest-visible effects because (since we don't have > GITS_TYPER.HCC != 0) the guest must in any case set up all the > GITS_BASER<n> to point to valid tables, so this only happens in an > odd misbehaving-guest corner case. > > Fix the check to 'break', so that we leave the case statement and > loop back around to the next GITS_BASER<n>. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index f321f10189e..c97b9982ae1 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -795,7 +795,7 @@ static void extract_table_params(GICv3ITSState *s) s->dt.valid = FIELD_EX64(value, GITS_BASER, VALID); if (!s->dt.valid) { - return; + break; } s->dt.page_sz = page_sz; @@ -826,7 +826,7 @@ static void extract_table_params(GICv3ITSState *s) * hence writes are discarded if ct.valid is 0 */ if (!s->ct.valid) { - return; + break; } s->ct.page_sz = page_sz;
In extract_table_params() we process each GITS_BASER<n> register. If the register's Valid bit is not set, this means there is no in-guest-memory table and so we should not try to interpret the other fields in the register. This was incorrectly coded as a 'return' rather than a 'break', so instead of looping round to process the next GITS_BASER<n> we would stop entirely, treating any later tables as being not valid also. This has no real guest-visible effects because (since we don't have GITS_TYPER.HCC != 0) the guest must in any case set up all the GITS_BASER<n> to point to valid tables, so this only happens in an odd misbehaving-guest corner case. Fix the check to 'break', so that we leave the case statement and loop back around to the next GITS_BASER<n>. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I suspect this was an accidental result from a refactoring at some point in the development of the ITS code. --- hw/intc/arm_gicv3_its.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)