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

[Jiri Denemark]

On Tue, Dec 06, 2016 at 17:14:07 -0500, Jason J. Herne wrote:
> 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?

Yeah, a commend would be helpful.

> > 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.

This is not what virCPUDefStealModel is doing.

+     if (guest->mode != VIR_CPU_MODE_HOST_MODEL) {
+         ret = 0;
+         goto cleanup;
+     }
+
+     if (!host) {
+         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                        _("unknown host CPU model"));
+         goto cleanup;
+     }
+
+     if (!(updated = virCPUDefCopyWithoutModel(guest)))
+         goto cleanup;
+
+     updated->mode = VIR_CPU_MODE_CUSTOM;
+     if (virCPUDefCopyModel(updated, host, true) < 0)
+         goto cleanup;

"updated" contains just a copy of "host" CPU. The additional features
are in "guest".

+
+     virCPUDefStealModel(guest, updated, false);

And this just throws away all features in "guest" and replaces them with
those found in "updated".

In other words, before calling virCPUDefStealModel() you need to make
sure all features specified in "guest" are properly propagated to
"updated". See x86UpdateHostModel.

+     guest->mode = VIR_CPU_MODE_CUSTOM;
+     guest->match = VIR_CPU_MATCH_EXACT;

Jirka