[libvirt] [PATCH 1/6] s390: Cpu driver support for update and compare

[Jason J. Herne]

On 11/28/2016 04:55 AM, Jiri Denemark wrote:
...
>> +static virCPUCompareResult
>> +virCPUs390Compare(virCPUDefPtr host ATTRIBUTE_UNUSED,
>> +                 virCPUDefPtr cpu ATTRIBUTE_UNUSED,
>> +                 bool failMessages ATTRIBUTE_UNUSED)
>
> The indentation is a bit off here.
>
>> +{
>> +    return VIR_CPU_COMPARE_IDENTICAL;
>> +}
>
> I know there is no code to detect host CPU, but this essentially means
> that any guest CPU configuration can be started on any s390 host (I'm
> talking about KVM, of course). Is this correct or would it make sense to
> somehow compare the guest CPU with the host model returned by QEMU?

We rely on Qemu to perform all runability checking. Not duplicating the
checks in Libvirt was a design choice. We are returning
VIR_CPU_COMPARE_IDENTICAL to essentially bypass Libvirt checking. perhaps
we should just comment that fact here?

> Which reminds me I don't think I've ever seen any s390 CPU XML. Could
> you just add some test cases focused on s390 CPUs to qemuxml2argvtest?

Sure :)

> And since this series is largely about QEMU capabilities, it would be
> nice if you could add a few .replies and corresponding .xml files to
> tests/qemucapabilitiesdata and also use them in domaincapstest?

Yep.

...

> Are you going to support additional features for host-model CPUs? In
> other words, does the following XML make sense in s390?
>
>     <cpu mode='host-model'>
>       <feature name='bla' policy='require'/>
>       <feature name='ble' policy='disable'/>
>     </cpu>
>
> If so, the code is insufficient. Otherwise, it's unnecessarily
> complicated. There's no need to create "updated" CPU model when you
> don't need to look at the features specified in the original guest CPU
> definition.

Yes we are allowing policy='require' and policy='disable'. I don't 
understand
why the current code is insufficient. It seems like virCPUDefPtr
guest->features already has the guest's features with the correct policy
statements. Our call to virCPUDefStealModel will copy the features pointer
from guest to updated. What am I missing.

-- 
-- Jason J. Herne (jjherne at linux.vnet.ibm.com)