Re: [Qemu-devel] [PATCH] spapr: Add ibm, processor-storage-keys property to CPU DT node

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

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

On Thu, Aug 24, 2017 at 11:11:22AM -0700, Ram Pai wrote:
> On Thu, Aug 24, 2017 at 12:54:48PM +1000, Paul Mackerras wrote:
> > On Mon, Aug 21, 2017 at 05:00:36PM -0300, Thiago Jung Bauermann wrote:
> > > LoPAPR says:
> > > 
> > >     “ibm,processor-storage-keys”
> > > 
> > >     property name indicating the number of virtual storage keys supported
> > >     by the processor described by this node.
> > > 
> > >     prop-encoded-array: Consists of two cells encoded as with encode-int.
> > >     The first cell represents the number of virtual storage keys supported
> > >     for data accesses while the second cell represents the number of
> > >     virtual storage keys supported for instruction accesses. The cell value
> > >     of zero indicates that no storage keys are supported for the access
> > >     type.
> > > 
> > > pHyp provides the property above but there's a bug in P8 firmware where the
> > > second cell is zero even though POWER8 supports instruction access keys.
> > > This bug will be fixed for P9.
> > > 
> > > Tested with KVM on POWER8 Firenze machine and with TCG on x86_64 machine.
> > > 
> > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> > > ---
> > > 
> > > The sysfs files are provided by this patch for Linux:
> > 
> > Those sysfs files relate to the kernel's support for userspace
> > processes using storage keys.  That is quite distinct from KVM's
> > support for guests using storage keys, so I think that using those
> > sysfs files to indicate what the guest can do is wrong.
> > 
> > In fact KVM allows guests to specify storage keys in the HPTE values
> > that they set, except that there is a bug (for which Ram Pai has
> > posted a patch) that means that KVM loses the top two bits of the key
> > number.
> > 
> > What I would suggest is that we use the 'pad' field in the struct
> > kvm_ppc_smmu_info to report the number of keys supported by KVM for
> > guest use.  That will be 0 in all current kernels, indicating that
> > keys are not supported, which is reasonable because of the bug.  I
> > will make sure the patch fixing the bug goes in first.
> 
> with the current kernels, even with the bug, KVM can still support 8
> keys. Should we say 8 instead of 0?  It will help enable keys on KVM
> earlier and give a jump start to its adaption by applications.

But the point isn't really what we *can* say, it's with what is
implicitly said by kernels that don't advertise anything
specifically.  Which is 0.

If we're going to change the kernel to put something else in smmu
info, we can also change it to fix the storage key stuff anyway, so we
can put the full value in there.

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