[PATCH v2 0/5] hw/intc/apic: QOM cleanup

[PATCH v2 0/5] hw/intc/apic: QOM cleanup

[Philippe Mathieu-Daudé]

Since v1:
- Dropped change in x86_cpu_realizefn (rth)
- Simplify kvm_apic_realize() error propagation

Minor cleanup extracted from a bigger series
touching x86_cpu_realizefn().

Philippe Mathieu-Daudé (5):
  hw/intc/apic: Use ERRP_GUARD() in apic_common_realize()
  hw/i386/apic: Defer error check from apic_get_class to
    kvm_apic_realize
  hw/i386/apic: Simplify apic_get_class()
  hw/intc/apic: Rename x86_cpu_apic_create() -> x86_cpu_apic_new()
  hw/intc/apic: Pass CPU using QOM link property

 include/hw/i386/apic_internal.h |  2 +-
 target/i386/cpu-internal.h      |  2 +-
 hw/i386/amd_iommu.c             |  2 +-
 hw/i386/intel_iommu.c           |  4 ++--
 hw/i386/kvm/apic.c              |  5 +++++
 hw/intc/apic_common.c           |  8 +++++++-
 target/i386/cpu-sysemu.c        | 23 +++++++----------------
 target/i386/cpu.c               |  5 +----
 8 files changed, 25 insertions(+), 26 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] hw/intc/apic: Use ERRP_GUARD() in apic_common_realize()

[Philippe Mathieu-Daudé]

APICCommonClass::realize() is a DeviceRealize() handler which
take an Error** parameter and can fail. Do not proceed further
on failure.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/apic_common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 68ad30e2f5..bccb4241c2 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -257,6 +257,7 @@ static const VMStateDescription vmstate_apic_common;
 
 static void apic_common_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
@@ -267,6 +268,9 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
+    if (*errp) {
+        return;
+    }
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
-- 
2.41.0

Re: [PATCH v2 1/5] hw/intc/apic: Use ERRP_GUARD() in apic_common_realize()

[Peter Xu]

On Tue, Oct 03, 2023 at 10:27:24AM +0200, Philippe Mathieu-Daudé wrote:
> APICCommonClass::realize() is a DeviceRealize() handler which
> take an Error** parameter and can fail. Do not proceed further
> on failure.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

[Philippe Mathieu-Daudé]

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

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

[Peter Xu]

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

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

[Bernhard Beschow]

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));

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

[Philippe Mathieu-Daudé]

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

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

[PATCH v2 3/5] hw/i386/apic: Simplify apic_get_class()

[Philippe Mathieu-Daudé]

Now than apic_get_class() can not fail, remove its
Error** parameter. It can't return NULL neither, so
simplify x86_cpu_apic_create().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/apic_internal.h | 2 +-
 hw/i386/amd_iommu.c             | 2 +-
 hw/i386/intel_iommu.c           | 4 ++--
 target/i386/cpu-sysemu.c        | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 5f2ba24bfc..e61ad04769 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -225,6 +225,6 @@ static inline int apic_get_bit(uint32_t *tab, int index)
     return !!(tab[i] & mask);
 }
 
-APICCommonClass *apic_get_class(Error **errp);
+APICCommonClass *apic_get_class(void);
 
 #endif /* QEMU_APIC_INTERNAL_H */
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index c98a3c6e11..0a95025ab7 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1368,7 +1368,7 @@ static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
         return MEMTX_ERROR;
     }
 
-    apic_get_class(NULL)->send_msi(&to);
+    apic_get_class()->send_msi(&to);
 
     trace_amdvi_mem_ir_write(to.address, to.data);
     return MEMTX_OK;
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2c832ab68b..dffe3583bd 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -397,7 +397,7 @@ static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
 
     trace_vtd_irq_generate(msi.address, msi.data);
 
-    apic_get_class(NULL)->send_msi(&msi);
+    apic_get_class()->send_msi(&msi);
 }
 
 /* Generate a fault event to software via MSI if conditions are met.
@@ -3554,7 +3554,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
         return MEMTX_ERROR;
     }
 
-    apic_get_class(NULL)->send_msi(&to);
+    apic_get_class()->send_msi(&to);
 
     return MEMTX_OK;
 }
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 6a228c9178..9038c65267 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -247,7 +247,7 @@ void x86_cpu_machine_reset_cb(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
-APICCommonClass *apic_get_class(Error **errp)
+APICCommonClass *apic_get_class(void)
 {
     const char *apic_type = "apic";
 
@@ -266,7 +266,7 @@ APICCommonClass *apic_get_class(Error **errp)
 void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
     APICCommonState *apic;
-    APICCommonClass *apic_class = apic_get_class(errp);
+    APICCommonClass *apic_class = apic_get_class();
 
     cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
     object_property_add_child(OBJECT(cpu), "lapic",
-- 
2.41.0

Re: [PATCH v2 3/5] hw/i386/apic: Simplify apic_get_class()

[Peter Xu]

On Tue, Oct 03, 2023 at 10:27:26AM +0200, Philippe Mathieu-Daudé wrote:
> Now than apic_get_class() can not fail, remove its
> Error** parameter. It can't return NULL neither, so
> simplify x86_cpu_apic_create().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 3/5] hw/i386/apic: Simplify apic_get_class()

[Bernhard Beschow]

Am 3. Oktober 2023 08:27:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Now than apic_get_class() can not fail, remove its

s/than/that/
s/can not/can't/ (which is stylistically consistent with "neither" below)

Best regards,
Bernhard

>Error** parameter. It can't return NULL neither, so
>simplify x86_cpu_apic_create().
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/hw/i386/apic_internal.h | 2 +-
> hw/i386/amd_iommu.c             | 2 +-
> hw/i386/intel_iommu.c           | 4 ++--
> target/i386/cpu-sysemu.c        | 4 ++--
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
>index 5f2ba24bfc..e61ad04769 100644
>--- a/include/hw/i386/apic_internal.h
>+++ b/include/hw/i386/apic_internal.h
>@@ -225,6 +225,6 @@ static inline int apic_get_bit(uint32_t *tab, int index)
>     return !!(tab[i] & mask);
> }
> 
>-APICCommonClass *apic_get_class(Error **errp);
>+APICCommonClass *apic_get_class(void);
> 
> #endif /* QEMU_APIC_INTERNAL_H */
>diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>index c98a3c6e11..0a95025ab7 100644
>--- a/hw/i386/amd_iommu.c
>+++ b/hw/i386/amd_iommu.c
>@@ -1368,7 +1368,7 @@ static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
>         return MEMTX_ERROR;
>     }
> 
>-    apic_get_class(NULL)->send_msi(&to);
>+    apic_get_class()->send_msi(&to);
> 
>     trace_amdvi_mem_ir_write(to.address, to.data);
>     return MEMTX_OK;
>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>index 2c832ab68b..dffe3583bd 100644
>--- a/hw/i386/intel_iommu.c
>+++ b/hw/i386/intel_iommu.c
>@@ -397,7 +397,7 @@ static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
> 
>     trace_vtd_irq_generate(msi.address, msi.data);
> 
>-    apic_get_class(NULL)->send_msi(&msi);
>+    apic_get_class()->send_msi(&msi);
> }
> 
> /* Generate a fault event to software via MSI if conditions are met.
>@@ -3554,7 +3554,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
>         return MEMTX_ERROR;
>     }
> 
>-    apic_get_class(NULL)->send_msi(&to);
>+    apic_get_class()->send_msi(&to);
> 
>     return MEMTX_OK;
> }
>diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>index 6a228c9178..9038c65267 100644
>--- a/target/i386/cpu-sysemu.c
>+++ b/target/i386/cpu-sysemu.c
>@@ -247,7 +247,7 @@ void x86_cpu_machine_reset_cb(void *opaque)
>     cpu_reset(CPU(cpu));
> }
> 
>-APICCommonClass *apic_get_class(Error **errp)
>+APICCommonClass *apic_get_class(void)
> {
>     const char *apic_type = "apic";
> 
>@@ -266,7 +266,7 @@ APICCommonClass *apic_get_class(Error **errp)
> void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> {
>     APICCommonState *apic;
>-    APICCommonClass *apic_class = apic_get_class(errp);
>+    APICCommonClass *apic_class = apic_get_class();
> 
>     cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
>     object_property_add_child(OBJECT(cpu), "lapic",

[PATCH v2 4/5] hw/intc/apic: Rename x86_cpu_apic_create() -> x86_cpu_apic_new()

[Philippe Mathieu-Daudé]

x86_cpu_apic_create()'s Error** parameter is unused, drop it.

While there is no convention, QDev methods are usually named
with _new() / _realize() suffixes. Rename as appropriate.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu-internal.h | 2 +-
 hw/intc/apic_common.c      | 2 +-
 target/i386/cpu-sysemu.c   | 2 +-
 target/i386/cpu.c          | 5 +----
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
index 9baac5c0b4..8299b347f7 100644
--- a/target/i386/cpu-internal.h
+++ b/target/i386/cpu-internal.h
@@ -62,7 +62,7 @@ GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs);
 void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
                                 const char *name, void *opaque, Error **errp);
 
-void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
+void x86_cpu_apic_new(X86CPU *cpu);
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
 void x86_cpu_machine_reset_cb(void *opaque);
 #endif /* !CONFIG_USER_ONLY */
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index bccb4241c2..8a79eacdb0 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -451,7 +451,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
     dc->unrealize = apic_common_unrealize;
     /*
      * Reason: APIC and CPU need to be wired up by
-     * x86_cpu_apic_create()
+     * x86_cpu_apic_new()
      */
     dc->user_creatable = false;
 }
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 9038c65267..373dc6b1c7 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -263,7 +263,7 @@ APICCommonClass *apic_get_class(void)
     return APIC_COMMON_CLASS(object_class_by_name(apic_type));
 }
 
-void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
+void x86_cpu_apic_new(X86CPU *cpu)
 {
     APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class();
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ed72883bf3..69d56bae91 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7418,10 +7418,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
-        x86_cpu_apic_create(cpu, &local_err);
-        if (local_err != NULL) {
-            goto out;
-        }
+        x86_cpu_apic_new(cpu);
     }
 #endif
 
-- 
2.41.0

Re: [PATCH v2 4/5] hw/intc/apic: Rename x86_cpu_apic_create() -> x86_cpu_apic_new()

[Peter Xu]

On Tue, Oct 03, 2023 at 10:27:27AM +0200, Philippe Mathieu-Daudé wrote:
> x86_cpu_apic_create()'s Error** parameter is unused, drop it.
> 
> While there is no convention, QDev methods are usually named
> with _new() / _realize() suffixes. Rename as appropriate.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 4/5] hw/intc/apic: Rename x86_cpu_apic_create() -> x86_cpu_apic_new()

[Paolo Bonzini]

On 10/3/23 10:27, Philippe Mathieu-Daudé wrote:
> -        x86_cpu_apic_create(cpu, &local_err);
> -        if (local_err != NULL) {
> -            goto out;
> -        }
> +        x86_cpu_apic_new(cpu);

I don't like this, "*_new" is generally for functions that return what 
they create.

Patch 2 is scary with the newly-introduced possible failure, but I 
suppose it's safer if you reason that any problem will occur at startup, 
not at hotplug time for example.

Paolo

[PATCH v2 5/5] hw/intc/apic: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

Declare the 'cpu' and 'base-addr' properties, set them
using object_property_set_link() and qdev_prop_set_uint32()
respectively. Since the _set_link() call can't fail, use
&error_abort in case there is a programming error.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/apic_common.c    | 2 ++
 target/i386/cpu-sysemu.c | 9 ++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 8a79eacdb0..be7cf3b332 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -398,6 +398,8 @@ static Property apic_properties_common[] = {
                     true),
     DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
                      false),
+    DEFINE_PROP_LINK("cpu", APICCommonState, cpu, TYPE_X86_CPU, X86CPU *),
+    DEFINE_PROP_UINT32("base-addr", APICCommonState, apicbase, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 373dc6b1c7..b084706531 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -265,7 +265,6 @@ APICCommonClass *apic_get_class(void)
 
 void x86_cpu_apic_new(X86CPU *cpu)
 {
-    APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class();
 
     cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
@@ -273,11 +272,11 @@ void x86_cpu_apic_new(X86CPU *cpu)
                               OBJECT(cpu->apic_state));
     object_unref(OBJECT(cpu->apic_state));
 
+    object_property_set_link(OBJECT(cpu->apic_state), "cpu",
+                             OBJECT(cpu), &error_abort);
     qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
-    /* TODO: convert to link<> */
-    apic = APIC_COMMON(cpu->apic_state);
-    apic->cpu = cpu;
-    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
+    qdev_prop_set_uint32(cpu->apic_state, "base-addr",
+                         APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
 }
 
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
2.41.0

Re: [PATCH v2 5/5] hw/intc/apic: Pass CPU using QOM link property

[Paolo Bonzini]

On 10/3/23 10:27, Philippe Mathieu-Daudé wrote:
> -    /* TODO: convert to link<> */
> -    apic = APIC_COMMON(cpu->apic_state);
> -    apic->cpu = cpu;
> -    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
> +    qdev_prop_set_uint32(cpu->apic_state, "base-addr",
> +                         APIC_DEFAULT_ADDRESS | MSR_IA32_APIC

For this to use a link, it's missing the corresponding 
object_unref(apic->cpu) + apic->cpu = NULL assignment somewhere.  For 
example you can add it in apic_common_unrealize (called by 
device_unparent - which is called in turn by x86_cpu_unrealizefn).

Paolo

Re: [PATCH v2 5/5] hw/intc/apic: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

Hi Paolo,

On 6/10/23 01:04, Paolo Bonzini wrote:
> On 10/3/23 10:27, Philippe Mathieu-Daudé wrote:
>> -    /* TODO: convert to link<> */
>> -    apic = APIC_COMMON(cpu->apic_state);
>> -    apic->cpu = cpu;
>> -    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
>> +    qdev_prop_set_uint32(cpu->apic_state, "base-addr",
>> +                         APIC_DEFAULT_ADDRESS | MSR_IA32_APIC
> 
> For this to use a link, it's missing the corresponding 
> object_unref(apic->cpu) + apic->cpu = NULL assignment somewhere.  For 
> example you can add it in apic_common_unrealize (called by 
> device_unparent - which is called in turn by x86_cpu_unrealizefn).

I am a bit confused.

DEFINE_PROP_LINK() sets OBJ_PROP_LINK_STRONG:

  * If the link property was created with
  * %OBJ_PROP_LINK_STRONG bit, the old target object is
  * unreferenced, and a reference is added to the new target object.

Is this what you are pointing at? If so, I agree this should be
unref in apic_common_unrealize().