Re: [Qemu-devel] [PATCH 7/8] hmp: added io apic dump state

[Pavel <pbutsykin@odin.com>]

On 19.06.2015 18:53, Andreas Färber wrote:
> Am 19.06.2015 um 16:48 schrieb Denis V. Lunev:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> Added the hmp command to query io apic state, may be usefull after guest
>> crashes to understand IRQ routing in guest.
>>
>> Implementation is only for kvm here. The dump will look like
>> (qemu) info apic-io
>> ioapic ID=00 IRR=00000000 SEL=18
>> ioapic 00 00000000000100ff: int=ff delmod=0:Fixed  P.H.EM dest=0
>> ioapic 01 0300000000000993: int=93 delmod=1:LowPri L.H.E. dest=3
>> ...
>> ioapic 23 00000000000100ff: int=ff delmod=0:Fixed  P.H.EM dest=0
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>   hmp-commands.hx                   |  2 ++
>>   hw/i386/kvm/ioapic.c              | 10 ++++++++++
>>   hw/intc/ioapic_common.c           | 31 +++++++++++++++++++++++++++++++
>>   include/hw/i386/ioapic_internal.h |  2 ++
>>   include/hw/i386/pc.h              |  4 ++++
>>   monitor.c                         | 20 ++++++++++++++++++++
>>   6 files changed, 69 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 95f554e..894ff65 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1726,6 +1726,8 @@ show block device statistics
>>   show the cpu registers
>>   @item info apic-local
>>   show local APIC state
>> +@item info apic-io
>> +show io APIC state
>>   @item info cpus
>>   show infos for each CPU
>>   @item info history
>> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
>> index d2a6c4c..b7390ca 100644
>> --- a/hw/i386/kvm/ioapic.c
>> +++ b/hw/i386/kvm/ioapic.c
>> @@ -10,6 +10,7 @@
>>    * See the COPYING file in the top-level directory.
>>    */
>>   
>> +#include "monitor/monitor.h"
>>   #include "hw/i386/pc.h"
>>   #include "hw/i386/ioapic_internal.h"
>>   #include "hw/i386/apic_internal.h"
>> @@ -110,6 +111,15 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
>>       }
>>   }
>>   
>> +void kvm_ioapic_dump_state(Monitor *mon, const QDict *qdict)
>> +{
>> +    IOAPICCommonState s;
>> +
>> +    kvm_ioapic_get(&s);
>> +
>> +    ioapic_print_redtbl(mon, &s);
>> +}
>> +
>>   static void kvm_ioapic_reset(DeviceState *dev)
>>   {
>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
>> index 8b7d118..83edf69 100644
>> --- a/hw/intc/ioapic_common.c
>> +++ b/hw/intc/ioapic_common.c
>> @@ -19,6 +19,7 @@
>>    * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>    */
>>   
>> +#include "monitor/monitor.h"
>>   #include "hw/i386/ioapic.h"
>>   #include "hw/i386/ioapic_internal.h"
>>   #include "hw/sysbus.h"
>> @@ -31,6 +32,36 @@
>>    */
>>   int ioapic_no;
>>   
>> +void ioapic_print_redtbl(Monitor *mon, IOAPICCommonState *s)
>> +{
>> +    static const char *delm_str[] = {
>> +        "Fixed", "LowPri", "SMI", "...", "NMI", "INIT", "...", "ExtINT"};
>> +    int i;
>> +
>> +    monitor_printf(mon, "ioapic ID=%02x IRR=%08x SEL=%02x\n",
>> +                   s->id, s->irr, s->ioregsel);
>> +
>> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>> +        uint64_t entry = s->ioredtbl[i];
>> +        uint32_t delm = (uint32_t)((entry & IOAPIC_LVT_DELIV_MODE) >>
>> +                                   IOAPIC_LVT_DELIV_MODE_SHIFT);
>> +        monitor_printf(mon, "ioapic %02u %016jx: int=%02jx "
>> +                       "delmod=%x:%-6s %c%c%c%c%c%c dest=%jx\n",
>> +                       i, entry,
>> +                       entry & IOAPIC_VECTOR_MASK,
>> +                       delm,
>> +                       delm_str[delm],
>> +                       entry & IOAPIC_LVT_DEST_MODE ? 'L' : 'P',
>> +                       entry & IOAPIC_LVT_DELIV_STATUS ? 'P' : '.',
>> +                       entry & IOAPIC_LVT_POLARITY ? 'L' : 'H',
>> +                       entry & IOAPIC_LVT_REMOTE_IRR ? 'R' : '.',
>> +                       entry & IOAPIC_LVT_TRIGGER_MODE ? 'L' : 'E',
>> +                       entry & IOAPIC_LVT_MASKED ? 'M' : '.',
>> +                       (entry >> IOAPIC_LVT_DEST_SHIFT) &
>> +                            (entry & IOAPIC_LVT_DEST_MODE ? 0xff : 0xf));
>> +    }
>> +}
>> +
>>   void ioapic_reset_common(DeviceState *dev)
>>   {
>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>> diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
>> index 4f7764e..797ed47 100644
>> --- a/include/hw/i386/ioapic_internal.h
>> +++ b/include/hw/i386/ioapic_internal.h
>> @@ -105,4 +105,6 @@ struct IOAPICCommonState {
>>   
>>   void ioapic_reset_common(DeviceState *dev);
>>   
>> +void ioapic_print_redtbl(Monitor *mon, IOAPICCommonState *s);
>> +
>>   #endif /* !QEMU_IOAPIC_INTERNAL_H */
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 86c5651..437085f 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -123,6 +123,10 @@ int pic_get_output(DeviceState *d);
>>   void hmp_info_pic(Monitor *mon, const QDict *qdict);
>>   void hmp_info_irq(Monitor *mon, const QDict *qdict);
>>   
>> +/* ioapic.c */
>> +
>> +void kvm_ioapic_dump_state(Monitor *mon, const QDict *qdict);
>> +
>>   /* Global System Interrupts */
>>   
>>   #define GSI_NUM_PINS IOAPIC_NUM_PINS
>> diff --git a/monitor.c b/monitor.c
>> index aad2792..5449663 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -957,6 +957,19 @@ int monitor_get_cpu_index(void)
>>       return cpu->cpu_index;
>>   }
>>   
>> +#if defined(TARGET_I386)
>> +static void hmp_info_apic_io(Monitor *mon, const QDict *qdict)
>> +{
>> +    if (kvm_irqchip_in_kernel()) {
>> +        kvm_ioapic_dump_state(mon, qdict);
>> +    }
> Why no else clause for TCG?
See next commit pls

>
>> +}
>> +#else
>> +static void hmp_info_apic_io(Monitor *mon, const QDict *qdict)
>> +{
>> +}
> Rather than having a no-op info apic-io on ARM etc., I would suggest to
> #ifdef the below array entry too, so that it is for x86 exclusively.
>
> Regards,
> Andreas
Yes, thanks. I think it needs to be fixed..

>> +#endif
>> +
>>   static void hmp_info_apic_local(Monitor *mon, const QDict *qdict)
>>   {
>>       CPUState *cpu;
>> @@ -2588,6 +2601,13 @@ static mon_cmd_t info_cmds[] = {
>>           .mhandler.cmd = hmp_info_apic_local,
>>       },
>>       {
>> +        .name       = "apic-io",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "show io apic state",
>> +        .mhandler.cmd = hmp_info_apic_io,
>> +    },
>> +    {
>>           .name       = "cpus",
>>           .args_type  = "",
>>           .params     = "",