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;
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
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
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
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
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.
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
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.
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
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.