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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 06/12/2014 07:39 PM, Alexander Graf wrote:
> 
> 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?

Oh, right, this is a leftover in my head from when it was a CPU callback :)



-- 
Alexey