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: 3169 bytes --]

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.

Ah!  Glad someone pointed that out.

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

Is there a particular reason to put it there rather than a new KVM capability?

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

Note that the same migration concerns still apply, so we'll still want
machine properties to control this in qemu, which are validated
against what the host can do.  Since current kernels are buggy, I
suggest we have these properties default to 0 - i.e. you need to
explicitly request storage keys on the command line if you want your
guest to use them.  Once fixed kernels are rolled out we can look at
changing that in a future machine type.

> We could either have two u16 fields for the number of keys for data
> and instruction, or we could have a u32 field for the number of keys
> and a separate bit in the flags field to indicate that instruction
> keys are supported.  Which would be preferable?

I'd prefer the separate numbers for I and D.  It's more flexible and
no harder to implement AFAICT.

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