Re: [Qemu-devel] [PATCH v6 2/4] target-s390x: Migrate to new NMI interface

[Alexander Graf <agraf@suse.de>]

On 12.06.14 11:38, Alexey Kardashevskiy wrote:
> On 06/12/2014 04:31 PM, Cornelia Huck wrote:
>> On Thu, 12 Jun 2014 03:03:01 +1000
>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>> This implements an NMI interface for s390 machine.
>>>
>>> This removes #ifdef s390 branch in qmp_inject_nmi so new s390's
>>> nmi_monitor_handler() callback is going to be used for NMI.
>>>
>>> Since nmi_monitor_handler()-calling code is platform independent,
>>> CPUState::cpu_index is used instead of S390CPU::env.cpu_num.
>>> There should not be any change in behaviour as both @cpu_index and
>>> @cpu_num are global CPU numbers.
>>>
>>> Also, s390_cpu_restart() takes care of preforming operations in
>>> the specific CPU thread so no extra measure is required here either.
>>>
>>> Since the only error s390_cpu_restart() can return is ENOSYS, convert
>>> it to QERR_UNSUPPORTED.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v6:
>>> * supported NMI interface
>>>
>>> v5:
>>> * added ENOSYS -> QERR_UNSUPPORTED, qapi/qmp/qerror.h was added for this
>>>
>>> v4:
>>> * s/\<nmi\>/nmi_monitor_handler/
>>>
>>> v3:
>>> * now contains both old code removal and new code insertion, easier to
>>> track changes
>>>
>>> ---
>>> Is there any good reason to have @cpu_num in addition to @cpu_index?
>>> Just asking :)
>>> ---
>>>   cpus.c                 | 14 --------------
>>>   hw/s390x/s390-virtio.c | 31 +++++++++++++++++++++++++++++++
>>>   target-s390x/cpu.c     |  1 +
>>>   3 files changed, 32 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>>> index 93c7ace..9c5b7b2 100644
>>> --- a/hw/s390x/s390-virtio.c
>>> +++ b/hw/s390x/s390-virtio.c
>>> @@ -38,6 +38,7 @@
>>>   #include "hw/s390x/sclp.h"
>>>   #include "hw/s390x/s390_flic.h"
>>>   #include "hw/s390x/s390-virtio.h"
>>> +#include "hw/nmi.h"
>>>
>>>   //#define DEBUG_S390
>>>
>>> @@ -51,6 +52,7 @@
>>>
>>>   #define MAX_BLK_DEVS                    10
>>>   #define ZIPL_FILENAME                   "s390-zipl.rom"
>>> +#define TYPE_NMI_S390                   "s390_nmi"
>> I'd prefer "s390-nmi" instead.
>>
>>>   static VirtIOS390Bus *s390_bus;
>>>   static S390CPU **ipi_states;
>>> @@ -277,6 +279,9 @@ static void s390_init(MachineState *machine)
>>>
>>>       /* Create VirtIO network adapters */
>>>       s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
>>> +
>>> +    object_property_add_child(OBJECT(machine), "nmi",
>>> +                              object_new(TYPE_NMI_S390), NULL);
>> This only adds the nmi interface to the old s390-virtio machine; we
>> want this for the s390-virtio-ccw machine as well.
>>
>>>   }
>>>
>>>   static QEMUMachine s390_machine = {
>>> @@ -295,8 +300,34 @@ static QEMUMachine s390_machine = {
>>>       .is_default = 1,
>>>   };
>>>
>>> +static void s390_nmi(NMI *n, int cpu_index, Error **errp)
>>> +{
>>> +    CPUState *cs = qemu_get_cpu(cpu_index);
>>> +
>>> +    if (s390_cpu_restart(S390_CPU(cs))) {
>>> +        error_set(errp, QERR_UNSUPPORTED);
>>> +    }
>>> +}
>>> +
>>> +static void s390_nmi_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    NMIClass *nc = NMI_CLASS(oc);
>>> +    nc->nmi_monitor_handler = s390_nmi;
>>> +}
>>> +
>>> +static const TypeInfo s390_nmi_info = {
>>> +    .name          = TYPE_NMI_S390,
>>> +    .parent        = TYPE_OBJECT,
>>> +    .class_init    = s390_nmi_class_init,
>>> +    .interfaces = (InterfaceInfo[]) {
>>> +        { TYPE_NMI },
>>> +        { }
>>> +    },
>>> +};
>>> +
>>>   static void s390_machine_init(void)
>>>   {
>>> +    type_register_static(&s390_nmi_info);
>> s390-virtio-ccw needs this as well.
>>
>>>       qemu_register_machine(&s390_machine);
>>>   }
>> The best way would probably be to put all nmi-related things into a new
>> file that registers the nmi type and provides an s390_register_nmi()
>> helper.
>
> I pushed some version to git@github.com:aik/qemu.git , branch nmi-v7
> Please have a look and give it a go - I do not have s390 kernel/images
> handy. Thanks!
>
> It does not look like we really need a new file for NMI now.
> Also, should I put it under #ifdef CONFIG_USER_ONLY on s390?

The machine only ever gets compiled on for non-user, so why would you 
need any #ifdefs?


Alex