Re: [PATCH v2 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM if PV

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4068 bytes --]

On Wed, Jun 10, 2020 at 03:19:22PM +0200, Viktor Mihajlovski wrote:
> 
> 
> On 6/10/20 12:24 PM, David Hildenbrand wrote:
> > On 10.06.20 12:07, David Gibson wrote:
> > > On Wed, Jun 10, 2020 at 09:22:45AM +0200, David Hildenbrand wrote:
> > > > On 10.06.20 06:31, David Gibson wrote:
> > > > > On Tue, Jun 09, 2020 at 12:44:39PM -0400, Michael S. Tsirkin wrote:
> > > > > > On Tue, Jun 09, 2020 at 06:28:39PM +0200, Halil Pasic wrote:
> > > > > > > On Tue, 9 Jun 2020 17:47:47 +0200
> > > > > > > Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> > > > > > > 
> > > > > > > > On Tue, 9 Jun 2020 11:41:30 +0200
> > > > > > > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > > I don't know. Janosch could answer that, but he is on vacation. Adding
> > > > > > > > > Claudio maybe he can answer. My understanding is, that while it might
> > > > > > > > > be possible, it is ugly at best. The ability to do a transition is
> > > > > > > > > indicated by a CPU model feature. Indicating the feature to the guest
> > > > > > > > > and then failing the transition sounds wrong to me.
> > > > > > > > 
> > > > > > > > I agree. If the feature is advertised, then it has to work. I don't
> > > > > > > > think we even have an architected way to fail the transition for that
> > > > > > > > reason.
> > > > > > > > 
> > > > > > > > What __could__ be done is to prevent qemu from even starting if an
> > > > > > > > incompatible device is specified together with PV.
> > > > > > > 
> > > > > > > AFAIU, the "specified together with PV" is the problem here. Currently
> > > > > > > we don't "specify PV" but PV is just a capability that is managed by the
> > > > > > > CPU model (like so many other).
> > > > > > 
> > > > > > So if we want to keep it user friendly, there could be
> > > > > > protection property with values on/off/auto, and auto
> > > > > > would poke at host capability to figure out whether
> > > > > > it's supported.
> > > > > > 
> > > > > > Both virtio and CPU would inherit from that.
> > > > > 
> > > > > Right, that's what I have in mind for my 'host-trust-limitation'
> > > > > property (a generalized version of the existing 'memory-encryption'
> > > > > machine option).  My draft patches already set virtio properties
> > > > > accordingly, it should be possible to set (default) cpu properties as
> > > > > well.
> > > > 
> > > > No crazy CPU model hacks please (at least speaking for the s390x).
> > > 
> > > Uh... I'm not really sure what you have in mind here.
> > > 
> > 
> > Reading along I got the impression that we want to glue the availability
> > of CPU features to other QEMU cmdline parameters (besides the
> > accelerator). ("to set (default) cpu properties as well"). If we are
> > talking about other CPU properties not expressed as CPU features (e.g.,
> > -cpu X,Y=on ...), then there is no issue.
> > 
> 
> The reason that the capability to run in PV mode is expressed in the CPU
> model is that this capability *is* provided by the CPU in terms of
> available instructions. I wouldn't see a benefit in providing
> a meta-property that needs to be synced with the CPU model.
> 
> So, if something has to be concluded from the fact that a VM
> could run in PV mode, that decision should be derived from the
> CPU model.

The trouble is that that approach is inherently s390 specific, and I'm
hoping we can make the configuration at least somewhat common between
platforms.

It also seems a very nasty layering violation to me for changing cpu
properties to affect apparently unrelated devices (like virtio, for
example).  It's still a bit nasty doing it from a machine property,
but it seems more reasonable to me that a machine property could
affect things elsewhere in the.. well.. machine.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]