[PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Zheyu Ma]

This commit adds validation checks for the MCOPRE and MCOSEL values in
the rcc_update_cfgr_register function. If the MCOPRE value exceeds
0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
corresponding clock mux is disabled. This helps in identifying and
handling invalid configurations in the RCC registers.

Reproducer:
cat << EOF | qemu-system-aarch64 -display \
none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
stdio
writeq 0x40021008 0xffffffff
EOF

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 hw/misc/stm32l4x5_rcc.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
index 417bd5e85f..59d428fa66 100644
--- a/hw/misc/stm32l4x5_rcc.c
+++ b/hw/misc/stm32l4x5_rcc.c
@@ -543,19 +543,31 @@ static void rcc_update_cfgr_register(Stm32l4x5RccState *s)
     uint32_t val;
     /* MCOPRE */
     val = FIELD_EX32(s->cfgr, CFGR, MCOPRE);
-    assert(val <= 0b100);
-    clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
-                         1, 1 << val);
+    if (val > 0b100) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid MCOPRE value: 0x%"PRIx32"\n",
+                      __func__, val);
+        clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
+    } else {
+        clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
+                             1, 1 << val);
+    }
 
     /* MCOSEL */
     val = FIELD_EX32(s->cfgr, CFGR, MCOSEL);
-    assert(val <= 0b111);
-    if (val == 0) {
+    if (val > 0b111) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid MCOSEL value: 0x%"PRIx32"\n",
+                      __func__, val);
         clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
     } else {
-        clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], true);
-        clock_mux_set_source(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
-                             val - 1);
+        if (val == 0) {
+            clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
+        } else {
+            clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], true);
+            clock_mux_set_source(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
+                                 val - 1);
+        }
     }
 
     /* STOPWUCK */
-- 
2.34.1

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Peter Maydell]

On Tue, 18 Jun 2024 at 14:56, Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> This commit adds validation checks for the MCOPRE and MCOSEL values in
> the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> corresponding clock mux is disabled. This helps in identifying and
> handling invalid configurations in the RCC registers.
>
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display \
> none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> stdio
> writeq 0x40021008 0xffffffff
> EOF
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  hw/misc/stm32l4x5_rcc.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)


Applied to target-arm.next, thanks. (I added a line
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2356
to the commit message.)

-- PMM

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Philippe Mathieu-Daudé]

On 18/6/24 15:55, Zheyu Ma wrote:
> This commit adds validation checks for the MCOPRE and MCOSEL values in
> the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> corresponding clock mux is disabled. This helps in identifying and
> handling invalid configurations in the RCC registers.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display \
> none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> stdio
> writeq 0x40021008 0xffffffff
> EOF
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>   hw/misc/stm32l4x5_rcc.c | 28 ++++++++++++++++++++--------
>   1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index 417bd5e85f..59d428fa66 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -543,19 +543,31 @@ static void rcc_update_cfgr_register(Stm32l4x5RccState *s)
>       uint32_t val;
>       /* MCOPRE */
>       val = FIELD_EX32(s->cfgr, CFGR, MCOPRE);
> -    assert(val <= 0b100);
> -    clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> -                         1, 1 << val);
> +    if (val > 0b100) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Invalid MCOPRE value: 0x%"PRIx32"\n",
> +                      __func__, val);

How the hardware handles that? I suppose it just ignores the
value, so I'd simply:

           return;

instead of disabling the clock...

Inès, Arnaud, what do you think?

> +        clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
> +    } else {
> +        clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> +                             1, 1 << val);
> +    }
>   
>       /* MCOSEL */
>       val = FIELD_EX32(s->cfgr, CFGR, MCOSEL);

MCOSEL is 3-bit wide, so ...

> -    assert(val <= 0b111);

... this condition is always true (and can be removed) ...

> -    if (val == 0) {
> +    if (val > 0b111) {

... and this path isn't reachable IIUC. So you added dead code.

> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Invalid MCOSEL value: 0x%"PRIx32"\n",
> +                      __func__, val);
>           clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
>       } else {
> -        clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], true);
> -        clock_mux_set_source(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> -                             val - 1);
> +        if (val == 0) {
> +            clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
> +        } else {
> +            clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], true);
> +            clock_mux_set_source(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> +                                 val - 1);
> +        }
>       }
>   
>       /* STOPWUCK */

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Peter Maydell]

On Fri, 21 Jun 2024 at 16:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 18/6/24 15:55, Zheyu Ma wrote:
> > This commit adds validation checks for the MCOPRE and MCOSEL values in
> > the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> > 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> > corresponding clock mux is disabled. This helps in identifying and
> > handling invalid configurations in the RCC registers.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> > stdio
> > writeq 0x40021008 0xffffffff
> > EOF
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >   hw/misc/stm32l4x5_rcc.c | 28 ++++++++++++++++++++--------
> >   1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> > index 417bd5e85f..59d428fa66 100644
> > --- a/hw/misc/stm32l4x5_rcc.c
> > +++ b/hw/misc/stm32l4x5_rcc.c
> > @@ -543,19 +543,31 @@ static void rcc_update_cfgr_register(Stm32l4x5RccState *s)
> >       uint32_t val;
> >       /* MCOPRE */
> >       val = FIELD_EX32(s->cfgr, CFGR, MCOPRE);
> > -    assert(val <= 0b100);
> > -    clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> > -                         1, 1 << val);
> > +    if (val > 0b100) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Invalid MCOPRE value: 0x%"PRIx32"\n",
> > +                      __func__, val);
>
> How the hardware handles that? I suppose it just ignores the
> value, so I'd simply:
>
>            return;
>
> instead of disabling the clock...
>
> Inès, Arnaud, what do you think?

The datasheet just says the values are reserved. The hardware
might do anything, including behaving completely weirdly (e.g
trying to divide the clock by something more than 16 and running
into timing problems as a result). I suggested "disable" in review
on the first version of this patch (you'll definitely notice the
guest bug then ;-)).

> > +        clock_mux_set_enable(&s->clock_muxes[RCC_CLOCK_MUX_MCO], false);
> > +    } else {
> > +        clock_mux_set_factor(&s->clock_muxes[RCC_CLOCK_MUX_MCO],
> > +                             1, 1 << val);
> > +    }
> >
> >       /* MCOSEL */
> >       val = FIELD_EX32(s->cfgr, CFGR, MCOSEL);
>
> MCOSEL is 3-bit wide, so ...
>
> > -    assert(val <= 0b111);
>
> ... this condition is always true (and can be removed) ...

Yes, I noticed that. But if you look at the datasheet[*] the actual
field in the register is 4 bits wide, because on some l4x5 SoCs
bit 4 is used for something. So it seemed to me reasonable to have
the guard against this, because at some point somebody might update
the FIELD definition to match the datasheet field width.

[*] https://www.st.com/resource/en/reference_manual/rm0351-stm32l47xxx-stm32l48xxx-stm32l49xxx-and-stm32l4axxx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

thanks
-- PMM

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Peter Maydell]

On Fri, 21 Jun 2024 at 13:59, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 18 Jun 2024 at 14:56, Zheyu Ma <zheyuma97@gmail.com> wrote:
> >
> > This commit adds validation checks for the MCOPRE and MCOSEL values in
> > the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> > 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> > corresponding clock mux is disabled. This helps in identifying and
> > handling invalid configurations in the RCC registers.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> > stdio
> > writeq 0x40021008 0xffffffff
> > EOF
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >  hw/misc/stm32l4x5_rcc.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
>
>
> Applied to target-arm.next, thanks. (I added a line
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2356
> to the commit message.)

I've just noticed that although I said I'd applied this
patch it somehow got lost and never made it upstream.
I have applied it to target-arm.next for real this time.
(This is the second patch around this timeframe that I
discovered I'd somehow accidentally dropped, so I just
did a check through all the patches I claimed to have
applied to target-arm.next in the last three months
to see if any others slipped through the net, but it
looks like there weren't any others.)

thanks, and apologies for the delay
-- PMM

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Michael Tokarev]

18.06.2024 16:55, Zheyu Ma wrote:
> This commit adds validation checks for the MCOPRE and MCOSEL values in
> the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> corresponding clock mux is disabled. This helps in identifying and
> handling invalid configurations in the RCC registers.
> 
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display \
> none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> stdio
> writeq 0x40021008 0xffffffff
> EOF

Is it a -stable material?

Thanks,

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH] hw/misc/stm32l4x5_rcc: Add validation for MCOPRE and MCOSEL values

[Peter Maydell]

On Wed, 14 Aug 2024 at 05:35, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 18.06.2024 16:55, Zheyu Ma wrote:
> > This commit adds validation checks for the MCOPRE and MCOSEL values in
> > the rcc_update_cfgr_register function. If the MCOPRE value exceeds
> > 0b100 or the MCOSEL value exceeds 0b111, an error is logged and the
> > corresponding clock mux is disabled. This helps in identifying and
> > handling invalid configurations in the RCC registers.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine b-l475e-iot01a -qtest \
> > stdio
> > writeq 0x40021008 0xffffffff
> > EOF
>
> Is it a -stable material?

No, it's not worthwhile. It turns a case where the guest
misprograms the register from an assert to a log-the-error.
No real guest code is going to do this.

thanks
-- PMM