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

[David Gibson]

On Wed, Aug 23, 2017 at 08:14:32PM -0300, Thiago Jung Bauermann wrote:
> 
> Hello David,
> 
> Thank you for your input.
> 
> David Gibson <address@hidden> writes:
> 
> > 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>
> >
> > Regardless of anything else, this clearly won't go in until 2.11 opens.
> 
> Ok, not a problem.
> 
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -605,6 +605,80 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> >> *fdt, int offset,
> >>                            pcc->radix_page_info->count *
> >>                            sizeof(radix_AP_encodings[0]))));
> >>      }
> >> +
> >> +    if (spapr->storage_keys) {
> >> +        uint32_t val[2];
> >> +
> >> +        val[0] = cpu_to_be32(spapr->storage_keys);
> >> +        val[1] = spapr->insn_keys ? val[0] : 0;
> >> +
> >> +        _FDT(fdt_setprop(fdt, offset, "ibm,processor-storage-keys",
> >> +                         val, sizeof(val)));
> >> +    }
> >> +}
> >> +
> >> +#define SYSFS_PROT_KEYS_PATH "/sys/kernel/mm/protection_keys/"
> >> +#define SYSFS_USABLE_STORAGE_KEYS SYSFS_PROT_KEYS_PATH "usable_keys"
> >> +#define SYSFS_DISABLE_EXEC_KEYS SYSFS_PROT_KEYS_PATH 
> >> "disable_execute_supported"
> >> +
> >> +static void setup_storage_keys(CPUPPCState *env, sPAPRMachineState *spapr)
> >> +{
> >> +    if (!(env->mmu_model & POWERPC_MMU_AMR))
> >> +        return;
> >> +
> >> +    if (kvm_enabled()) {
> >> +        char buf[sizeof("false\n")];
> >> +        uint32_t keys;
> >> +        FILE *fd;
> >
> > For starters, reasonably complex kvm-only code like this should go
> > into target/ppc/kvm.c.
> 
> Ok, will move the code there.
> 
> > But, there's a more fundamental problem.  Automatically adjusting
> > guest visible parameters based on the host's capabilities is really
> > problematic: if you migrate to a host with different capabilities
> > what's usable suddenly changes, but the guest can't know.
> >
> > The usual way to deal with this is instead to have explicit machine
> > parameters to control the value, and check if those are possible on
> > the host.  It's then up to the user and/or management layer to set the
> > parameters suitable to allow migration between all machines that they
> > care about.
> 
> Good point, I hadn't thought of it. I will add the machine parameter
> then and send a v2. Can the default value of the parameter be what is
> supported by the host? 

That's probably ok, although it's not entirely unproblematic.  For
safety I think we'd also need to migrate the value and on the
destination check that the migrated value is supportable.  Presumably
it's ok if the new host supports more storage keys than the former
one, just not the other way around.

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

signature.asc
Description: PGP signature