target/i386/monitor: synchronize cpu before printing lapic state

[PATCH] target/i386/monitor: synchronize cpu before printing lapic state

Posted by David Woodhouse 2 years, 3 months ago

From: David Woodhouse <dwmw@amazon.co.uk>

Where the local APIC is emulated by KVM, we need kvm_get_apic() to pull
the current state into userspace before it's printed. Otherwise we get
stale values.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 target/i386/monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 6512846327..0754d699ba 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -29,6 +29,7 @@
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
 #include "sysemu/kvm.h"
+#include "sysemu/hw_accel.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qapi-commands-misc.h"
@@ -655,6 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
     if (qdict_haskey(qdict, "apic-id")) {
         int id = qdict_get_try_int(qdict, "apic-id", 0);
         cs = cpu_by_arch_id(id);
+        cpu_synchronize_state(cs);
     } else {
         cs = mon_get_cpu(mon);
     }
-- 
2.34.1

Re: [PATCH] target/i386/monitor: synchronize cpu before printing lapic state

Posted by Dongli Zhang 2 years, 3 months ago

Hi David,

On 10/26/23 08:39, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Where the local APIC is emulated by KVM, we need kvm_get_apic() to pull
> the current state into userspace before it's printed. Otherwise we get
> stale values.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  target/i386/monitor.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 6512846327..0754d699ba 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -29,6 +29,7 @@
>  #include "monitor/hmp.h"
>  #include "qapi/qmp/qdict.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/hw_accel.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-misc-target.h"
>  #include "qapi/qapi-commands-misc.h"
> @@ -655,6 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
>      if (qdict_haskey(qdict, "apic-id")) {
>          int id = qdict_get_try_int(qdict, "apic-id", 0);
>          cs = cpu_by_arch_id(id);
> +        cpu_synchronize_state(cs);

AFAIR, there is a case that cs may be NULL here when I was sending the similar
bugfix long time ago.

https://lore.kernel.org/qemu-devel/20210701214051.1588-1-dongli.zhang@oracle.com/

... and resend:

https://lore.kernel.org/qemu-devel/20210908143803.29191-1-dongli.zhang@oracle.com/

... and resent by Daniel as part of another patchset (after review):

https://lore.kernel.org/qemu-devel/20211028155457.967291-19-berrange@redhat.com/


This utility is helpful for the diagnostic of loss of interrupt issue.

Dongli Zhang

>      } else {
>          cs = mon_get_cpu(mon);
>      }

Re: [PATCH] target/i386/monitor: synchronize cpu before printing lapic state

Posted by Dongli Zhang 2 years, 3 months ago

Hi David,

Thank you very much for the Reviewed-by in another thread.

I have re-based the patch and sent again.

https://lore.kernel.org/all/20231026211938.162815-1-dongli.zhang@oracle.com/

Dongli Zhang

On 10/26/23 09:39, Dongli Zhang wrote:
> Hi David,
> 
> On 10/26/23 08:39, David Woodhouse wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> Where the local APIC is emulated by KVM, we need kvm_get_apic() to pull
>> the current state into userspace before it's printed. Otherwise we get
>> stale values.
>>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>  target/i386/monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
>> index 6512846327..0754d699ba 100644
>> --- a/target/i386/monitor.c
>> +++ b/target/i386/monitor.c
>> @@ -29,6 +29,7 @@
>>  #include "monitor/hmp.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "sysemu/kvm.h"
>> +#include "sysemu/hw_accel.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-commands-misc-target.h"
>>  #include "qapi/qapi-commands-misc.h"
>> @@ -655,6 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
>>      if (qdict_haskey(qdict, "apic-id")) {
>>          int id = qdict_get_try_int(qdict, "apic-id", 0);
>>          cs = cpu_by_arch_id(id);
>> +        cpu_synchronize_state(cs);
> 
> AFAIR, there is a case that cs may be NULL here when I was sending the similar
> bugfix long time ago.
> 
> https://lore.kernel.org/qemu-devel/20210701214051.1588-1-dongli.zhang@oracle.com/
> 
> ... and resend:
> 
> https://lore.kernel.org/qemu-devel/20210908143803.29191-1-dongli.zhang@oracle.com/
> 
> ... and resent by Daniel as part of another patchset (after review):
> 
> https://lore.kernel.org/qemu-devel/20211028155457.967291-19-berrange@redhat.com/
> 
> 
> This utility is helpful for the diagnostic of loss of interrupt issue.
> 
> Dongli Zhang
> 
>>      } else {
>>          cs = mon_get_cpu(mon);
>>      }