Re: [PATCH v2 2/5] hw/i386/apic: Defer error check from apic_get_class to kvm_apic_realize

[Peter Xu]

On Tue, Oct 03, 2023 at 10:27:25AM +0200, Philippe Mathieu-Daudé wrote:
> apic_get_class() isn't supposed to fail. kvm_apic_realize() is
> DeviceRealize() handler, which can fail. Defer the error check
> to the latter.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/i386/kvm/apic.c       | 5 +++++
>  target/i386/cpu-sysemu.c | 8 --------
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 1e89ca0899..4883308247 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -228,6 +228,11 @@ static void kvm_apic_realize(DeviceState *dev, Error 
> **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
>  
> +    if (!kvm_irqchip_in_kernel()) {
> +        error_setg(errp, "KVM does not support userspace APIC");
> +        return;
> +    }

IMO the code reads a bit weird, that we are already trying to create
kvm-apic even if !kvm_irqchip_in_kernel()..

Would it be better to check this in i386's kvm_arch_irqchip_create()?

> +
>      memory_region_init_io(&s->io_memory, OBJECT(s), &kvm_apic_io_ops, s,
>                            "kvm-apic-msi", APIC_SPACE_SIZE);
>  
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 2375e48178..6a228c9178 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -253,10 +253,6 @@ APICCommonClass *apic_get_class(Error **errp)
>  
>      /* TODO: in-kernel irqchip for hvf */
>      if (kvm_enabled()) {
> -        if (!kvm_irqchip_in_kernel()) {
> -            error_setg(errp, "KVM does not support userspace APIC");
> -            return NULL;
> -        }
>          apic_type = "kvm-apic";
>      } else if (xen_enabled()) {
>          apic_type = "xen-apic";
> @@ -272,10 +268,6 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      APICCommonState *apic;
>      APICCommonClass *apic_class = apic_get_class(errp);
>  
> -    if (!apic_class) {
> -        return;
> -    }
> -
>      cpu->apic_state = 
> DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
>      object_property_add_child(OBJECT(cpu), "lapic",
>                                OBJECT(cpu->apic_state));
> -- 
> 2.41.0
> 

-- 
Peter Xu