[PATCH v1 1/4] qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting

[Daniel Henrique Barboza]

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