Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize()

[Zhao Liu <zhao1.liu@linux.intel.com>]

Hi Philippe,

On Wed, Jan 31, 2024 at 05:48:24PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 31 Jan 2024 17:48:24 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH] hw/intc: Handle the error of
>  IOAPICCommonClass.realize()
> 
> Hi Zhao,
> 
> On 31/1/24 15:29, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > IOAPICCommonClass implements its own private realize(), and this private
> > realize() allows error.
> > 
> > Therefore, return directly if IOAPICCommonClass.realize() meets error.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/intc/ioapic_common.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> > index cb9bf6214608..3772863377c2 100644
> > --- a/hw/intc/ioapic_common.c
> > +++ b/hw/intc/ioapic_common.c
> > @@ -162,6 +162,9 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
> >       info = IOAPIC_COMMON_GET_CLASS(s);
> >       info->realize(dev, errp);
> > +    if (*errp) {
> > +        return;
> > +    }
> 
> Could be clearer to deviate from DeviceRealize and let the
> handler return a boolean:
> 
> -- >8 --
> diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h
> index 37b8565539..9664bb3e00 100644
> --- a/hw/intc/ioapic_internal.h
> +++ b/hw/intc/ioapic_internal.h
> @@ -92,3 +92,3 @@ struct IOAPICCommonClass {
> 
> -    DeviceRealize realize;
> +    bool (*realize)(DeviceState *dev, Error **errp);

What about I change the name of this interface?

Maybe ioapic_realize(), to distinguish it from DeviceClass.realize().

>      DeviceUnrealize unrealize;

Additionally, if I change the pattern of realize(), should I also avoid
the DeviceUnrealize macro for symmetry's sake and just declare a similar
function pointer as you said?

Further, do you think it's necessary to introduce InternalRealize and
InternalUnrealize macros for qdev to wrap these special realize/unrealize
to differentiate them from normal DeviceRealize/DeviceUnrealize?

Because I found that this pattern of realize() (i.e. registering the
realize() of the child class in the parent class instead of DeviceClass,
and then calling the registered realize() in parent realize()) is also
widely used in many cases:

* xen_block_realize()
* virtser_port_device_realize()
* x86_iommu_realize()
* virtio_input_device_realize()
* apic_common_realize()
* pc_dimm_realize()
* virtio_device_realize()
...

I'm not quite sure if this is a generic way to use it, although it looks
like it could easily be confused with DeviceClass.realize().

> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
> index 409d0c8c76..96747ef2b8 100644
> --- a/hw/i386/kvm/ioapic.c
> +++ b/hw/i386/kvm/ioapic.c
> @@ -121,3 +121,3 @@ static void kvm_ioapic_set_irq(void *opaque, int irq,
> int level)
> 
> -static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
> +static bool kvm_ioapic_realize(DeviceState *dev, Error **errp)
>  {
> @@ -133,2 +133,4 @@ static void kvm_ioapic_realize(DeviceState *dev, Error
> **errp)
>      qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
> +
> +    return true;
>  }
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index cb9bf62146..beab65be04 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -163,3 +163,5 @@ static void ioapic_common_realize(DeviceState *dev,
> Error **errp)
>      info = IOAPIC_COMMON_GET_CLASS(s);
> -    info->realize(dev, errp);
> +    if (!info->realize(dev, errp)) {
> +        return;
> +    }
> 
> ---
> 
> What do you think?

I'm OK with the change here, but not sure if the return of private
realize() should be changed elsewhere as well.

Thanks,
Zhao