The "<apic/>" feature, although it's not available for pseries,
can be declared in the domain XML of ppc64 guests without errors.
But setting its 'eoi' attribute will break QEMU. For "<apic eoi='on'/>":
qemu-kvm: Expected key=value format, found +kvm_pv_eoi
A similar error happens with eoi='off'.
One can argue that it's better to simply forbid launching ppc64
guests with "<apic/>" declared in the ppc64 XML - it is a feature that
the architecture doesn't support and this would make it clearer
about it. This is sensible, but there are ppc64 guests that are running
with "<apic/>" declared in the domain (and A LOT of guests running with
"<acpi/>" for that matter, probably reminiscent of x86 templates that
were reused for ppc64) that will stop working if we go this route.
A more subtle approach is to detect if the 'eoi' element is being set
for ppc64 guests, and warn the user about it with a better error
message than the one QEMU provides. This is the new error message
when any value is set for the 'eoi' element:
error: unsupported configuration: The 'eoi' attribute of the 'apic'
feature is not supported for architecture 'ppc64' or machine type
'pseries'.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
src/qemu/qemu_domain.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index edc8ba2ddb..381b62b96a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5262,8 +5262,26 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
}
break;
- case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_APIC:
+ /* the <apic/> declaration is harmless for ppc64, but
+ * its 'eoi' attribute isn't. To detect if 'eoi' was
+ * present in the XML we can check the tristate switch
+ * of def->apic_eoi */
+ if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+ def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT &&
+ ARCH_IS_PPC64(def->os.arch)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The 'eoi' attribute of the '%s' feature "
+ "is not supported for architecture '%s' or "
+ "machine type '%s'"),
+ featureName,
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
--
2.25.1
On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote:
> qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting
s/qemu_domain.c/qemu/
Same for the other patches in the series.
> The "<apic/>" feature, although it's not available for pseries,
> can be declared in the domain XML of ppc64 guests without errors.
> But setting its 'eoi' attribute will break QEMU. For "<apic eoi='on'/>":
>
> qemu-kvm: Expected key=value format, found +kvm_pv_eoi
>
> A similar error happens with eoi='off'.
This is
https://bugzilla.redhat.com/show_bug.cgi?id=1236440
Please include the Bugzilla URL for other patches in the series
as well, if applicable.
[...]
> case VIR_DOMAIN_FEATURE_APIC:
> + /* the <apic/> declaration is harmless for ppc64, but
> + * its 'eoi' attribute isn't. To detect if 'eoi' was
> + * present in the XML we can check the tristate switch
> + * of def->apic_eoi */
> + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
> + def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT &&
> + ARCH_IS_PPC64(def->os.arch)) {
Since the underlying kvm_pv_eoi feature is x86-only, you should
change this last part to
!ARCH_IS_X86(def->os.arch)
and benefit ARM, RISC-V and s390x users at the same time. When you
do so, please reduce the comment to something like
/* The kvm_pv_eoi feature is x86-only */
--
Andrea Bolognani / Red Hat / Virtualization
On 3/23/20 2:01 PM, Andrea Bolognani wrote:
> On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote:
>> qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting
>
> s/qemu_domain.c/qemu/
>
> Same for the other patches in the series.
>
>> The "<apic/>" feature, although it's not available for pseries,
>> can be declared in the domain XML of ppc64 guests without errors.
>> But setting its 'eoi' attribute will break QEMU. For "<apic eoi='on'/>":
>>
>> qemu-kvm: Expected key=value format, found +kvm_pv_eoi
>>
>> A similar error happens with eoi='off'.
>
> This is
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1236440
>
> Please include the Bugzilla URL for other patches in the series
> as well, if applicable.
Sure. I didn't include the link because it can't be opened unless you have a RH bugzilla
account and I wasn't sure I could add it here.
>
> [...]
>> case VIR_DOMAIN_FEATURE_APIC:
>> + /* the <apic/> declaration is harmless for ppc64, but
>> + * its 'eoi' attribute isn't. To detect if 'eoi' was
>> + * present in the XML we can check the tristate switch
>> + * of def->apic_eoi */
>> + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
>> + def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT &&
>> + ARCH_IS_PPC64(def->os.arch)) {
>
> Since the underlying kvm_pv_eoi feature is x86-only, you should
> change this last part to
>
> !ARCH_IS_X86(def->os.arch)
>
> and benefit ARM, RISC-V and s390x users at the same time. When you
> do so, please reduce the comment to something like
I got a bit confused when doing these patches with what aarch64 supports or not
(specially kvm_pv_unhalt, the one from patch 2) then I decided to play it safer.
I'll gate this one (and the one from patch 2) to be x86 only.
And while we're at it, something that just occurred to me, I'll also gate the ppc64
only capabilities as well in a new patch.
>
> /* The kvm_pv_eoi feature is x86-only */
>
I'll reduce the comments as well.
Thanks,
DHB
On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote: > On 3/23/20 2:01 PM, Andrea Bolognani wrote: > > This is > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1236440 > > > > Please include the Bugzilla URL for other patches in the series > > as well, if applicable. > > Sure. I didn't include the link because it can't be opened unless you have a RH bugzilla > account and I wasn't sure I could add it here. Oh, I did not realize that bug was private. That's very silly. I'll ask for it to be made public. > And while we're at it, something that just occurred to me, I'll also gate the ppc64 > only capabilities as well in a new patch. Yeah, that makes sense too. We're probably never going to get to a point where these checks are completely accurate, but any improvement can only be a welcome one :) -- Andrea Bolognani / Red Hat / Virtualization
On 3/23/20 2:45 PM, Andrea Bolognani wrote:
> On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote:
>> On 3/23/20 2:01 PM, Andrea Bolognani wrote:
>>> This is
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1236440
>>>
>>> Please include the Bugzilla URL for other patches in the series
>>> as well, if applicable.
>>
>> Sure. I didn't include the link because it can't be opened unless you have a RH bugzilla
>> account and I wasn't sure I could add it here.
>
> Oh, I did not realize that bug was private. That's very silly. I'll
> ask for it to be made public.
>
>> And while we're at it, something that just occurred to me, I'll also gate the ppc64
>> only capabilities as well in a new patch.
>
> Yeah, that makes sense too. We're probably never going to get to a
> point where these checks are completely accurate, but any improvement
> can only be a welcome one :)
Just realized that we're already doing that here:
case VIR_DOMAIN_FEATURE_HPT:
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
case VIR_DOMAIN_FEATURE_CCF_ASSIST:
if (qemuDomainDefValidatePSeriesFeature(def, qemuCaps, i) < 0)
return -1;
break;
qemuDomainDefValidatePseriesFeature() filters if the arch is ppc64:
static int
qemuDomainDefValidatePSeriesFeature(const virDomainDef *def,
virQEMUCapsPtr qemuCaps,
int feature)
{
const char *str;
if (def->features[feature] != VIR_TRISTATE_SWITCH_ABSENT &&
!qemuDomainIsPSeries(def)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%s' feature is not supported for "
"architecture '%s' or machine type '%s'"),
virDomainFeatureTypeToString(feature),
virArchToString(def->os.arch),
def->os.machine);
return -1;
}
(...)
There is no need for an extra patch to handle it.
Thanks,
DHB
>
On Tue, 2020-03-24 at 10:22 -0300, Daniel Henrique Barboza wrote: > On 3/23/20 2:45 PM, Andrea Bolognani wrote: > > On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote: > > > And while we're at it, something that just occurred to me, I'll also gate the ppc64 > > > only capabilities as well in a new patch. > > > > Yeah, that makes sense too. We're probably never going to get to a > > point where these checks are completely accurate, but any improvement > > can only be a welcome one :) > > Just realized that we're already doing that here: > > case VIR_DOMAIN_FEATURE_HPT: > case VIR_DOMAIN_FEATURE_HTM: > case VIR_DOMAIN_FEATURE_NESTED_HV: > case VIR_DOMAIN_FEATURE_CCF_ASSIST: > if (qemuDomainDefValidatePSeriesFeature(def, qemuCaps, i) < 0) > return -1; > break; Right! I had forgotten about writing that validation logic O:-) > There is no need for an extra patch to handle it. Nope, we're good :) -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2026 Red Hat, Inc.