Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE

2024-03-20 Thread Tao Su 
On Wed, Mar 20, 2024 at 08:17:36AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Tao,
> 
> On 20/3/24 07:02, Markus Armbruster wrote:
> > Tao Su  writes:
> > 
> > > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > > before calling monitor_puts().
> > > 
> > > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > > Reviwed-by: Xiaoyao Li 
> > > Signed-off-by: Tao Su 
> > > ---
> > >   target/i386/helper.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > > index 2070dd0dda..a9ff830a17 100644
> > > --- a/target/i386/helper.c
> > > +++ b/target/i386/helper.c
> > > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, 
> > > run_on_cpu_data data)
> > >   if (need_reset) {
> > >   emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> > > recursive);
> > > -monitor_puts(params->mon, msg);
> > > +if (params->mon)
> 
> Missing braces, see QEMU coding style:
> https://www.qemu.org/docs/master/devel/style.html#block-structure

Yes, I prefer to revert the broken part.

Anyway, I got this point and will always pay attention to it, thanks for
reminding.

> 
> > > +monitor_puts(params->mon, msg);
> > >   qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > >   qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > >   return;
> > 
> > Could instead revert the broken part of commit bf0c50d4aa85:
> > 
> >-monitor_puts(params->mon, msg);
> >+monitor_printf(params->mon, "%s", msg);
> > qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > 
> > Then the fact that we send the same message to monitor and log is again
> > more obvious.
> > 
> > Either way:
> > Reviewed-by: Markus Armbruster 
> > 
> > 
>

Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE

2024-03-20 Thread Tao Su 
On Wed, Mar 20, 2024 at 07:02:46AM +0100, Markus Armbruster wrote:
> Tao Su  writes:
> 
> > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > before calling monitor_puts().
> >
> > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > Reviwed-by: Xiaoyao Li 
> > Signed-off-by: Tao Su 
> > ---
> >  target/i386/helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index 2070dd0dda..a9ff830a17 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, 
> > run_on_cpu_data data)
> >  if (need_reset) {
> >  emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> >recursive);
> > -monitor_puts(params->mon, msg);
> > +if (params->mon)
> > +monitor_puts(params->mon, msg);
> >  qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> >  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >  return;
> 
> Could instead revert the broken part of commit bf0c50d4aa85:
> 
>   -monitor_puts(params->mon, msg);
>   +monitor_printf(params->mon, "%s", msg);
>qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> 
> Then the fact that we send the same message to monitor and log is again
> more obvious.

Good suggestion. I will send a v2 with this change.

> 
> Either way:
> Reviewed-by: Markus Armbruster 

Thanks for review!

>

Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE

2024-03-20 Thread Philippe Mathieu-Daudé 

Hi Tao,

On 20/3/24 07:02, Markus Armbruster wrote:

Tao Su  writes:


monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
may have a parameter with NULL monitor pointer. Check the monitor pointer
before calling monitor_puts().

Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li 
Signed-off-by: Tao Su 
---
  target/i386/helper.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..a9ff830a17 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data 
data)
  if (need_reset) {
  emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
recursive);
-monitor_puts(params->mon, msg);
+if (params->mon)


Missing braces, see QEMU coding style:
https://www.qemu.org/docs/master/devel/style.html#block-structure


+monitor_puts(params->mon, msg);
  qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
  return;


Could instead revert the broken part of commit bf0c50d4aa85:

   -monitor_puts(params->mon, msg);
   +monitor_printf(params->mon, "%s", msg);
qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);

Then the fact that we send the same message to monitor and log is again
more obvious.

Either way:
Reviewed-by: Markus Armbruster

Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE

2024-03-20 Thread Markus Armbruster 
Tao Su  writes:

> monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> may have a parameter with NULL monitor pointer. Check the monitor pointer
> before calling monitor_puts().
>
> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> Reviwed-by: Xiaoyao Li 
> Signed-off-by: Tao Su 
> ---
>  target/i386/helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 2070dd0dda..a9ff830a17 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, 
> run_on_cpu_data data)
>  if (need_reset) {
>  emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>recursive);
> -monitor_puts(params->mon, msg);
> +if (params->mon)
> +monitor_puts(params->mon, msg);
>  qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>  return;

Could instead revert the broken part of commit bf0c50d4aa85:

  -monitor_puts(params->mon, msg);
  +monitor_printf(params->mon, "%s", msg);
   qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);

Then the fact that we send the same message to monitor and log is again
more obvious.

Either way:
Reviewed-by: Markus Armbruster

[PATCH] target/i386: Check NULL monitor pointer when injecting MCE

2024-03-19 Thread Tao Su 
monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
may have a parameter with NULL monitor pointer. Check the monitor pointer
before calling monitor_puts().

Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li 
Signed-off-by: Tao Su 
---
 target/i386/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..a9ff830a17 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data 
data)
 if (need_reset) {
 emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
   recursive);
-monitor_puts(params->mon, msg);
+if (params->mon)
+monitor_puts(params->mon, msg);
 qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
 qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
 return;
-- 
2.34.1