qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH] spapr: Add ibm, processor-storage-keys property t


From: David Gibson
Subject: Re: [Qemu-ppc] [PATCH] spapr: Add ibm, processor-storage-keys property to CPU DT node
Date: Tue, 22 Aug 2017 12:02:58 +1000
User-agent: Mutt/1.8.3 (2017-05-23)

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 <address@hidden>

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]