On Mon, 18 Sep 2023 18:02:34 +0200
Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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);
if errp doesn't lead to error_fatal/abort, wouldn't that effectively mask
following error
apic_get_class():
error_setg(errp, "KVM does not support userspace APIC");
return NULL;
???
>
> 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);
it would be better to explode in one place only inside apic_get_class(),
provided !kvm_irqchip_in_kernel() case is dealt with properly.
> 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;
> + }
better move 'if (cpu->apic_state == NULL) {' from x86_cpu_apic_realize()
to the caller, instead of repeating test again.
> }
> #endif /* !CONFIG_USER_ONLY */
> cpu_reset(cs);
