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;
+ }
+
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
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(-)
>
>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;
>+ }
>+
> 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;
>- }
>-
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.
Best regards,
Bernhard
> cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
> object_property_add_child(OBJECT(cpu), "lapic",
> OBJECT(cpu->apic_state));
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.
>
> Best regards,
> Bernhard
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
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
© 2016 - 2026 Red Hat, Inc.