Re: [PATCH v3 0/9] Generalize memory encryption models

From: David Hildenbrand <david@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>,
	Cornelia Huck <cohuck@redhat.com>
Cc: qemu-devel@nongnu.org, brijesh.singh@amd.com, pair@us.ibm.com,
	pbonzini@redhat.com, dgilbert@redhat.com, frankja@linux.ibm.com,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	kvm@vger.kernel.org, qemu-ppc@nongnu.org, mst@redhat.com,
	mdroth@linux.vnet.ibm.com, Richard Henderson <rth@twiddle.net>,
	pasic@linux.ibm.com, Eduardo Habkost <ehabkost@redhat.com>,
	qemu-s390x@nongnu.org
Subject: Re: [PATCH v3 0/9] Generalize memory encryption models
Date: Thu, 25 Jun 2020 09:06:05 +0200	[thread overview]
Message-ID: <025fb54b-60b7-a58b-e3d7-1bbaad152c5c@redhat.com> (raw)
In-Reply-To: <20200625052518.GD172395@umbus.fritz.box>

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

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

-- 
Thanks,

David / dhildenb