Hello David, Thank you for your input.
David Gibson <da...@gibson.dropbear.id.au> 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 <bauer...@linux.vnet.ibm.com> > > 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? -- Thiago Jung Bauermann IBM Linux Technology Center