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
