[PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[Zhao Liu]

Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (
e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
reported:

Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
Aborted (core dumped)

This error can be resolved by setting x2apic=on in -cpu. In order to
better help users deal with this scenario, add the error hint to
instruct users on how to enable the x2apic feature. Then, the error
report becomes the following:

Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
Try x2apic=on in -cpu.
Aborted (core dumped)

Note since @errp is &error_abort, error_append_hint() can't be applied
on @errp. And in order to separate the exact error message from the
(perhaps effectively) hint, adding a hint via error_append_hint() is
also necessary. Therefore, introduce @local_error in
apic_common_set_id() to handle both the error message and the error
hint.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/intc/apic_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d8fc1e2815fe..c13cdd79943d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -433,6 +433,7 @@ static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
     APICCommonState *s = APIC_COMMON(obj);
     DeviceState *dev = DEVICE(obj);
     uint32_t value;
+    Error *local_err = NULL;
 
     if (dev->realized) {
         qdev_prop_set_after_realize(dev, name, errp);
@@ -444,7 +445,11 @@ static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
     }
 
     if (value >= 255 && !cpu_has_x2apic_feature(&s->cpu->env)) {
-        error_setg(errp, "APIC ID %d requires x2APIC feature in CPU", value);
+        error_setg(&local_err,
+                   "APIC ID %d requires x2APIC feature in CPU",
+                   value);
+        error_append_hint(&local_err, "Try x2apic=on in -cpu.\n");
+        error_propagate(errp, local_err);
         return;
     }
 
-- 
2.34.1

Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[Philippe Mathieu-Daudé]

On 6/6/24 16:08, Zhao Liu wrote:
> Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
> cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (
> e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
> reported:
> 
> Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
> qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> Aborted (core dumped)
> 
> This error can be resolved by setting x2apic=on in -cpu. In order to
> better help users deal with this scenario, add the error hint to
> instruct users on how to enable the x2apic feature.

Why not automatically set x2apic=on in this case instead?

> Then, the error
> report becomes the following:
> 
> Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
> qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> Try x2apic=on in -cpu.
> Aborted (core dumped)
> 
> Note since @errp is &error_abort, error_append_hint() can't be applied
> on @errp. And in order to separate the exact error message from the
> (perhaps effectively) hint, adding a hint via error_append_hint() is
> also necessary. Therefore, introduce @local_error in
> apic_common_set_id() to handle both the error message and the error
> hint.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/intc/apic_common.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[Zhao Liu]

Hi Philippe,

On Fri, Jun 07, 2024 at 08:17:36AM +0200, Philippe Mathieu-Daudé wrote:
> Date: Fri, 7 Jun 2024 08:17:36 +0200
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of
>  disabling x2APIC
> 
> On 6/6/24 16:08, Zhao Liu wrote:
> > Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
> > cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (
> > e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
> > reported:
> > 
> > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
> > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> > Aborted (core dumped)
> > 
> > This error can be resolved by setting x2apic=on in -cpu. In order to
> > better help users deal with this scenario, add the error hint to
> > instruct users on how to enable the x2apic feature.
> 
> Why not automatically set x2apic=on in this case instead?

The default CPU model is qemu64 without x2APIC. I think it might be
necessary to update the version to add x2APIC in qemu64, and I'd like to
look into it again for any other potential issues.

In addition to adding x2APIC directly to the qemu64, this hint can also
help some users who want a large number of vCPUs based on other older
CPU models. Though, it's not very common but this hint would be helpful.

> > Then, the error
> > report becomes the following:
> > 
> > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
> > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> > Try x2apic=on in -cpu.
> > Aborted (core dumped)
> > 
> > Note since @errp is &error_abort, error_append_hint() can't be applied
> > on @errp. And in order to separate the exact error message from the
> > (perhaps effectively) hint, adding a hint via error_append_hint() is
> > also necessary. Therefore, introduce @local_error in
> > apic_common_set_id() to handle both the error message and the error
> > hint.
> > 
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/intc/apic_common.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[Xiaoyao Li]

On 6/7/2024 3:46 PM, Zhao Liu wrote:
> Hi Philippe,
> 
> On Fri, Jun 07, 2024 at 08:17:36AM +0200, Philippe Mathieu-Daudé wrote:
>> Date: Fri, 7 Jun 2024 08:17:36 +0200
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of
>>   disabling x2APIC
>>
>> On 6/6/24 16:08, Zhao Liu wrote:
>>> Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
>>> cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (

why bother mentioning TCG case?

you below example is not for TCG. And the issue is not caused by TCG but 
default cpu model doesn't have X2APIC enabled.

>>> e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
>>> reported:
>>>
>>> Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
>>> qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
>>> Aborted (core dumped)
>>>
>>> This error can be resolved by setting x2apic=on in -cpu. In order to
>>> better help users deal with this scenario, add the error hint to
>>> instruct users on how to enable the x2apic feature.
>>
>> Why not automatically set x2apic=on in this case instead?
> 
> The default CPU model is qemu64 without x2APIC. I think it might be
> necessary to update the version to add x2APIC in qemu64, and I'd like to
> look into it again for any other potential issues.
> 
> In addition to adding x2APIC directly to the qemu64, this hint can also
> help some users who want a large number of vCPUs based on other older
> CPU models. Though, it's not very common but this hint would be helpful.
> 
>>> Then, the error
>>> report becomes the following:
>>>
>>> Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
>>> qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
>>> Try x2apic=on in -cpu.
>>> Aborted (core dumped)
>>>
>>> Note since @errp is &error_abort, error_append_hint() can't be applied
>>> on @errp. And in order to separate the exact error message from the
>>> (perhaps effectively) hint, adding a hint via error_append_hint() is
>>> also necessary. Therefore, introduce @local_error in
>>> apic_common_set_id() to handle both the error message and the error
>>> hint.
>>>
>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>> ---
>>>    hw/intc/apic_common.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Thanks!
> 
> 

Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC

[Zhao Liu]

On Fri, Jun 07, 2024 at 03:47:01PM +0800, Xiaoyao Li wrote:
> Date: Fri, 7 Jun 2024 15:47:01 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of
>  disabling x2APIC
> 
> On 6/7/2024 3:46 PM, Zhao Liu wrote:
> > Hi Philippe,
> > 
> > On Fri, Jun 07, 2024 at 08:17:36AM +0200, Philippe Mathieu-Daudé wrote:
> > > Date: Fri, 7 Jun 2024 08:17:36 +0200
> > > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of
> > >   disabling x2APIC
> > > 
> > > On 6/6/24 16:08, Zhao Liu wrote:
> > > > Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
> > > > cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (
> 
> why bother mentioning TCG case?

I'm restating the problem scenario for this error message. This is a
common use case, which is enough to support adding a simple hint.

> you below example is not for TCG. 

Not?

> And the issue is not caused by TCG but
> default cpu model doesn't have X2APIC enabled.

This is not an issue, and it is normal to report an error in current QEMU.

> > > > e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
> > > > reported:
> > > > 
> > > > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
> > > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> > > > Aborted (core dumped)
> > > > 
> > > > This error can be resolved by setting x2apic=on in -cpu. In order to
> > > > better help users deal with this scenario, add the error hint to
> > > > instruct users on how to enable the x2apic feature.
> > > 
> > > Why not automatically set x2apic=on in this case instead?
> > 
> > The default CPU model is qemu64 without x2APIC. I think it might be
> > necessary to update the version to add x2APIC in qemu64, and I'd like to
> > look into it again for any other potential issues.
> > 
> > In addition to adding x2APIC directly to the qemu64, this hint can also
> > help some users who want a large number of vCPUs based on other older
> > CPU models. Though, it's not very common but this hint would be helpful.
> > 
> > > > Then, the error
> > > > report becomes the following:
> > > > 
> > > > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
> > > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> > > > Try x2apic=on in -cpu.
> > > > Aborted (core dumped)
> > > > 
> > > > Note since @errp is &error_abort, error_append_hint() can't be applied
> > > > on @errp. And in order to separate the exact error message from the
> > > > (perhaps effectively) hint, adding a hint via error_append_hint() is
> > > > also necessary. Therefore, introduce @local_error in
> > > > apic_common_set_id() to handle both the error message and the error
> > > > hint.
> > > > 
> > > > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > ---
> > > >    hw/intc/apic_common.c | 7 ++++++-
> > > >    1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > Thanks!
> > 
> > 
>