On Thu, Jun 25, 2020 at 09:06:05AM +0200, David Hildenbrand wrote:
> >> Still unsure how to bring this new machine property and the cpu feature
> >> together. Would be great to have the same interface everywhere, but
> >> having two distinct command line objects depend on each other sucks.
> >
> > Kinda, but the reality is that hardware - virtual and otherwise -
> > frequently doesn't have entirely orthogonal configuration for each of
> > its components. This is by no means new in that regard.
> >
> >> Automatically setting the feature bit if pv is supported complicates
> >> things further.
> >
> > AIUI, on s390 the "unpack" feature is available by default on recent
> > models. In that case you could do this:
> >
> > * Don't modify either cpu or HTL options based on each other
> > * Bail out if the user specifies a non "unpack" secure CPU along with
> > the HTL option
> >
> > Cases of note:
> > - User specifies an old CPU model + htl
> > or explicitly sets unpack=off + htl
> > => fails with an error, correctly
> > - User specifies modern/default cpu + htl, with secure aware guest
> > => works as a secure guest
> > - User specifies modern/default cpu + htl, with non secure aware guest
> > => works, though not secure (and maybe slower than neccessary)
> > - User specifies modern/default cpu, no htl, with non-secure guest
> > => works, "unpack" feature is present but unused
> > - User specifies modern/default cpu, no htl, secure guest
> > => this is the worst one. It kind of works by accident if
> > you've also manually specified whatever virtio (and
> > anything else) options are necessary. Ugly, but no
> > different from the situation right now, IIUC
> >
> >> (Is there any requirement that the machine object has been already set
> >> up before the cpu features are processed? Or the other way around?)
> >
> > CPUs are usually created by the machine, so I believe we can count on
> > the machine object being there first.
>
> CPU model initialization is one of the first things machine
> initialization code does on s390x.
As it is for most platforms, but still, the values of machine
properties are available to you at this point.
> static void ccw_init(MachineState *machine)
> {
> [... memory init ...]
> s390_sclp_init();
> s390_memory_init(machine->ram);
> /* init CPUs (incl. CPU model) early so s390_has_feature() works */
> s390_init_cpus(machine);
> [...]
> }
>
> >
> >> Does this have any implications when probing with the 'none' machine?
> >
> > I'm not sure. In your case, I guess the cpu bit would still show up
> > as before, so it would tell you base feature availability, but not
> > whether you can use the new configuration option.
> >
> > Since the HTL option is generic, you could still set it on the "none"
> > machine, though it wouldn't really have any effect. That is, if you
> > could create a suitable object to point it at, which would depend on
> > ... details.
> >
>
> The important point is that we never want the (expanded) host cpu model
> look different when either specifying or not specifying the HTL
> property.
Ah, yes, I see your point. So my current suggestion will satisfy
that, basically it is:
cpu has unpack (inc. by default) && htl specified
=> works (allowing secure), as expected
!cpu has unpack && htl specified
=> bails out with an error
!cpu has unpack && !htl specified
=> works for a non-secure guest, as expected
=> guest will fail if it attempts to go secure
cpu has unpack && !htl specified
=> works as expected for a non-secure guest (unpack feature is
present, but unused)
=> secure guest may work "by accident", but only if all virtio
properties have the right values, which is the user's
problem
That last case is kinda ugly, but I think it's tolerable.
> We don't want to run into issues where libvirt probes and gets
> host model X, but when using that probed model (automatically) for a
> guest domain, we suddenly cannot run X anymore.
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
