diff mbox series

hw/core: Fix data type in do_nmi()

Message ID 20191206063642.40942-1-gshan@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/core: Fix data type in do_nmi() | expand

Commit Message

Gavin Shan Dec. 6, 2019, 6:36 a.m. UTC
object_dynamic_cast() should return machine (or GPIO) state instad of NMI
state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.

This changes the prototype of NMIClass::nmi_monitor_handler() to accept
the parent object of NMI state instead of itself. With this, he parent object
is passed to the function, to avoid potential data corruption.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/core/nmi.c              | 8 ++++----
 hw/i386/x86.c              | 2 +-
 hw/misc/macio/gpio.c       | 6 +++---
 hw/ppc/spapr.c             | 2 +-
 hw/s390x/s390-virtio-ccw.c | 2 +-
 include/hw/nmi.h           | 2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

Comments

Greg Kurz Dec. 6, 2019, 4:50 p.m. UTC | #1
On Fri,  6 Dec 2019 17:36:42 +1100
Gavin Shan <gshan@redhat.com> wrote:

> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
> 

object_dynamic_cast() returns either its first argument if it can be cast
to the given typename or NULL. Since the x86, ccw and spapr machines and
the PowerMac GPIO implement the NMI interface, nothing is wrong here.

The do_nmi() function is called for all objects in the QOM tree and simply
filters out the ones that don't implement the NMI interface.

I do agree that this isn't super obvious though. It would be clearer to
do the filtering in the foreach() function. Something like:

https://patchwork.ozlabs.org/patch/1182232/

> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
> the parent object of NMI state instead of itself. With this, he parent object
> is passed to the function, to avoid potential data corruption.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/core/nmi.c              | 8 ++++----
>  hw/i386/x86.c              | 2 +-
>  hw/misc/macio/gpio.c       | 6 +++---
>  hw/ppc/spapr.c             | 2 +-
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  include/hw/nmi.h           | 2 +-
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> index 481c4b3c7e..554708d0db 100644
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
>  static int do_nmi(Object *o, void *opaque)
>  {
>      struct do_nmi_s *ns = opaque;
> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
>  
> -    if (n) {
> -        NMIClass *nc = NMI_GET_CLASS(n);
> +    if (parent) {
> +        NMIClass *nc = NMI_GET_CLASS(parent);
>  
>          ns->handled = true;
> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
>          if (ns->err) {
>              return -1;
>          }
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 394edc2f72..b98204f104 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>      return ms->possible_cpus;
>  }
>  
> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
>  {
>      /* cpu index isn't used */
>      CPUState *cs;
> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
> index 6cca6b27d6..6b4dfcc188 100644
> --- a/hw/misc/macio/gpio.c
> +++ b/hw/misc/macio/gpio.c
> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
>      macio_set_gpio(s, 1, true);
>  }
>  
> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
>  {
> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
>  }
>  
>  static void macio_gpio_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..3b92e4013d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>      ppc_cpu_do_system_reset(cs);
>  }
>  
> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
>  {
>      CPUState *cs;
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad..a49952a8b9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
>      s390x_new_cpu(object_class_get_name(oc), id, errp);
>  }
>  
> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
>  {
>      CPUState *cs = qemu_get_cpu(cpu_index);
>  
> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> index a1e128724e..75afa67790 100644
> --- a/include/hw/nmi.h
> +++ b/include/hw/nmi.h
> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
>  typedef struct NMIClass {
>      InterfaceClass parent_class;
>  
> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>  } NMIClass;
>  
>  void nmi_monitor_handle(int cpu_index, Error **errp);
Gavin Shan Dec. 7, 2019, 1:14 p.m. UTC | #2
On 12/7/19 3:50 AM, Greg Kurz wrote:
> On Fri,  6 Dec 2019 17:36:42 +1100
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
>> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
>>
> 
> object_dynamic_cast() returns either its first argument if it can be cast
> to the given typename or NULL. Since the x86, ccw and spapr machines and
> the PowerMac GPIO implement the NMI interface, nothing is wrong here.
> 
> The do_nmi() function is called for all objects in the QOM tree and simply
> filters out the ones that don't implement the NMI interface.
> 
> I do agree that this isn't super obvious though. It would be clearer to
> do the filtering in the foreach() function. Something like:
> 
> https://patchwork.ozlabs.org/patch/1182232/
> 

Greg, Thanks for the review and comments. I think we're talking about
different issues. I do agree it's worthy to simplify the code with the
object_child_foreach_type(), but later.

When we have below parameters to qemu, object_dynamic_cast(o, TYPE_NMI)
returns object of "pc-q35-4.2-machine", which is passed to the subquent
NMIClass::nmi_monitor_handler(). However, the function expects a NMIState
from its prototype as below. This patch changes the prototype to fill the
gap.

    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);

    Changed to:

    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);

    ~/sandbox/src/qemu/qemu.main.x86/x86_64-softmmu/qemu-system-x86_64              \
    --enable-kvm -machine type=q35,accel=kvm,kernel-irqchip=on -cpu host -smp 2 -m 1G    \
    -monitor none -serial mon:stdio -nographic -s                                        \
    -bios ~/sandbox/src/firmware/seabios/out/bios.bin                                    \
    -kernel ~/sandbox/src/linux/linux.main.x86/arch/x86/boot/bzImage                     \
    -initrd ~/sandbox/src/util/buildroot/output/images/rootfs.cpio.xz                    \
    -append "earlyprintk=ttyS0 console=ttyS0 debug"                                      \
    -device virtio-net-pci,netdev=unet,mac=52:54:00:f1:26:a6                             \
    -netdev user,id=unet,hostfwd=tcp::50959-:22                                          \
    -drive file=~/sandbox/images/vm.img,if=none,format=raw,id=nvme0                      \
    -device nvme,drive=nvme0,serial=foo                                                  \
    -drive file=~/sandbox/images/vm1.img,if=none,format=raw,id=nvme1                     \
    -device nvme,drive=nvme1,serial=foo1

>> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
>> the parent object of NMI state instead of itself. With this, he parent object
>> is passed to the function, to avoid potential data corruption.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/core/nmi.c              | 8 ++++----
>>   hw/i386/x86.c              | 2 +-
>>   hw/misc/macio/gpio.c       | 6 +++---
>>   hw/ppc/spapr.c             | 2 +-
>>   hw/s390x/s390-virtio-ccw.c | 2 +-
>>   include/hw/nmi.h           | 2 +-
>>   6 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
>> index 481c4b3c7e..554708d0db 100644
>> --- a/hw/core/nmi.c
>> +++ b/hw/core/nmi.c
>> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
>>   static int do_nmi(Object *o, void *opaque)
>>   {
>>       struct do_nmi_s *ns = opaque;
>> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
>> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
>>   
>> -    if (n) {
>> -        NMIClass *nc = NMI_GET_CLASS(n);
>> +    if (parent) {
>> +        NMIClass *nc = NMI_GET_CLASS(parent);
>>   
>>           ns->handled = true;
>> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
>> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
>>           if (ns->err) {
>>               return -1;
>>           }
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index 394edc2f72..b98204f104 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>>       return ms->possible_cpus;
>>   }
>>   
>> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
>>   {
>>       /* cpu index isn't used */
>>       CPUState *cs;
>> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
>> index 6cca6b27d6..6b4dfcc188 100644
>> --- a/hw/misc/macio/gpio.c
>> +++ b/hw/misc/macio/gpio.c
>> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
>>       macio_set_gpio(s, 1, true);
>>   }
>>   
>> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
>> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
>>   {
>> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
>> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
>> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
>> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
>>   }
>>   
>>   static void macio_gpio_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e076f6023c..3b92e4013d 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>       ppc_cpu_do_system_reset(cs);
>>   }
>>   
>> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
>>   {
>>       CPUState *cs;
>>   
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index d3edeef0ad..a49952a8b9 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
>>       s390x_new_cpu(object_class_get_name(oc), id, errp);
>>   }
>>   
>> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
>> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
>>   {
>>       CPUState *cs = qemu_get_cpu(cpu_index);
>>   
>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>> index a1e128724e..75afa67790 100644
>> --- a/include/hw/nmi.h
>> +++ b/include/hw/nmi.h
>> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
>>   typedef struct NMIClass {
>>       InterfaceClass parent_class;
>>   
>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>>   } NMIClass;
>>   
>>   void nmi_monitor_handle(int cpu_index, Error **errp);
> 

Regards,
Gavin
Greg Kurz Dec. 9, 2019, 8:36 a.m. UTC | #3
On Sun, 8 Dec 2019 00:14:00 +1100
Gavin Shan <gshan@redhat.com> wrote:

> On 12/7/19 3:50 AM, Greg Kurz wrote:
> > On Fri,  6 Dec 2019 17:36:42 +1100
> > Gavin Shan <gshan@redhat.com> wrote:
> > 
> >> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
> >> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
> >>
> > 
> > object_dynamic_cast() returns either its first argument if it can be cast
> > to the given typename or NULL. Since the x86, ccw and spapr machines and
> > the PowerMac GPIO implement the NMI interface, nothing is wrong here.
> > 
> > The do_nmi() function is called for all objects in the QOM tree and simply
> > filters out the ones that don't implement the NMI interface.
> > 
> > I do agree that this isn't super obvious though. It would be clearer to
> > do the filtering in the foreach() function. Something like:
> > 
> > https://patchwork.ozlabs.org/patch/1182232/
> > 
> 
> Greg, Thanks for the review and comments. I think we're talking about
> different issues. I do agree it's worthy to simplify the code with the
> object_child_foreach_type(), but later.
> 
> When we have below parameters to qemu, object_dynamic_cast(o, TYPE_NMI)
> returns object of "pc-q35-4.2-machine", which is passed to the subquent
> NMIClass::nmi_monitor_handler(). However, the function expects a NMIState
> from its prototype as below. This patch changes the prototype to fill the
> gap.
> 
>     void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> 

Hmm... the "pc-q35-4.2-machine" type inherits from the TYPE_PC_MACHINE,
which in turns inherits from TYPE_X86_MACHINE, which implements the
TYPE_NMI interface. It thus seems okay to pass this object to x86_nmi().

What are you trying to fix here ?

>     Changed to:
> 
>     void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
> 
>     ~/sandbox/src/qemu/qemu.main.x86/x86_64-softmmu/qemu-system-x86_64              \
>     --enable-kvm -machine type=q35,accel=kvm,kernel-irqchip=on -cpu host -smp 2 -m 1G    \
>     -monitor none -serial mon:stdio -nographic -s                                        \
>     -bios ~/sandbox/src/firmware/seabios/out/bios.bin                                    \
>     -kernel ~/sandbox/src/linux/linux.main.x86/arch/x86/boot/bzImage                     \
>     -initrd ~/sandbox/src/util/buildroot/output/images/rootfs.cpio.xz                    \
>     -append "earlyprintk=ttyS0 console=ttyS0 debug"                                      \
>     -device virtio-net-pci,netdev=unet,mac=52:54:00:f1:26:a6                             \
>     -netdev user,id=unet,hostfwd=tcp::50959-:22                                          \
>     -drive file=~/sandbox/images/vm.img,if=none,format=raw,id=nvme0                      \
>     -device nvme,drive=nvme0,serial=foo                                                  \
>     -drive file=~/sandbox/images/vm1.img,if=none,format=raw,id=nvme1                     \
>     -device nvme,drive=nvme1,serial=foo1
> 
> >> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
> >> the parent object of NMI state instead of itself. With this, he parent object
> >> is passed to the function, to avoid potential data corruption.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   hw/core/nmi.c              | 8 ++++----
> >>   hw/i386/x86.c              | 2 +-
> >>   hw/misc/macio/gpio.c       | 6 +++---
> >>   hw/ppc/spapr.c             | 2 +-
> >>   hw/s390x/s390-virtio-ccw.c | 2 +-
> >>   include/hw/nmi.h           | 2 +-
> >>   6 files changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> >> index 481c4b3c7e..554708d0db 100644
> >> --- a/hw/core/nmi.c
> >> +++ b/hw/core/nmi.c
> >> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
> >>   static int do_nmi(Object *o, void *opaque)
> >>   {
> >>       struct do_nmi_s *ns = opaque;
> >> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
> >> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
> >>   
> >> -    if (n) {
> >> -        NMIClass *nc = NMI_GET_CLASS(n);
> >> +    if (parent) {
> >> +        NMIClass *nc = NMI_GET_CLASS(parent);
> >>   
> >>           ns->handled = true;
> >> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
> >> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
> >>           if (ns->err) {
> >>               return -1;
> >>           }
> >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >> index 394edc2f72..b98204f104 100644
> >> --- a/hw/i386/x86.c
> >> +++ b/hw/i386/x86.c
> >> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
> >>       return ms->possible_cpus;
> >>   }
> >>   
> >> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
> >> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
> >>   {
> >>       /* cpu index isn't used */
> >>       CPUState *cs;
> >> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
> >> index 6cca6b27d6..6b4dfcc188 100644
> >> --- a/hw/misc/macio/gpio.c
> >> +++ b/hw/misc/macio/gpio.c
> >> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
> >>       macio_set_gpio(s, 1, true);
> >>   }
> >>   
> >> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
> >> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
> >>   {
> >> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
> >> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
> >> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
> >> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
> >>   }
> >>   
> >>   static void macio_gpio_class_init(ObjectClass *oc, void *data)
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index e076f6023c..3b92e4013d 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
> >>       ppc_cpu_do_system_reset(cs);
> >>   }
> >>   
> >> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> >> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
> >>   {
> >>       CPUState *cs;
> >>   
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index d3edeef0ad..a49952a8b9 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
> >>       s390x_new_cpu(object_class_get_name(oc), id, errp);
> >>   }
> >>   
> >> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
> >> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
> >>   {
> >>       CPUState *cs = qemu_get_cpu(cpu_index);
> >>   
> >> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> >> index a1e128724e..75afa67790 100644
> >> --- a/include/hw/nmi.h
> >> +++ b/include/hw/nmi.h
> >> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
> >>   typedef struct NMIClass {
> >>       InterfaceClass parent_class;
> >>   
> >> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> >> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
> >>   } NMIClass;
> >>   
> >>   void nmi_monitor_handle(int cpu_index, Error **errp);
> > 
> 
> Regards,
> Gavin
>
Gavin Shan Dec. 9, 2019, 9:12 a.m. UTC | #4
On 12/9/19 7:36 PM, Greg Kurz wrote:
> On Sun, 8 Dec 2019 00:14:00 +1100
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> On 12/7/19 3:50 AM, Greg Kurz wrote:
>>> On Fri,  6 Dec 2019 17:36:42 +1100
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
>>>> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
>>>>
>>>
>>> object_dynamic_cast() returns either its first argument if it can be cast
>>> to the given typename or NULL. Since the x86, ccw and spapr machines and
>>> the PowerMac GPIO implement the NMI interface, nothing is wrong here.
>>>
>>> The do_nmi() function is called for all objects in the QOM tree and simply
>>> filters out the ones that don't implement the NMI interface.
>>>
>>> I do agree that this isn't super obvious though. It would be clearer to
>>> do the filtering in the foreach() function. Something like:
>>>
>>> https://patchwork.ozlabs.org/patch/1182232/
>>>
>>
>> Greg, Thanks for the review and comments. I think we're talking about
>> different issues. I do agree it's worthy to simplify the code with the
>> object_child_foreach_type(), but later.
>>
>> When we have below parameters to qemu, object_dynamic_cast(o, TYPE_NMI)
>> returns object of "pc-q35-4.2-machine", which is passed to the subquent
>> NMIClass::nmi_monitor_handler(). However, the function expects a NMIState
>> from its prototype as below. This patch changes the prototype to fill the
>> gap.
>>
>>      void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>
> 
> Hmm... the "pc-q35-4.2-machine" type inherits from the TYPE_PC_MACHINE,
> which in turns inherits from TYPE_X86_MACHINE, which implements the
> TYPE_NMI interface. It thus seems okay to pass this object to x86_nmi().
> 
> What are you trying to fix here ?
> 

There is no valid instance associated with interface class. For this specific
case, there is no NMIState instance. I don't understand how we can pass a
valid NMIState to the function. Furthermore, below code returns "pc-q35-4.2-machine"
object instead of NMIState. It's not safe to do the forced conversion here.

    /* It returns "pc-q35-4.2-machine" instance */
    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
      :
    nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);


If "struct NMIState" is declared and the data struct is written in the function.
We will run into data corruption if I'm correct.

I'm pasting the message I had. Hope this helps for understanding the issue:

--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -42,6 +42,10 @@ static int do_nmi(Object *o, void *opaque)
      if (n) {
          NMIClass *nc = NMI_GET_CLASS(n);
  
+       fprintf(stdout, "%s: o=%s, n=%s\n",
+                __func__, object_get_typename(OBJECT(o)),
+                object_get_typename(OBJECT(n)));
+


# QEMU 4.1.93 monitor - type 'help' for more information
(qemu) nmi
do_nmi: o=pc-q35-4.2-machine, n=pc-q35-4.2-machine


>>      Changed to:
>>
>>      void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>>
>>      ~/sandbox/src/qemu/qemu.main.x86/x86_64-softmmu/qemu-system-x86_64              \
>>      --enable-kvm -machine type=q35,accel=kvm,kernel-irqchip=on -cpu host -smp 2 -m 1G    \
>>      -monitor none -serial mon:stdio -nographic -s                                        \
>>      -bios ~/sandbox/src/firmware/seabios/out/bios.bin                                    \
>>      -kernel ~/sandbox/src/linux/linux.main.x86/arch/x86/boot/bzImage                     \
>>      -initrd ~/sandbox/src/util/buildroot/output/images/rootfs.cpio.xz                    \
>>      -append "earlyprintk=ttyS0 console=ttyS0 debug"                                      \
>>      -device virtio-net-pci,netdev=unet,mac=52:54:00:f1:26:a6                             \
>>      -netdev user,id=unet,hostfwd=tcp::50959-:22                                          \
>>      -drive file=~/sandbox/images/vm.img,if=none,format=raw,id=nvme0                      \
>>      -device nvme,drive=nvme0,serial=foo                                                  \
>>      -drive file=~/sandbox/images/vm1.img,if=none,format=raw,id=nvme1                     \
>>      -device nvme,drive=nvme1,serial=foo1
>>
>>>> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
>>>> the parent object of NMI state instead of itself. With this, he parent object
>>>> is passed to the function, to avoid potential data corruption.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>    hw/core/nmi.c              | 8 ++++----
>>>>    hw/i386/x86.c              | 2 +-
>>>>    hw/misc/macio/gpio.c       | 6 +++---
>>>>    hw/ppc/spapr.c             | 2 +-
>>>>    hw/s390x/s390-virtio-ccw.c | 2 +-
>>>>    include/hw/nmi.h           | 2 +-
>>>>    6 files changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
>>>> index 481c4b3c7e..554708d0db 100644
>>>> --- a/hw/core/nmi.c
>>>> +++ b/hw/core/nmi.c
>>>> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
>>>>    static int do_nmi(Object *o, void *opaque)
>>>>    {
>>>>        struct do_nmi_s *ns = opaque;
>>>> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
>>>> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
>>>>    
>>>> -    if (n) {
>>>> -        NMIClass *nc = NMI_GET_CLASS(n);
>>>> +    if (parent) {
>>>> +        NMIClass *nc = NMI_GET_CLASS(parent);
>>>>    
>>>>            ns->handled = true;
>>>> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
>>>> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
>>>>            if (ns->err) {
>>>>                return -1;
>>>>            }
>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>> index 394edc2f72..b98204f104 100644
>>>> --- a/hw/i386/x86.c
>>>> +++ b/hw/i386/x86.c
>>>> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>>>>        return ms->possible_cpus;
>>>>    }
>>>>    
>>>> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>>> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
>>>>    {
>>>>        /* cpu index isn't used */
>>>>        CPUState *cs;
>>>> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
>>>> index 6cca6b27d6..6b4dfcc188 100644
>>>> --- a/hw/misc/macio/gpio.c
>>>> +++ b/hw/misc/macio/gpio.c
>>>> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
>>>>        macio_set_gpio(s, 1, true);
>>>>    }
>>>>    
>>>> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
>>>> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
>>>>    {
>>>> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
>>>> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
>>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
>>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
>>>>    }
>>>>    
>>>>    static void macio_gpio_class_init(ObjectClass *oc, void *data)
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index e076f6023c..3b92e4013d 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>>>        ppc_cpu_do_system_reset(cs);
>>>>    }
>>>>    
>>>> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>>>> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
>>>>    {
>>>>        CPUState *cs;
>>>>    
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index d3edeef0ad..a49952a8b9 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
>>>>        s390x_new_cpu(object_class_get_name(oc), id, errp);
>>>>    }
>>>>    
>>>> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
>>>> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
>>>>    {
>>>>        CPUState *cs = qemu_get_cpu(cpu_index);
>>>>    
>>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>>>> index a1e128724e..75afa67790 100644
>>>> --- a/include/hw/nmi.h
>>>> +++ b/include/hw/nmi.h
>>>> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
>>>>    typedef struct NMIClass {
>>>>        InterfaceClass parent_class;
>>>>    
>>>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>>> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>>>>    } NMIClass;
>>>>    
>>>>    void nmi_monitor_handle(int cpu_index, Error **errp);
>>>

Regards,
Gavin
Greg Kurz Dec. 9, 2019, 10:55 a.m. UTC | #5
On Mon, 9 Dec 2019 20:12:09 +1100
Gavin Shan <gshan@redhat.com> wrote:

> On 12/9/19 7:36 PM, Greg Kurz wrote:
> > On Sun, 8 Dec 2019 00:14:00 +1100
> > Gavin Shan <gshan@redhat.com> wrote:
> > 
> >> On 12/7/19 3:50 AM, Greg Kurz wrote:
> >>> On Fri,  6 Dec 2019 17:36:42 +1100
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>
> >>>> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
> >>>> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
> >>>>
> >>>
> >>> object_dynamic_cast() returns either its first argument if it can be cast
> >>> to the given typename or NULL. Since the x86, ccw and spapr machines and
> >>> the PowerMac GPIO implement the NMI interface, nothing is wrong here.
> >>>
> >>> The do_nmi() function is called for all objects in the QOM tree and simply
> >>> filters out the ones that don't implement the NMI interface.
> >>>
> >>> I do agree that this isn't super obvious though. It would be clearer to
> >>> do the filtering in the foreach() function. Something like:
> >>>
> >>> https://patchwork.ozlabs.org/patch/1182232/
> >>>
> >>
> >> Greg, Thanks for the review and comments. I think we're talking about
> >> different issues. I do agree it's worthy to simplify the code with the
> >> object_child_foreach_type(), but later.
> >>
> >> When we have below parameters to qemu, object_dynamic_cast(o, TYPE_NMI)
> >> returns object of "pc-q35-4.2-machine", which is passed to the subquent
> >> NMIClass::nmi_monitor_handler(). However, the function expects a NMIState
> >> from its prototype as below. This patch changes the prototype to fill the
> >> gap.
> >>
> >>      void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> >>
> > 
> > Hmm... the "pc-q35-4.2-machine" type inherits from the TYPE_PC_MACHINE,
> > which in turns inherits from TYPE_X86_MACHINE, which implements the
> > TYPE_NMI interface. It thus seems okay to pass this object to x86_nmi().
> > 
> > What are you trying to fix here ?
> > 
> 
> There is no valid instance associated with interface class. For this specific
> case, there is no NMIState instance. I don't understand how we can pass a
> valid NMIState to the function.

Ah I get it. Please read the include/qom/object.h to understand what QOM
interfaces are about. Especially this paragraph:

 * # Interfaces #
 *
 * Interfaces allow a limited form of multiple inheritance.  Instances are
 * similar to normal types except for the fact that are only defined by
 * their classes and never carry any state.  You can dynamically cast an object
 * to one of its #Interface types and vice versa.

> Furthermore, below code returns "pc-q35-4.2-machine"
> object instead of NMIState. It's not safe to do the forced conversion here.
> 
>     /* It returns "pc-q35-4.2-machine" instance */
>     NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
>       :
>     nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);

As explained in my previous mail, this _forced conversion_ is the current
way to implement object_child_foreach_type(). It is ugly and confusing for
newcomers but it is safe since the pointer is either TYPE_NMI or NULL, in
which case the handler isn't called.

> 
> 
> If "struct NMIState" is declared and the data struct is written in the function.
> We will run into data corruption if I'm correct.
> 

The 'struct NMIState' isn't defined on purpose, but I agree this isn't documented
anywhere except in this changelog:

commit 00ed3da9b5c2e66e796a172df3e19545462b9c90
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Tue Sep 24 16:00:33 2019 +1000

    xics: Minor fixes for XICSFabric interface
    
    Interface instances should never be directly dereferenced.  So, the common
    practice is to make them incomplete types to make sure no-one does that.
    XICSFrabric, however, had a dummy type which is less safe.
    
    We were also using OBJECT_CHECK() where we should have been using
    INTERFACE_CHECK().
    
    Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
    Reviewed-by: Greg Kurz <groug@kaod.org>
    Reviewed-by: Cédric Le Goater <clg@kaod.org>

Maybe the NMIState naming was a poor choice since there's no associated
state with QOM interfaces. But we do want to keep a typedef though: it
is more precise and hence safer to pass an NMIState * than an Object *.

> I'm pasting the message I had. Hope this helps for understanding the issue:
> 
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -42,6 +42,10 @@ static int do_nmi(Object *o, void *opaque)
>       if (n) {
>           NMIClass *nc = NMI_GET_CLASS(n);
>   
> +       fprintf(stdout, "%s: o=%s, n=%s\n",
> +                __func__, object_get_typename(OBJECT(o)),
> +                object_get_typename(OBJECT(n)));
> +
> 

object_get_typename() returns the type of the instance, which is either
a machine or a GPIO in the current code base. This is different from
object_dynamic_cast() which just tells if a given object can be cast
to a give type (either regular QOM type or QOM interface).

It is up to the various NMIClass::nmi_monitor_handler() to make sure
they're being passed the right object.

macio_gpio_nmi() explicitly does that with MACIO_GPIO(n). The other
ones only need to access the CPU state and end up checking it is
valid: s390_nmi() checks S390_CPU(cs), x86_nmi() checks X86_CPU(cs)
and

spapr_nmi()
 ->async_run_on_cpu()
   ->spapr_do_system_reset_on_cpu()
     ->ppc_cpu_do_system_reset() checks POWERPC_CPU(cs)

but they could maybe assert(object_dynamic_cast()) as well for
clarity.

> 
> # QEMU 4.1.93 monitor - type 'help' for more information
> (qemu) nmi
> do_nmi: o=pc-q35-4.2-machine, n=pc-q35-4.2-machine
> 
> 
> >>      Changed to:
> >>
> >>      void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
> >>
> >>      ~/sandbox/src/qemu/qemu.main.x86/x86_64-softmmu/qemu-system-x86_64              \
> >>      --enable-kvm -machine type=q35,accel=kvm,kernel-irqchip=on -cpu host -smp 2 -m 1G    \
> >>      -monitor none -serial mon:stdio -nographic -s                                        \
> >>      -bios ~/sandbox/src/firmware/seabios/out/bios.bin                                    \
> >>      -kernel ~/sandbox/src/linux/linux.main.x86/arch/x86/boot/bzImage                     \
> >>      -initrd ~/sandbox/src/util/buildroot/output/images/rootfs.cpio.xz                    \
> >>      -append "earlyprintk=ttyS0 console=ttyS0 debug"                                      \
> >>      -device virtio-net-pci,netdev=unet,mac=52:54:00:f1:26:a6                             \
> >>      -netdev user,id=unet,hostfwd=tcp::50959-:22                                          \
> >>      -drive file=~/sandbox/images/vm.img,if=none,format=raw,id=nvme0                      \
> >>      -device nvme,drive=nvme0,serial=foo                                                  \
> >>      -drive file=~/sandbox/images/vm1.img,if=none,format=raw,id=nvme1                     \
> >>      -device nvme,drive=nvme1,serial=foo1
> >>
> >>>> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
> >>>> the parent object of NMI state instead of itself. With this, he parent object
> >>>> is passed to the function, to avoid potential data corruption.
> >>>>
> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >>>> ---
> >>>>    hw/core/nmi.c              | 8 ++++----
> >>>>    hw/i386/x86.c              | 2 +-
> >>>>    hw/misc/macio/gpio.c       | 6 +++---
> >>>>    hw/ppc/spapr.c             | 2 +-
> >>>>    hw/s390x/s390-virtio-ccw.c | 2 +-
> >>>>    include/hw/nmi.h           | 2 +-
> >>>>    6 files changed, 11 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> >>>> index 481c4b3c7e..554708d0db 100644
> >>>> --- a/hw/core/nmi.c
> >>>> +++ b/hw/core/nmi.c
> >>>> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
> >>>>    static int do_nmi(Object *o, void *opaque)
> >>>>    {
> >>>>        struct do_nmi_s *ns = opaque;
> >>>> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
> >>>> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
> >>>>    
> >>>> -    if (n) {
> >>>> -        NMIClass *nc = NMI_GET_CLASS(n);
> >>>> +    if (parent) {
> >>>> +        NMIClass *nc = NMI_GET_CLASS(parent);
> >>>>    
> >>>>            ns->handled = true;
> >>>> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
> >>>> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
> >>>>            if (ns->err) {
> >>>>                return -1;
> >>>>            }
> >>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >>>> index 394edc2f72..b98204f104 100644
> >>>> --- a/hw/i386/x86.c
> >>>> +++ b/hw/i386/x86.c
> >>>> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
> >>>>        return ms->possible_cpus;
> >>>>    }
> >>>>    
> >>>> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
> >>>> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
> >>>>    {
> >>>>        /* cpu index isn't used */
> >>>>        CPUState *cs;
> >>>> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
> >>>> index 6cca6b27d6..6b4dfcc188 100644
> >>>> --- a/hw/misc/macio/gpio.c
> >>>> +++ b/hw/misc/macio/gpio.c
> >>>> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
> >>>>        macio_set_gpio(s, 1, true);
> >>>>    }
> >>>>    
> >>>> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
> >>>> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
> >>>>    {
> >>>> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
> >>>> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
> >>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
> >>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
> >>>>    }
> >>>>    
> >>>>    static void macio_gpio_class_init(ObjectClass *oc, void *data)
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index e076f6023c..3b92e4013d 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
> >>>>        ppc_cpu_do_system_reset(cs);
> >>>>    }
> >>>>    
> >>>> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> >>>> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
> >>>>    {
> >>>>        CPUState *cs;
> >>>>    
> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>>> index d3edeef0ad..a49952a8b9 100644
> >>>> --- a/hw/s390x/s390-virtio-ccw.c
> >>>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>>> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
> >>>>        s390x_new_cpu(object_class_get_name(oc), id, errp);
> >>>>    }
> >>>>    
> >>>> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
> >>>> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
> >>>>    {
> >>>>        CPUState *cs = qemu_get_cpu(cpu_index);
> >>>>    
> >>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
> >>>> index a1e128724e..75afa67790 100644
> >>>> --- a/include/hw/nmi.h
> >>>> +++ b/include/hw/nmi.h
> >>>> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
> >>>>    typedef struct NMIClass {
> >>>>        InterfaceClass parent_class;
> >>>>    
> >>>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
> >>>> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
> >>>>    } NMIClass;
> >>>>    
> >>>>    void nmi_monitor_handle(int cpu_index, Error **errp);
> >>>
> 
> Regards,
> Gavin
>
Gavin Shan Dec. 9, 2019, 11:19 p.m. UTC | #6
On 12/9/19 9:55 PM, Greg Kurz wrote:
> On Mon, 9 Dec 2019 20:12:09 +1100
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> On 12/9/19 7:36 PM, Greg Kurz wrote:
>>> On Sun, 8 Dec 2019 00:14:00 +1100
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> On 12/7/19 3:50 AM, Greg Kurz wrote:
>>>>> On Fri,  6 Dec 2019 17:36:42 +1100
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>>
>>>>>> object_dynamic_cast() should return machine (or GPIO) state instad of NMI
>>>>>> state in do_nmi(). So it's wrong to convert it to NMI state unconditionally.
>>>>>>
>>>>>
>>>>> object_dynamic_cast() returns either its first argument if it can be cast
>>>>> to the given typename or NULL. Since the x86, ccw and spapr machines and
>>>>> the PowerMac GPIO implement the NMI interface, nothing is wrong here.
>>>>>
>>>>> The do_nmi() function is called for all objects in the QOM tree and simply
>>>>> filters out the ones that don't implement the NMI interface.
>>>>>
>>>>> I do agree that this isn't super obvious though. It would be clearer to
>>>>> do the filtering in the foreach() function. Something like:
>>>>>
>>>>> https://patchwork.ozlabs.org/patch/1182232/
>>>>>
>>>>
>>>> Greg, Thanks for the review and comments. I think we're talking about
>>>> different issues. I do agree it's worthy to simplify the code with the
>>>> object_child_foreach_type(), but later.
>>>>
>>>> When we have below parameters to qemu, object_dynamic_cast(o, TYPE_NMI)
>>>> returns object of "pc-q35-4.2-machine", which is passed to the subquent
>>>> NMIClass::nmi_monitor_handler(). However, the function expects a NMIState
>>>> from its prototype as below. This patch changes the prototype to fill the
>>>> gap.
>>>>
>>>>       void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>>>
>>>
>>> Hmm... the "pc-q35-4.2-machine" type inherits from the TYPE_PC_MACHINE,
>>> which in turns inherits from TYPE_X86_MACHINE, which implements the
>>> TYPE_NMI interface. It thus seems okay to pass this object to x86_nmi().
>>>
>>> What are you trying to fix here ?
>>>
>>
>> There is no valid instance associated with interface class. For this specific
>> case, there is no NMIState instance. I don't understand how we can pass a
>> valid NMIState to the function.
> 
> Ah I get it. Please read the include/qom/object.h to understand what QOM
> interfaces are about. Especially this paragraph:
> 
>   * # Interfaces #
>   *
>   * Interfaces allow a limited form of multiple inheritance.  Instances are
>   * similar to normal types except for the fact that are only defined by
>   * their classes and never carry any state.  You can dynamically cast an object
>   * to one of its #Interface types and vice versa.
> 

Thanks, Greg. The statements here clarify the forced conversion (casting) is
allowed, even I don't think it's safe to do so.

>> Furthermore, below code returns "pc-q35-4.2-machine"
>> object instead of NMIState. It's not safe to do the forced conversion here.
>>
>>      /* It returns "pc-q35-4.2-machine" instance */
>>      NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
>>        :
>>      nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
> 
> As explained in my previous mail, this _forced conversion_ is the current
> way to implement object_child_foreach_type(). It is ugly and confusing for
> newcomers but it is safe since the pointer is either TYPE_NMI or NULL, in
> which case the handler isn't called.
> 

Yes, as above statements said.

>>
>>
>> If "struct NMIState" is declared and the data struct is written in the function.
>> We will run into data corruption if I'm correct.
>>
> 
> The 'struct NMIState' isn't defined on purpose, but I agree this isn't documented
> anywhere except in this changelog:
> 
> commit 00ed3da9b5c2e66e796a172df3e19545462b9c90
> Author: David Gibson <david@gibson.dropbear.id.au>
> Date:   Tue Sep 24 16:00:33 2019 +1000
> 
>      xics: Minor fixes for XICSFabric interface
>      
>      Interface instances should never be directly dereferenced.  So, the common
>      practice is to make them incomplete types to make sure no-one does that.
>      XICSFrabric, however, had a dummy type which is less safe.
>      
>      We were also using OBJECT_CHECK() where we should have been using
>      INTERFACE_CHECK().
>      
>      Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>      Reviewed-by: Greg Kurz <groug@kaod.org>
>      Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Maybe the NMIState naming was a poor choice since there's no associated
> state with QOM interfaces. But we do want to keep a typedef though: it
> is more precise and hence safer to pass an NMIState * than an Object *.> 

It's arguable to be correct enough. The NMIClass and the associated callback
(nmi_monitor_handler()) are attached to the parent class, which is "pc-q35-4.2-machine"
in our example. The interface is strongly associated with the parent class. From
this perspective, it also makes sense to pass the parent object.

However, we didn't run into any issue so far. Maybe the code can be revisited
when it needs to be improved later, or never :)

>> I'm pasting the message I had. Hope this helps for understanding the issue:
>>
>> --- a/hw/core/nmi.c
>> +++ b/hw/core/nmi.c
>> @@ -42,6 +42,10 @@ static int do_nmi(Object *o, void *opaque)
>>        if (n) {
>>            NMIClass *nc = NMI_GET_CLASS(n);
>>    
>> +       fprintf(stdout, "%s: o=%s, n=%s\n",
>> +                __func__, object_get_typename(OBJECT(o)),
>> +                object_get_typename(OBJECT(n)));
>> +
>>
> 
> object_get_typename() returns the type of the instance, which is either
> a machine or a GPIO in the current code base. This is different from
> object_dynamic_cast() which just tells if a given object can be cast
> to a give type (either regular QOM type or QOM interface).
> 
> It is up to the various NMIClass::nmi_monitor_handler() to make sure
> they're being passed the right object.
> 
> macio_gpio_nmi() explicitly does that with MACIO_GPIO(n). The other
> ones only need to access the CPU state and end up checking it is
> valid: s390_nmi() checks S390_CPU(cs), x86_nmi() checks X86_CPU(cs)
> and
> 
> spapr_nmi()
>   ->async_run_on_cpu()
>     ->spapr_do_system_reset_on_cpu()
>       ->ppc_cpu_do_system_reset() checks POWERPC_CPU(cs)
> 
> but they could maybe assert(object_dynamic_cast()) as well for
> clarity.
> 

Yeah, nothing is wrong currently. macio_gpio_nmi() is the only case
where the NMIState is consumed after correct casting is applied.

>>
>> # QEMU 4.1.93 monitor - type 'help' for more information
>> (qemu) nmi
>> do_nmi: o=pc-q35-4.2-machine, n=pc-q35-4.2-machine
>>
>>
>>>>       Changed to:
>>>>
>>>>       void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>>>>
>>>>       ~/sandbox/src/qemu/qemu.main.x86/x86_64-softmmu/qemu-system-x86_64              \
>>>>       --enable-kvm -machine type=q35,accel=kvm,kernel-irqchip=on -cpu host -smp 2 -m 1G    \
>>>>       -monitor none -serial mon:stdio -nographic -s                                        \
>>>>       -bios ~/sandbox/src/firmware/seabios/out/bios.bin                                    \
>>>>       -kernel ~/sandbox/src/linux/linux.main.x86/arch/x86/boot/bzImage                     \
>>>>       -initrd ~/sandbox/src/util/buildroot/output/images/rootfs.cpio.xz                    \
>>>>       -append "earlyprintk=ttyS0 console=ttyS0 debug"                                      \
>>>>       -device virtio-net-pci,netdev=unet,mac=52:54:00:f1:26:a6                             \
>>>>       -netdev user,id=unet,hostfwd=tcp::50959-:22                                          \
>>>>       -drive file=~/sandbox/images/vm.img,if=none,format=raw,id=nvme0                      \
>>>>       -device nvme,drive=nvme0,serial=foo                                                  \
>>>>       -drive file=~/sandbox/images/vm1.img,if=none,format=raw,id=nvme1                     \
>>>>       -device nvme,drive=nvme1,serial=foo1
>>>>
>>>>>> This changes the prototype of NMIClass::nmi_monitor_handler() to accept
>>>>>> the parent object of NMI state instead of itself. With this, he parent object
>>>>>> is passed to the function, to avoid potential data corruption.
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>> ---
>>>>>>     hw/core/nmi.c              | 8 ++++----
>>>>>>     hw/i386/x86.c              | 2 +-
>>>>>>     hw/misc/macio/gpio.c       | 6 +++---
>>>>>>     hw/ppc/spapr.c             | 2 +-
>>>>>>     hw/s390x/s390-virtio-ccw.c | 2 +-
>>>>>>     include/hw/nmi.h           | 2 +-
>>>>>>     6 files changed, 11 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
>>>>>> index 481c4b3c7e..554708d0db 100644
>>>>>> --- a/hw/core/nmi.c
>>>>>> +++ b/hw/core/nmi.c
>>>>>> @@ -37,13 +37,13 @@ static void nmi_children(Object *o, struct do_nmi_s *ns);
>>>>>>     static int do_nmi(Object *o, void *opaque)
>>>>>>     {
>>>>>>         struct do_nmi_s *ns = opaque;
>>>>>> -    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
>>>>>> +    Object *parent = object_dynamic_cast(o, TYPE_NMI);
>>>>>>     
>>>>>> -    if (n) {
>>>>>> -        NMIClass *nc = NMI_GET_CLASS(n);
>>>>>> +    if (parent) {
>>>>>> +        NMIClass *nc = NMI_GET_CLASS(parent);
>>>>>>     
>>>>>>             ns->handled = true;
>>>>>> -        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
>>>>>> +        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
>>>>>>             if (ns->err) {
>>>>>>                 return -1;
>>>>>>             }
>>>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>>>> index 394edc2f72..b98204f104 100644
>>>>>> --- a/hw/i386/x86.c
>>>>>> +++ b/hw/i386/x86.c
>>>>>> @@ -190,7 +190,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>>>>>>         return ms->possible_cpus;
>>>>>>     }
>>>>>>     
>>>>>> -static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>>>>> +static void x86_nmi(Object *parent, int cpu_index, Error **errp)
>>>>>>     {
>>>>>>         /* cpu index isn't used */
>>>>>>         CPUState *cs;
>>>>>> diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
>>>>>> index 6cca6b27d6..6b4dfcc188 100644
>>>>>> --- a/hw/misc/macio/gpio.c
>>>>>> +++ b/hw/misc/macio/gpio.c
>>>>>> @@ -196,10 +196,10 @@ static void macio_gpio_reset(DeviceState *dev)
>>>>>>         macio_set_gpio(s, 1, true);
>>>>>>     }
>>>>>>     
>>>>>> -static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
>>>>>> +static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
>>>>>>     {
>>>>>> -    macio_set_gpio(MACIO_GPIO(n), 9, true);
>>>>>> -    macio_set_gpio(MACIO_GPIO(n), 9, false);
>>>>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, true);
>>>>>> +    macio_set_gpio(MACIO_GPIO(parent), 9, false);
>>>>>>     }
>>>>>>     
>>>>>>     static void macio_gpio_class_init(ObjectClass *oc, void *data)
>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>> index e076f6023c..3b92e4013d 100644
>>>>>> --- a/hw/ppc/spapr.c
>>>>>> +++ b/hw/ppc/spapr.c
>>>>>> @@ -3377,7 +3377,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
>>>>>>         ppc_cpu_do_system_reset(cs);
>>>>>>     }
>>>>>>     
>>>>>> -static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>>>>>> +static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
>>>>>>     {
>>>>>>         CPUState *cs;
>>>>>>     
>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>> index d3edeef0ad..a49952a8b9 100644
>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>> @@ -431,7 +431,7 @@ static void s390_hot_add_cpu(MachineState *machine,
>>>>>>         s390x_new_cpu(object_class_get_name(oc), id, errp);
>>>>>>     }
>>>>>>     
>>>>>> -static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
>>>>>> +static void s390_nmi(Object *parent, int cpu_index, Error **errp)
>>>>>>     {
>>>>>>         CPUState *cs = qemu_get_cpu(cpu_index);
>>>>>>     
>>>>>> diff --git a/include/hw/nmi.h b/include/hw/nmi.h
>>>>>> index a1e128724e..75afa67790 100644
>>>>>> --- a/include/hw/nmi.h
>>>>>> +++ b/include/hw/nmi.h
>>>>>> @@ -38,7 +38,7 @@ typedef struct NMIState NMIState;
>>>>>>     typedef struct NMIClass {
>>>>>>         InterfaceClass parent_class;
>>>>>>     
>>>>>> -    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
>>>>>> +    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
>>>>>>     } NMIClass;
>>>>>>     
>>>>>>     void nmi_monitor_handle(int cpu_index, Error **errp);
>>>>>


Regards,
Gavin
diff mbox series

Patch

diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 481c4b3c7e..554708d0db 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -37,13 +37,13 @@  static void nmi_children(Object *o, struct do_nmi_s *ns);
 static int do_nmi(Object *o, void *opaque)
 {
     struct do_nmi_s *ns = opaque;
-    NMIState *n = (NMIState *) object_dynamic_cast(o, TYPE_NMI);
+    Object *parent = object_dynamic_cast(o, TYPE_NMI);
 
-    if (n) {
-        NMIClass *nc = NMI_GET_CLASS(n);
+    if (parent) {
+        NMIClass *nc = NMI_GET_CLASS(parent);
 
         ns->handled = true;
-        nc->nmi_monitor_handler(n, ns->cpu_index, &ns->err);
+        nc->nmi_monitor_handler(parent, ns->cpu_index, &ns->err);
         if (ns->err) {
             return -1;
         }
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 394edc2f72..b98204f104 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -190,7 +190,7 @@  const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
-static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
+static void x86_nmi(Object *parent, int cpu_index, Error **errp)
 {
     /* cpu index isn't used */
     CPUState *cs;
diff --git a/hw/misc/macio/gpio.c b/hw/misc/macio/gpio.c
index 6cca6b27d6..6b4dfcc188 100644
--- a/hw/misc/macio/gpio.c
+++ b/hw/misc/macio/gpio.c
@@ -196,10 +196,10 @@  static void macio_gpio_reset(DeviceState *dev)
     macio_set_gpio(s, 1, true);
 }
 
-static void macio_gpio_nmi(NMIState *n, int cpu_index, Error **errp)
+static void macio_gpio_nmi(Object *parent, int cpu_index, Error **errp)
 {
-    macio_set_gpio(MACIO_GPIO(n), 9, true);
-    macio_set_gpio(MACIO_GPIO(n), 9, false);
+    macio_set_gpio(MACIO_GPIO(parent), 9, true);
+    macio_set_gpio(MACIO_GPIO(parent), 9, false);
 }
 
 static void macio_gpio_class_init(ObjectClass *oc, void *data)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f6023c..3b92e4013d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3377,7 +3377,7 @@  void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg)
     ppc_cpu_do_system_reset(cs);
 }
 
-static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
+static void spapr_nmi(Object *parent, int cpu_index, Error **errp)
 {
     CPUState *cs;
 
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef0ad..a49952a8b9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -431,7 +431,7 @@  static void s390_hot_add_cpu(MachineState *machine,
     s390x_new_cpu(object_class_get_name(oc), id, errp);
 }
 
-static void s390_nmi(NMIState *n, int cpu_index, Error **errp)
+static void s390_nmi(Object *parent, int cpu_index, Error **errp)
 {
     CPUState *cs = qemu_get_cpu(cpu_index);
 
diff --git a/include/hw/nmi.h b/include/hw/nmi.h
index a1e128724e..75afa67790 100644
--- a/include/hw/nmi.h
+++ b/include/hw/nmi.h
@@ -38,7 +38,7 @@  typedef struct NMIState NMIState;
 typedef struct NMIClass {
     InterfaceClass parent_class;
 
-    void (*nmi_monitor_handler)(NMIState *n, int cpu_index, Error **errp);
+    void (*nmi_monitor_handler)(Object *parent, int cpu_index, Error **errp);
 } NMIClass;
 
 void nmi_monitor_handle(int cpu_index, Error **errp);