| Message ID | 20231003082728.83496-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/apic: QOM cleanup | expand |
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 >
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
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));
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(-)