Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[wang.yi59]

Hi Eduardo,

Thank you for your reply!

>On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:

>> Add [vcpu] index support for hmp command "info lapic", which is

>> useful when debugging ipi and so on. Current behavior is not

>> changed when the parameter isn't specified.

>> 

>> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>

>> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>

>

>We have 8 monitor commands (see below) that use the CPU set by

>the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"

>special?

When we debugging a problem of ipi, we wanted to verify lapic info

on each vCPU, but we found that we could only get vCPU 0's lapic

through "info lapic", so we supposed this patch could help those

who have the same problem as us.




>

>* info cpustats

>* info lapic

>* info mem

>* info registers

>* info tlb

>* x

>* memsave

>* inject-nmi (QMP)

Monitor command "info registers" already have "-a" parameter to

show all vCPU's info. -)

May I send some new patches for the other monitor commands, which may

be helpful for analyzing multiple cpu problems?





---

Best wishes

Yi Wang

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Wed, Jul 19, 2017 at 12:47:53PM +0800, wang.yi59@zte.com.cn wrote:
> Hi Eduardo,
> 
> Thank you for your reply!
> 
> >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:
> >> Add [vcpu] index support for hmp command "info lapic", which is
> >> useful when debugging ipi and so on. Current behavior is not
> >> changed when the parameter isn't specified.
> >> 
> >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
> >
> 
> >We have 8 monitor commands (see below) that use the CPU set by
> >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"
> >special?
> 
> When we debugging a problem of ipi, we wanted to verify lapic info
> on each vCPU, but we found that we could only get vCPU 0's lapic
> through "info lapic", so we supposed this patch could help those
> who have the same problem as us.

The "cpu" command is supposed to allow you to select the CPU for
those commands.  Doesn't it work?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Wed, Jul 19, 2017 at 12:47:53PM +0800, wang.yi59@zte.com.cn wrote:
> > Hi Eduardo,
> > 
> > Thank you for your reply!
> > 
> > >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:
> > >> Add [vcpu] index support for hmp command "info lapic", which is
> > >> useful when debugging ipi and so on. Current behavior is not
> > >> changed when the parameter isn't specified.
> > >> 
> > >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
> > >
> > 
> > >We have 8 monitor commands (see below) that use the CPU set by
> > >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"
> > >special?
> > 
> > When we debugging a problem of ipi, we wanted to verify lapic info
> > on each vCPU, but we found that we could only get vCPU 0's lapic
> > through "info lapic", so we supposed this patch could help those
> > who have the same problem as us.
> 
> The "cpu" command is supposed to allow you to select the CPU for
> those commands.  Doesn't it work?

In the other arm to the thread Yi explained that they were driving
it via virsh qemu-monitor-command,  and I've just tried and it
doesn't seem to work; doing:

virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "cpu 1"
virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "info lapic"
dumping local APIC state for CPU 0

Dave

> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Wed, Jul 19, 2017 at 01:16:28PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Wed, Jul 19, 2017 at 12:47:53PM +0800, wang.yi59@zte.com.cn wrote:
> > > Hi Eduardo,
> > > 
> > > Thank you for your reply!
> > > 
> > > >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:
> > > >> Add [vcpu] index support for hmp command "info lapic", which is
> > > >> useful when debugging ipi and so on. Current behavior is not
> > > >> changed when the parameter isn't specified.
> > > >> 
> > > >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
> > > >
> > > 
> > > >We have 8 monitor commands (see below) that use the CPU set by
> > > >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"
> > > >special?
> > > 
> > > When we debugging a problem of ipi, we wanted to verify lapic info
> > > on each vCPU, but we found that we could only get vCPU 0's lapic
> > > through "info lapic", so we supposed this patch could help those
> > > who have the same problem as us.
> > 
> > The "cpu" command is supposed to allow you to select the CPU for
> > those commands.  Doesn't it work?
> 
> In the other arm to the thread Yi explained that they were driving
> it via virsh qemu-monitor-command,  and I've just tried and it
> doesn't seem to work; doing:
> 
> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "cpu 1"
> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "info lapic"
> dumping local APIC state for CPU 0

Right, the "cpu" command is useless inside a
'human-monitor-command' QMP command.  The 'cpu-index' argument
should be used instead. should make "cpu" print an error if ran
inside 'human-monitor-command' instead of silently pretend it
worked.

If virsh doesn't support the 'cpu-index' argument to
'human-monitor-command', it's possible to work around that
limitation by building your own QMP command.  e.g.:

  # virsh qemu-monitor-command f26test '{"execute":"human-monitor-command", "arguments":{"command-line":"info lapic", "cpu-index":1}}' | jq -r '.return'
  dumping local APIC state for CPU 1 
  
  LVT0     0x00010000 active-hi edge  masked                      Fixed  (vec 0)
  LVT1     0x00010000 active-hi edge  masked                      Fixed  (vec 0)
  LVTPC    0x00010000 active-hi edge  masked                      Fixed  (vec 0)
  LVTERR   0x00010000 active-hi edge  masked                      Fixed  (vec 0)
  LVTTHMR  0x00010000 active-hi edge  masked                      Fixed  (vec 0)
  LVTT     0x00010000 active-hi edge  masked         one-shot     Fixed  (vec 0)
  Timer    DCR=0x0 (divide by 2) initial_count = 0
  SPIV     0x000000ff APIC disabled, focus=off, spurious vec 255
  ICR      0x000000fd physical edge de-assert no-shorthand
  ICR2     0x00000000 cpu 0 (X2APIC ID)
  ESR      0x00000000
  ISR      (none)
  IRR      (none)
  
  APR 0x00 TPR 0x00 DFR 0x0f LDR 0x00 PPR 0x00


-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

[adding libvirt]

On 07/19/2017 07:41 AM, Eduardo Habkost wrote:

>> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "cpu 1"
>> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "info lapic"
>> dumping local APIC state for CPU 0
> 
> Right, the "cpu" command is useless inside a
> 'human-monitor-command' QMP command.  The 'cpu-index' argument
> should be used instead. should make "cpu" print an error if ran
> inside 'human-monitor-command' instead of silently pretend it
> worked.
> 
> If virsh doesn't support the 'cpu-index' argument to
> 'human-monitor-command',

It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
addition, although it's probably easier to just use QMP than HMP when
using 'virsh qemu-monitor-command' if HMP doesn't do what you want.

> it's possible to work around that
> limitation by building your own QMP command.  e.g.:
> 
>   # virsh qemu-monitor-command f26test '{"execute":"human-monitor-command", "arguments":{"command-line":"info lapic", "cpu-index":1}}' | jq -r '.return'

Indeed, there's the use of QMP to work around the HMP deficiency.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Daniel P. Berrange]

On Wed, Jul 19, 2017 at 10:02:04AM -0500, Eric Blake wrote:
> [adding libvirt]
> 
> On 07/19/2017 07:41 AM, Eduardo Habkost wrote:
> 
> >> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "cpu 1"
> >> virsh  qemu-monitor-command --domain rhel6.8 --hmp --cmd "info lapic"
> >> dumping local APIC state for CPU 0
> > 
> > Right, the "cpu" command is useless inside a
> > 'human-monitor-command' QMP command.  The 'cpu-index' argument
> > should be used instead. should make "cpu" print an error if ran
> > inside 'human-monitor-command' instead of silently pretend it
> > worked.
> > 
> > If virsh doesn't support the 'cpu-index' argument to
> > 'human-monitor-command',
> 
> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
> addition, although it's probably easier to just use QMP than HMP when
> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.

Or special case the "cpu 1" command - ie notice that it is being
requested and don't execute 'human-montor-command'. Instead just
record the CPU index, and use that for future "human-monitor-command"
invokations, so we get full compat with the (dubious) stateful HMP
semantics that traditionally existed.

> 
> > it's possible to work around that
> > limitation by building your own QMP command.  e.g.:
> > 
> >   # virsh qemu-monitor-command f26test '{"execute":"human-monitor-command", "arguments":{"command-line":"info lapic", "cpu-index":1}}' | jq -r '.return'
> 
> Indeed, there's the use of QMP to work around the HMP deficiency.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On 07/19/2017 10:07 AM, Daniel P. Berrange wrote:
>> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
>> addition, although it's probably easier to just use QMP than HMP when
>> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.
> 
> Or special case the "cpu 1" command - ie notice that it is being
> requested and don't execute 'human-montor-command'. Instead just
> record the CPU index, and use that for future "human-monitor-command"
> invokations, so we get full compat with the (dubious) stateful HMP
> semantics that traditionally existed.

Is 'cpu' (and the followup commands affected by it) the only stateful
HMP command pairing?  Is there a way to specify multiple HMP commands in
a single human-monitor-command QMP call?

Indeed, tweaking qemu's human-monitor-command call to track the state
might be cleaner than having libvirt have to tweak API to work around
this wart of HMP.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Wed, Jul 19, 2017 at 10:17:36AM -0500, Eric Blake wrote:
> On 07/19/2017 10:07 AM, Daniel P. Berrange wrote:
> >> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
> >> addition, although it's probably easier to just use QMP than HMP when
> >> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.
> > 
> > Or special case the "cpu 1" command - ie notice that it is being
> > requested and don't execute 'human-montor-command'. Instead just
> > record the CPU index, and use that for future "human-monitor-command"
> > invokations, so we get full compat with the (dubious) stateful HMP
> > semantics that traditionally existed.
> 
> Is 'cpu' (and the followup commands affected by it) the only stateful
> HMP command pairing?  Is there a way to specify multiple HMP commands in
> a single human-monitor-command QMP call?
> 
> Indeed, tweaking qemu's human-monitor-command call to track the state
> might be cleaner than having libvirt have to tweak API to work around
> this wart of HMP.

The CPU index was the only state kept by the human monitor, and I
think it's by design that it stopped being considered "monitor
state" to be tracked, and became just an argument to
human-monitor-command.

It's true that it broke compatibility of
  "virsh qemu-monitor-command <domain> --hmp 'cpu <n>'",
when we moved to QMP, but this happened years ago, and it looks
like nobody was relying on it.  I don't see the point of trying
to emulate the previous stateful interface.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Wed, Jul 19, 2017 at 10:17:36AM -0500, Eric Blake wrote:
> > On 07/19/2017 10:07 AM, Daniel P. Berrange wrote:
> > >> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
> > >> addition, although it's probably easier to just use QMP than HMP when
> > >> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.
> > > 
> > > Or special case the "cpu 1" command - ie notice that it is being
> > > requested and don't execute 'human-montor-command'. Instead just
> > > record the CPU index, and use that for future "human-monitor-command"
> > > invokations, so we get full compat with the (dubious) stateful HMP
> > > semantics that traditionally existed.
> > 
> > Is 'cpu' (and the followup commands affected by it) the only stateful
> > HMP command pairing?  Is there a way to specify multiple HMP commands in
> > a single human-monitor-command QMP call?
> > 
> > Indeed, tweaking qemu's human-monitor-command call to track the state
> > might be cleaner than having libvirt have to tweak API to work around
> > this wart of HMP.
> 
> The CPU index was the only state kept by the human monitor, and I
> think it's by design that it stopped being considered "monitor
> state" to be tracked, and became just an argument to
> human-monitor-command.
> 
> It's true that it broke compatibility of
>   "virsh qemu-monitor-command <domain> --hmp 'cpu <n>'",
> when we moved to QMP, but this happened years ago, and it looks
> like nobody was relying on it.  I don't see the point of trying
> to emulate the previous stateful interface.

IMHO Yi's fix (once reworked) is the right fix - it removes the
use of that piece of state, when the optional parameter is used.
(OK, so it needs rework not to change that state and to
come to some agreement as to what to use instead of cpu index number
etc).

Dave

> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Wed, Jul 19, 2017 at 08:17:49PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Wed, Jul 19, 2017 at 10:17:36AM -0500, Eric Blake wrote:
> > > On 07/19/2017 10:07 AM, Daniel P. Berrange wrote:
> > > >> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API
> > > >> addition, although it's probably easier to just use QMP than HMP when
> > > >> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.
> > > > 
> > > > Or special case the "cpu 1" command - ie notice that it is being
> > > > requested and don't execute 'human-montor-command'. Instead just
> > > > record the CPU index, and use that for future "human-monitor-command"
> > > > invokations, so we get full compat with the (dubious) stateful HMP
> > > > semantics that traditionally existed.
> > > 
> > > Is 'cpu' (and the followup commands affected by it) the only stateful
> > > HMP command pairing?  Is there a way to specify multiple HMP commands in
> > > a single human-monitor-command QMP call?
> > > 
> > > Indeed, tweaking qemu's human-monitor-command call to track the state
> > > might be cleaner than having libvirt have to tweak API to work around
> > > this wart of HMP.
> > 
> > The CPU index was the only state kept by the human monitor, and I
> > think it's by design that it stopped being considered "monitor
> > state" to be tracked, and became just an argument to
> > human-monitor-command.
> > 
> > It's true that it broke compatibility of
> >   "virsh qemu-monitor-command <domain> --hmp 'cpu <n>'",
> > when we moved to QMP, but this happened years ago, and it looks
> > like nobody was relying on it.  I don't see the point of trying
> > to emulate the previous stateful interface.
> 
> IMHO Yi's fix (once reworked) is the right fix - it removes the
> use of that piece of state, when the optional parameter is used.
> (OK, so it needs rework not to change that state and to
> come to some agreement as to what to use instead of cpu index number
> etc).

Agreed, as it helps us to keep the "virsh qemu-monitor-command"
interface simpler.  But we have 8 commands that use
mon_get_cpu(), we shouldn't fix only "info lapic".

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Dr. David Alan Gilbert]

* wang.yi59@zte.com.cn (wang.yi59@zte.com.cn) wrote:
> Hi Eduardo,
> 
> Thank you for your reply!
> 
> >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:
> 
> >> Add [vcpu] index support for hmp command "info lapic", which is
> 
> >> useful when debugging ipi and so on. Current behavior is not
> 
> >> changed when the parameter isn't specified.
> 
> >> 
> 
> >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> 
> >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
> 
> >
> 
> >We have 8 monitor commands (see below) that use the CPU set by
> >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"
> >special?
> 
> When we debugging a problem of ipi, we wanted to verify lapic info
> on each vCPU, but we found that we could only get vCPU 0's lapic
> through "info lapic", so we supposed this patch could help those
> who have the same problem as us.

I think Eduardo's point is that you can already do:
    cpu 0
    info lapic
    cpu 1
    info lapic

Dave
> 
> 
> 
> 
> >
> 
> >* info cpustats
> 
> >* info lapic
> 
> >* info mem
> 
> >* info registers
> 
> >* info tlb
> 
> >* x
> 
> >* memsave
> 
> >* inject-nmi (QMP)
> 
> Monitor command "info registers" already have "-a" parameter to
> 
> show all vCPU's info. -)
> 
> May I send some new patches for the other monitor commands, which may
> 
> be helpful for analyzing multiple cpu problems?
> 
> 
> 
> 
> 
> ---
> 
> Best wishes
> 
> Yi Wang


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[wang.yi59]

>On Wed, Jul 19, 2017 at 08:17:49PM +0100, Dr. David Alan Gilbert wrote:

>> * Eduardo Habkost (address@hidden) wrote:

>> > On Wed, Jul 19, 2017 at 10:17:36AM -0500, Eric Blake wrote:

>> > > On 07/19/2017 10:07 AM, Daniel P. Berrange wrote:

>> > > >> It doesn't.  Perhaps we should add that as a future libvirt-qemu.so API

>> > > >> addition, although it's probably easier to just use QMP than HMP when

>> > > >> using 'virsh qemu-monitor-command' if HMP doesn't do what you want.

>> > > > 

>> > > > Or special case the "cpu 1" command - ie notice that it is being

>> > > > requested and don't execute 'human-montor-command'. Instead just

>> > > > record the CPU index, and use that for future "human-monitor-command"

>> > > > invokations, so we get full compat with the (dubious) stateful HMP

>> > > > semantics that traditionally existed.

>> > > 

>> > > Is 'cpu' (and the followup commands affected by it) the only stateful

>> > > HMP command pairing?  Is there a way to specify multiple HMP commands in

>> > > a single human-monitor-command QMP call?

>> > > 

>> > > Indeed, tweaking qemu's human-monitor-command call to track the state

>> > > might be cleaner than having libvirt have to tweak API to work around

>> > > this wart of HMP.

>> > 

>> > The CPU index was the only state kept by the human monitor, and I

>> > think it's by design that it stopped being considered "monitor

>> > state" to be tracked, and became just an argument to

>> > human-monitor-command.

>> > 

>> > It's true that it broke compatibility of

>> >   "virsh qemu-monitor-command <domain> --hmp 'cpu <n>'",

>> > when we moved to QMP, but this happened years ago, and it looks

>> > like nobody was relying on it.  I don't see the point of trying

>> > to emulate the previous stateful interface.

>> 

>> IMHO Yi's fix (once reworked) is the right fix - it removes the

>> use of that piece of state, when the optional parameter is used.

>> (OK, so it needs rework not to change that state and to

>> come to some agreement as to what to use instead of cpu index number

>> etc).

>

>Agreed, as it helps us to keep the "virsh qemu-monitor-command"

>interface simpler.  But we have 8 commands that use

>mon_get_cpu(), we shouldn't fix only "info lapic".




Thank you all!

I will rework this patch in this way:

  - extend 'info registers' with apic id value instead of current, like this:

      CPU#1 (socket-id: a, core-id: b, thread-id: c, apic-id: d)

  - add parameter 'apic id' for 'info lapic'




As to other commands, I want to send some other patches 'cause in my opinion not

all commands need 'apic-id' as index.




---

Best wishes

Yi Wang

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[wang.yi59]

>* wang.yi59@zte.com.cn (wang.yi59@zte.com.cn) wrote:


>> Hi Eduardo,

>> 

>> Thank you for your reply!

>> 

>> >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:

>> 

>> >> Add [vcpu] index support for hmp command "info lapic", which is

>> 

>> >> useful when debugging ipi and so on. Current behavior is not

>> 

>> >> changed when the parameter isn't specified.

>> 

>> >> 

>> 

>> >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>

>> 

>> >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>

>> 

>> >

>> 

>> >We have 8 monitor commands (see below) that use the CPU set by

>> >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"

>> >special?

>> 

>> When we debugging a problem of ipi, we wanted to verify lapic info

>> on each vCPU, but we found that we could only get vCPU 0's lapic

>> through "info lapic", so we supposed this patch could help those

>> who have the same problem as us.

>

>I think Eduardo's point is that you can already do:

>    cpu 0

>    info lapic

>    cpu 1

>    info lapic

Yes, I get it, thank you.

The reason of the problem we met is that we use "virsh qemu-monitor-command",

so the 'cpu' command didn't work.








---

Best wishes

Yi Wang

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Igor Mammedov]

On Wed, 19 Jul 2017 16:48:23 +0800 (CST)
<wang.yi59@zte.com.cn> wrote:

> >* wang.yi59@zte.com.cn (wang.yi59@zte.com.cn) wrote:  
> 
> 
> >> Hi Eduardo,  
> 
> >>   
> 
> >> Thank you for your reply!  
> 
> >>   
> 
> >> >On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:  
> 
> >>   
> 
> >> >> Add [vcpu] index support for hmp command "info lapic", which is  
> 
> >>   
> 
> >> >> useful when debugging ipi and so on. Current behavior is not  
> 
> >>   
> 
> >> >> changed when the parameter isn't specified.  
> 
> >>   
> 
> >> >>   
> 
> >>   
> 
> >> >> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>  
> 
> >>   
> 
> >> >> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>  
> 
> >>   
> 
> >> >  
> 
> >>   
> 
> >> >We have 8 monitor commands (see below) that use the CPU set by  
> 
> >> >the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"  
> 
> >> >special?  
> 
> >>   
> 
> >> When we debugging a problem of ipi, we wanted to verify lapic info  
> 
> >> on each vCPU, but we found that we could only get vCPU 0's lapic  
> 
> >> through "info lapic", so we supposed this patch could help those  
> 
> >> who have the same problem as us.  
> 
> >  
> 
> >I think Eduardo's point is that you can already do:  
> 
> >    cpu 0  
> 
> >    info lapic  
> 
> >    cpu 1  
> 
> >    info lapic  
> 
> Yes, I get it, thank you.
> 
> The reason of the problem we met is that we use "virsh qemu-monitor-command",
> 
> so the 'cpu' command didn't work.
you can try to use qmp interface directly which supports specifying cpu for monitor commands:

qemu supports:

 -- Command: human-monitor-command

     Execute a command on the human monitor and return the output.

     Arguments:
     'command-line: string'
          the command to execute in the human monitor
     'cpu-index: int' (optional)
          The CPU to use for commands that require an implicit CPU

maybe "virsh qemu-monitor-command" can also do it, CCing libvirt list

> 
> 
> 
> 
> 
> 
> 
> 
> ---
> 
> Best wishes
> 
> Yi Wang

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[wang.yi59]

>On Mon, 17 Jul 2017 21:49:37 -0400

>Yi Wang <address@hidden> wrote:

>

>> Add [vcpu] index support for hmp command "info lapic", which is

>> useful when debugging ipi and so on. Current behavior is not

>> changed when the parameter isn't specified.

>we shouldn't expose cpu_index to users anymore,

>

>I would suggest using to use real APIC ID here but we don't

>have monitor command that returns APIC IDs for present cpus.




Would you like to explain the reason we shouldn't use cpu_index any

more, which is more straightforward than socket-id/core-id/thread-id?

As Eduardo wrote in the next reply, "CPU #<n>" is already a perfectly

good identifier for a human interface :-)

Many thanks.





---

Best wishes

Yi Wang

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Igor Mammedov]

On Wed, 19 Jul 2017 12:25:53 +0800 (CST)
<wang.yi59@zte.com.cn> wrote:

> >On Mon, 17 Jul 2017 21:49:37 -0400
> 
> >Yi Wang <address@hidden> wrote:
> 
> >
> 
> >> Add [vcpu] index support for hmp command "info lapic", which is
> 
> >> useful when debugging ipi and so on. Current behavior is not
> 
> >> changed when the parameter isn't specified.
> 
> >we shouldn't expose cpu_index to users anymore,
> 
> >
> 
> >I would suggest using to use real APIC ID here but we don't
> 
> >have monitor command that returns APIC IDs for present cpus.
> 
> 
> 
> 
> Would you like to explain the reason we shouldn't use cpu_index any
> 
> more, which is more straightforward than socket-id/core-id/thread-id?
> 
> As Eduardo wrote in the next reply, "CPU #<n>" is already a perfectly
> 
> good identifier for a human interface :-)
We are working on hiding cpu_index from user interface,
it's still work in progress but and having old commands, that use cpu_index
and haven't been fixed yet, doesn't mean that we should add more.

Anyways as Eduardo pointed out combo of cpu/info lapic should be sufficient
for your task and this patch seems unnecessary.

> 
> Many thanks.
> 
> 
> 
> 
> 
> ---
> 
> Best wishes
> 
> Yi Wang

[Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Yi Wang]

Add [vcpu] index support for hmp command "info lapic", which is
useful when debugging ipi and so on. Current behavior is not
changed when the parameter isn't specified.

Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
---
 hmp-commands-info.hx  | 6 +++---
 target/i386/monitor.c | 8 +++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index d9df238..c534b03 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -115,9 +115,9 @@ ETEXI
 #if defined(TARGET_I386)
     {
         .name       = "lapic",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show local apic state",
+        .args_type  = "vcpu:i?",
+        .params     = "[vcpu]",
+        .help       = "show local apic state (vcpu: vCPU to read, default is 0)",
         .cmd        = hmp_info_local_apic,
     },
 #endif
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 77ead60..813005e 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -632,8 +632,14 @@ const MonitorDef *target_monitor_defs(void)
 
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
 {
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs;
 
+    if (qdict_haskey(qdict, "vcpu")) {
+        int index = qdict_get_try_int(qdict, "vcpu", 0);
+        cs = qemu_get_cpu(index);
+    } else {
+        cs = mon_get_cpu();
+    }
     if (!cs) {
         monitor_printf(mon, "No CPU available\n");
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Igor Mammedov]

On Mon, 17 Jul 2017 21:49:37 -0400
Yi Wang <wang.yi59@zte.com.cn> wrote:

> Add [vcpu] index support for hmp command "info lapic", which is
> useful when debugging ipi and so on. Current behavior is not
> changed when the parameter isn't specified.
we shouldn't expose cpu_index to users anymore,

I would suggest using to use real APIC ID here but we don't
have monitor command that returns APIC IDs for present cpus.

"info hotpluggable-cpus" gives you a list of available CPUs
it also gives you qom_path to cpu so potentially you could
read apic-id property of cpu.

But we have only QMP variant of qom-get so monitor needs
addition of qom-get command that will be a wrapper around
QMP command.

It could be solved in 2 ways:
 * use socket-id/core-id/thread-id to specify desired cpu
   /possible values in 'info hotpluggable-cpus'/

 * use apic-id value to specify interrupt controller
   - apic-id could be retrieved with new qom-get
     (qom-get would also be useful to read other properties)
   - extend 'info registers' with apic id value
    for example instead of current:
    
     CPU#1
     EAX=00000c06 EBX=00000000 ECX=000002ff EDX=00000000
     ....

    it would look like:

     CPU#1 (socket-id: a, core-id: b, thread-id: c, apic-id: d)
     ...


> 
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>
> ---
>  hmp-commands-info.hx  | 6 +++---
>  target/i386/monitor.c | 8 +++++++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index d9df238..c534b03 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -115,9 +115,9 @@ ETEXI
>  #if defined(TARGET_I386)
>      {
>          .name       = "lapic",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show local apic state",
> +        .args_type  = "vcpu:i?",
> +        .params     = "[vcpu]",
> +        .help       = "show local apic state (vcpu: vCPU to read, default is 0)",
>          .cmd        = hmp_info_local_apic,
>      },
>  #endif
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 77ead60..813005e 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -632,8 +632,14 @@ const MonitorDef *target_monitor_defs(void)
>  
>  void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
>  {
> -    CPUState *cs = mon_get_cpu();
> +    CPUState *cs;
>  
> +    if (qdict_haskey(qdict, "vcpu")) {
> +        int index = qdict_get_try_int(qdict, "vcpu", 0);
> +        cs = qemu_get_cpu(index);
> +    } else {
> +        cs = mon_get_cpu();
> +    }
>      if (!cs) {
>          monitor_printf(mon, "No CPU available\n");
>          return;

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Tue, Jul 18, 2017 at 04:54:17PM +0200, Igor Mammedov wrote:
> On Mon, 17 Jul 2017 21:49:37 -0400
> Yi Wang <wang.yi59@zte.com.cn> wrote:
> 
> > Add [vcpu] index support for hmp command "info lapic", which is
> > useful when debugging ipi and so on. Current behavior is not
> > changed when the parameter isn't specified.
> we shouldn't expose cpu_index to users anymore,
> 
> I would suggest using to use real APIC ID here but we don't
> have monitor command that returns APIC IDs for present cpus.
> 
> "info hotpluggable-cpus" gives you a list of available CPUs
> it also gives you qom_path to cpu so potentially you could
> read apic-id property of cpu.
> 
> But we have only QMP variant of qom-get so monitor needs
> addition of qom-get command that will be a wrapper around
> QMP command.
> 
> It could be solved in 2 ways:
>  * use socket-id/core-id/thread-id to specify desired cpu
>    /possible values in 'info hotpluggable-cpus'/
> 
>  * use apic-id value to specify interrupt controller
>    - apic-id could be retrieved with new qom-get
>      (qom-get would also be useful to read other properties)
>    - extend 'info registers' with apic id value
>     for example instead of current:
>     
>      CPU#1
>      EAX=00000c06 EBX=00000000 ECX=000002ff EDX=00000000
>      ....
> 
>     it would look like:
> 
>      CPU#1 (socket-id: a, core-id: b, thread-id: c, apic-id: d)
>      ...

We already print "CPU #<n>" on "info cpus", so <n> is already a
perfectly good identifier for a human interface.  I think HMP
should not require any identifier that isn't a simple number that
is shown very prominently on "info cpus".

If we don't want to use cpu_index as an identifier anymore, we
can start printing arch ID instead of cpu_index on commands that
print "CPU #<n>", and change mon_get_cpu() and monitor_set_cpu()
accordingly.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Igor Mammedov]

On Tue, 18 Jul 2017 20:26:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jul 18, 2017 at 04:54:17PM +0200, Igor Mammedov wrote:
> > On Mon, 17 Jul 2017 21:49:37 -0400
> > Yi Wang <wang.yi59@zte.com.cn> wrote:
> > 
> > > Add [vcpu] index support for hmp command "info lapic", which is
> > > useful when debugging ipi and so on. Current behavior is not
> > > changed when the parameter isn't specified.
> > we shouldn't expose cpu_index to users anymore,
> > 
> > I would suggest using to use real APIC ID here but we don't
> > have monitor command that returns APIC IDs for present cpus.
> > 
> > "info hotpluggable-cpus" gives you a list of available CPUs
> > it also gives you qom_path to cpu so potentially you could
> > read apic-id property of cpu.
> > 
> > But we have only QMP variant of qom-get so monitor needs
> > addition of qom-get command that will be a wrapper around
> > QMP command.
> > 
> > It could be solved in 2 ways:
> >  * use socket-id/core-id/thread-id to specify desired cpu
> >    /possible values in 'info hotpluggable-cpus'/
> > 
> >  * use apic-id value to specify interrupt controller
> >    - apic-id could be retrieved with new qom-get
> >      (qom-get would also be useful to read other properties)
> >    - extend 'info registers' with apic id value
> >     for example instead of current:
> >     
> >      CPU#1
> >      EAX=00000c06 EBX=00000000 ECX=000002ff EDX=00000000
> >      ....
> > 
> >     it would look like:
> > 
> >      CPU#1 (socket-id: a, core-id: b, thread-id: c, apic-id: d)
> >      ...
> 
> We already print "CPU #<n>" on "info cpus", so <n> is already a
> perfectly good identifier for a human interface.  I think HMP
> should not require any identifier that isn't a simple number that
> is shown very prominently on "info cpus".
> 
> If we don't want to use cpu_index as an identifier anymore, we
> can start printing arch ID instead of cpu_index on commands that
> print "CPU #<n>", and change mon_get_cpu() and monitor_set_cpu()
> accordingly.
Looks like a good plan, but it probably should touch all commands
that use cpu_index, also I'd leave cpu_index in 'info cpus' where
it is till we faze it out from CLI.

Re: [Qemu-devel] [PATCH v2] hmp: allow cpu index for "info lapic"

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 09:49:37PM -0400, Yi Wang wrote:
> Add [vcpu] index support for hmp command "info lapic", which is
> useful when debugging ipi and so on. Current behavior is not
> changed when the parameter isn't specified.
> 
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> Signed-off-by: Yun Liu <liu.yunh@zte.com.cn>

We have 8 monitor commands (see below) that use the CPU set by
the "cpu" command (mon_get_cpu()) as input.  Why is "info lapic"
special?

* info cpustats
* info lapic
* info mem
* info registers
* info tlb
* x
* memsave
* inject-nmi (QMP)

-- 
Eduardo