Re: [PATCH 01/22] target/i386: Only realize existing APIC device

[Richard Henderson <richard.henderson@linaro.org>]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> APIC state is created under a certain condition,
> use the same condition to realize it.
> Having a NULL APIC state is a bug: use assert().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/i386/cpu-sysemu.c | 9 +++------
>   target/i386/cpu.c        | 8 +++++---
>   2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 2375e48178..6a164d3769 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -272,9 +272,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>       APICCommonState *apic;
>       APICCommonClass *apic_class = apic_get_class(errp);
>   
> -    if (!apic_class) {
> -        return;
> -    }
> +    assert(apic_class);

Ok.

>       cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
>       object_property_add_child(OBJECT(cpu), "lapic",
> @@ -293,9 +291,8 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>       APICCommonState *apic;
>       static bool apic_mmio_map_once;
>   
> -    if (cpu->apic_state == NULL) {
> -        return;
> -    }
> +    assert(cpu->apic_state);
> +
>       qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
>   
>       /* Map APIC MMIO area */
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2a20365e1..a23d4795e0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7448,9 +7448,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       }
>   
>   #ifndef CONFIG_USER_ONLY
> -    x86_cpu_apic_realize(cpu, &local_err);
> -    if (local_err != NULL) {
> -        goto out;
> +    if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
> +        x86_cpu_apic_realize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            goto out;
> +        }

I'm not keen on the replication of the condition.  Testing whether the apic object was 
created seems reasonable.  It probably is clearer with the IF in the caller, as you do.


r~