qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paul Mackerras
Subject: Re: [Qemu-devel] [PATCH] spapr: Add ibm, processor-storage-keys property to CPU DT node
Date: Thu, 24 Aug 2017 14:15:01 +1000
User-agent: Mutt/1.5.24 (2015-08-30)

On Thu, Aug 24, 2017 at 02:02:43PM +1000, David Gibson 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 <address@hidden>
> > > ---
> > > 
> > > 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?

Just that storage keys are an aspect of the "server" (i.e. HPT) MMU,
so it seemed to make sense to put it together with the other
information about the HPT MMU (segment sizes, page sizes, etc.).

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

OK.

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

Paul.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]