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

[Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>]

Hello David,

Thank you for your input.

David Gibson <david@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 <bauerman@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