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

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

[Thiago Jung Bauermann]

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

The sysfs files are provided by this patch for Linux:

https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-August/162005.html

I realize that this patch can't be committed before the Linux one goes in,
but I'd appreciate feedback so that it will be ready by the time the kernel
side is accepted.

 hw/ppc/spapr.c         | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  6 ++++
 2 files changed, 82 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f7a19720dcdf..a665e4d830f7 100644
--- 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;
+
+        /*
+         * With KVM, we allow the guest to use the keys which the kernel tells
+         * us are available.
+         */
+
+        fd = fopen(SYSFS_USABLE_STORAGE_KEYS, "r");
+        if (!fd) {
+            error_report("%s: open %s failed", __func__,
+                         SYSFS_USABLE_STORAGE_KEYS);
+            return;
+        }
+
+        if (fscanf(fd, "%u", &keys) != 1) {
+            error_report("%s: error reading %s", __func__,
+                         SYSFS_USABLE_STORAGE_KEYS);
+            fclose(fd);
+            return;
+        }
+
+        fclose(fd);
+
+        /* Now find out whether the keys can be used for instruction access. */
+
+        fd = fopen(SYSFS_DISABLE_EXEC_KEYS, "r");
+        if (!fd) {
+            error_report("%s: open %s failed", __func__,
+                         SYSFS_USABLE_STORAGE_KEYS);
+            return;
+        }
+
+        if (!fread(buf, 1, sizeof(buf), fd)) {
+            error_report("%s: error reading %s", __func__,
+                         SYSFS_DISABLE_EXEC_KEYS);
+            fclose(fd);
+            return;
+        }
+
+        fclose(fd);
+
+        spapr->storage_keys = keys;
+        spapr->insn_keys = !strncmp(buf, "true\n", sizeof(buf));
+    } else {
+        /* Without KVM, all keys provided by the architecture are available. */
+        spapr->storage_keys = 32;
+
+        /* POWER7 doesn't support instruction access keys. */
+        spapr->insn_keys = POWERPC_MMU_VER(env->mmu_model) != POWERPC_MMU_VER_2_06;
+    }
 }
 
 static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
@@ -619,6 +693,8 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
     _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
     _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
 
+    setup_storage_keys(&POWERPC_CPU(first_cpu)->env, spapr);
+
     /*
      * We walk the CPUs in reverse order to ensure that CPU DT nodes
      * created by fdt_add_subnode() end up in the right order in FDT
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2a303a705c17..15af12010779 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -122,6 +122,12 @@ struct sPAPRMachineState {
      * occurs during the unplug process. */
     QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
 
+    /* Number of processor storage keys available to the guest. */
+    uint32_t storage_keys;
+
+    /* Whether storage keys can control instruction access. */
+    bool insn_keys;
+
     /*< public >*/
     char *kvm_type;
     MemoryHotplugState hotplug_memory;

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]

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.



> ---
> 
> The sysfs files are provided by this patch for Linux:
> 
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-August/162005.html
> 
> I realize that this patch can't be committed before the Linux one goes in,
> but I'd appreciate feedback so that it will be ready by the time the kernel
> side is accepted.
> 
>  hw/ppc/spapr.c         | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  6 ++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a19720dcdf..a665e4d830f7 100644
> --- 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.

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.


> +        /*
> +         * With KVM, we allow the guest to use the keys which the kernel tells
> +         * us are available.
> +         */
> +
> +        fd = fopen(SYSFS_USABLE_STORAGE_KEYS, "r");
> +        if (!fd) {
> +            error_report("%s: open %s failed", __func__,
> +                         SYSFS_USABLE_STORAGE_KEYS);
> +            return;
> +        }
> +
> +        if (fscanf(fd, "%u", &keys) != 1) {
> +            error_report("%s: error reading %s", __func__,
> +                         SYSFS_USABLE_STORAGE_KEYS);
> +            fclose(fd);
> +            return;
> +        }
> +
> +        fclose(fd);
> +
> +        /* Now find out whether the keys can be used for instruction access. */
> +
> +        fd = fopen(SYSFS_DISABLE_EXEC_KEYS, "r");
> +        if (!fd) {
> +            error_report("%s: open %s failed", __func__,
> +                         SYSFS_USABLE_STORAGE_KEYS);
> +            return;
> +        }
> +
> +        if (!fread(buf, 1, sizeof(buf), fd)) {
> +            error_report("%s: error reading %s", __func__,
> +                         SYSFS_DISABLE_EXEC_KEYS);
> +            fclose(fd);
> +            return;
> +        }
> +
> +        fclose(fd);
> +
> +        spapr->storage_keys = keys;
> +        spapr->insn_keys = !strncmp(buf, "true\n", sizeof(buf));
> +    } else {
> +        /* Without KVM, all keys provided by the architecture are available. */
> +        spapr->storage_keys = 32;
> +
> +        /* POWER7 doesn't support instruction access keys. */
> +        spapr->insn_keys = POWERPC_MMU_VER(env->mmu_model) != POWERPC_MMU_VER_2_06;
> +    }
>  }
>  
>  static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> @@ -619,6 +693,8 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
>      _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
>      _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
>  
> +    setup_storage_keys(&POWERPC_CPU(first_cpu)->env, spapr);
> +
>      /*
>       * We walk the CPUs in reverse order to ensure that CPU DT nodes
>       * created by fdt_add_subnode() end up in the right order in FDT
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2a303a705c17..15af12010779 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -122,6 +122,12 @@ struct sPAPRMachineState {
>       * occurs during the unplug process. */
>      QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
>  
> +    /* Number of processor storage keys available to the guest. */
> +    uint32_t storage_keys;
> +
> +    /* Whether storage keys can control instruction access. */
> +    bool insn_keys;
> +
>      /*< public >*/
>      char *kvm_type;
>      MemoryHotplugState hotplug_memory;
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thiago Jung Bauermann]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4003 bytes --]

On Wed, Aug 23, 2017 at 08:14:32PM -0300, Thiago Jung Bauermann wrote:
> 
> 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? 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170821200036.15036-1-bauerman@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH] spapr: Add ibm, processor-storage-keys property to CPU DT node

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
72241ec20e spapr: Add ibm, processor-storage-keys property to CPU DT node

=== OUTPUT BEGIN ===
Checking PATCH 1/1: spapr: Add ibm, processor-storage-keys property to CPU DT node...
ERROR: braces {} are necessary for all arms of this statement
#59: FILE: hw/ppc/spapr.c:626:
+    if (!(env->mmu_model & POWERPC_MMU_AMR))
[...]

WARNING: line over 80 characters
#113: FILE: hw/ppc/spapr.c:680:
+        spapr->insn_keys = POWERPC_MMU_VER(env->mmu_model) != POWERPC_MMU_VER_2_06;

total: 1 errors, 1 warnings, 100 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Paul Mackerras]

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

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

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?

Paul.

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

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 <bauerman@linux.vnet.ibm.com>
> > ---
> > 
> > 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?

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paul Mackerras]

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 <bauerman@linux.vnet.ibm.com>
> > > ---
> > > 
> > > 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.

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

On Thu, Aug 24, 2017 at 02:15:01PM +1000, Paul Mackerras wrote:
> 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 <bauerman@linux.vnet.ibm.com>
> > > > ---
> > > > 
> > > > 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.).

Ok, works for me.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ram Pai]

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 <bauerman@linux.vnet.ibm.com>
> > ---
> > 
> > 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.
> 
> 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.  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.

with the current kernels, even with the bug, KVM can still support 8
keys. Should we say 8 instead of 0?  It will help enable keys on KVM
earlier and give a jump start to its adaption by applications.

RP

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]

On Thu, Aug 24, 2017 at 11:11:22AM -0700, Ram Pai 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 <bauerman@linux.vnet.ibm.com>
> > > ---
> > > 
> > > 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.
> > 
> > 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.  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.
> 
> with the current kernels, even with the bug, KVM can still support 8
> keys. Should we say 8 instead of 0?  It will help enable keys on KVM
> earlier and give a jump start to its adaption by applications.

But the point isn't really what we *can* say, it's with what is
implicitly said by kernels that don't advertise anything
specifically.  Which is 0.

If we're going to change the kernel to put something else in smmu
info, we can also change it to fix the storage key stuff anyway, so we
can put the full value in there.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ram Pai]

On Fri, Aug 25, 2017 at 02:23:13PM +1000, David Gibson wrote:
> On Thu, Aug 24, 2017 at 11:11:22AM -0700, Ram Pai 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 <bauerman@linux.vnet.ibm.com>
> > > > ---
> > > > 
> > > > 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.
> > > 
> > > 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.  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.
> > 
> > with the current kernels, even with the bug, KVM can still support 8
> > keys. Should we say 8 instead of 0?  It will help enable keys on KVM
> > earlier and give a jump start to its adaption by applications.
> 
> But the point isn't really what we *can* say, it's with what is
> implicitly said by kernels that don't advertise anything
> specifically.  Which is 0.
> 

Right. I was pointing to the case where nothing is said implicitly by
the kernel (no advertised values), to default to 8 instead of defaulting
to 0.

> If we're going to change the kernel to put something else in smmu
> info, we can also change it to fix the storage key stuff anyway, so we
> can put the full value in there.

right. 

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



-- 
Ram Pai

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

On Mon, Aug 28, 2017 at 10:50:11AM -0700, Ram Pai wrote:
> On Fri, Aug 25, 2017 at 02:23:13PM +1000, David Gibson wrote:
> > On Thu, Aug 24, 2017 at 11:11:22AM -0700, Ram Pai 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 <bauerman@linux.vnet.ibm.com>
> > > > > ---
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.  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.
> > > 
> > > with the current kernels, even with the bug, KVM can still support 8
> > > keys. Should we say 8 instead of 0?  It will help enable keys on KVM
> > > earlier and give a jump start to its adaption by applications.
> > 
> > But the point isn't really what we *can* say, it's with what is
> > implicitly said by kernels that don't advertise anything
> > specifically.  Which is 0.
> > 
> 
> Right. I was pointing to the case where nothing is said implicitly by
> the kernel (no advertised values), to default to 8 instead of defaulting
> to 0.

The point is that the kernel advertising nothing is indistinguishable
from it advertising 0.  We don't get to chose that default value.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ram Pai]

On Thu, Aug 24, 2017 at 12:54:48PM +1000, Paul Mackerras wrote:
> 
> 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?

the second choice is more confusion-proof; to me atleast.

The first choice gives a illusion that there are 'x' number of data keys
and 'y' number of instruction keys; which is not exactly true.

RP

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Mon, Aug 28, 2017 at 10:53:56AM -0700, Ram Pai wrote:
> On Thu, Aug 24, 2017 at 12:54:48PM +1000, Paul Mackerras wrote:
> > 
> > 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?
> 
> the second choice is more confusion-proof; to me atleast.
> 
> The first choice gives a illusion that there are 'x' number of data keys
> and 'y' number of instruction keys; which is not exactly true.

Ah.. can you elaborate?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ram Pai]

On Tue, Aug 29, 2017 at 11:40:30AM +1000, David Gibson wrote:
> On Mon, Aug 28, 2017 at 10:53:56AM -0700, Ram Pai wrote:
> > On Thu, Aug 24, 2017 at 12:54:48PM +1000, Paul Mackerras wrote:
> > > 
> > > 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?
> > 
> > the second choice is more confusion-proof; to me atleast.
> > 
> > The first choice gives a illusion that there are 'x' number of data keys
> > and 'y' number of instruction keys; which is not exactly true.
> 
> Ah.. can you elaborate?

On power8 and power9, there are only 32 keys, each key can be configured to
disable data-access and instruction-access.  The first choice, will
report 32 keys for data-access and 32 keys for instruction-access. To a
casual on-looker it gives an impresssion that there are 32 keys for
data-access and 32 keys for instruction-access; 64 keys in total. And
that is what I think can be the cause for confusion.



RP

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

On Tue, Aug 29, 2017 at 09:31:07AM -0700, Ram Pai wrote:
> On Tue, Aug 29, 2017 at 11:40:30AM +1000, David Gibson wrote:
> > On Mon, Aug 28, 2017 at 10:53:56AM -0700, Ram Pai wrote:
> > > On Thu, Aug 24, 2017 at 12:54:48PM +1000, Paul Mackerras wrote:
> > > > 
> > > > 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?
> > > 
> > > the second choice is more confusion-proof; to me atleast.
> > > 
> > > The first choice gives a illusion that there are 'x' number of data keys
> > > and 'y' number of instruction keys; which is not exactly true.
> > 
> > Ah.. can you elaborate?
> 
> On power8 and power9, there are only 32 keys, each key can be configured to
> disable data-access and instruction-access.  The first choice, will
> report 32 keys for data-access and 32 keys for instruction-access. To a
> casual on-looker it gives an impresssion that there are 32 keys for
> data-access and 32 keys for instruction-access; 64 keys in total. And
> that is what I think can be the cause for confusion.

Ah, I see.

Paul, sorry, I hadn't realized the above when I said I preferred
separate values for data and instr keys.  In view of the above, I
change my preference to a single # of keys and a flag that they can be
used for instructions.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]