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

[Bernhard Beschow]

Am 5. Oktober 2023 07:06:38 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>Hi Bernhard,
>
>On 4/10/23 01:21, Bernhard Beschow wrote:
>> Am 3. Oktober 2023 08:27:25 UTC schrieb "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org>:
>>> 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(-)
>
>                        "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;
>>> -    }
>>> -
>> 
>> Did you intend to remove these lines in the next commit? There you're 
>> writing to simplify x86_cpu_apic_create() which you're doing here already.
>
>No: apic_get_class() doesn't return NULL anymore, so this is dead code.

Yes, makes sense.

Maybe move "It can't return NULL neither, so simplify x86_cpu_apic_create()." 
of the next commit message here? There, you're applying the same change to 
various functions but point out x86_cpu_apic_create() explicitly which is the 
part I was confused about. Though this may not warrant a respin.

Best regards,
Bernhard

>
>> 
>> Best regards,
>> Bernhard