[Skiboot] [RFC PATCH v3 2/6] Add ultravisor support in OPAL
Oliver O'Halloran
oohall at gmail.com
Wed Jan 29 01:24:34 AEDT 2020
On Wed, 2020-01-22 at 10:13 -0500, Ryan Grimm wrote:
> Ultravisor is the firmware which runs in the new priveleged mode called
> ultravisor mode, which was introduced in Power 9. Ultravisor enables
> running of secure virtual machines on the host.
>
> Protected execution facility in Power 9 uses special memory areas
> designated as secure memory, which can be accessed only in the
> ultravisor mode. This protection is provided by the hardware. These
> designated memory areas are used by the guests running as secure virtual
> machines.
>
> The secure memory ranges are provided by the hostboot through HDAT.
> Skiboot parses HDAT and creates secure-memory@ device tree nodes.
>
> Ultravisor firmware is present as a compressed lid file or as 'UVISOR'
> partition. Skiboot uses the resource load helper and xz decompress to
> load and decompress the ultravisor firmware into secure memory on chip
> 0.
>
> The ultravisor image is started on each CPU and the CPUs return with MSR
> S bit off.
> For Mambo, ultra.tcl creates the secure-memory-ranges property at 8GB.
> Mambo has no protection on secure memory, so a watchpoint should be used
> to ensure Skiboot does not touch secure memory. ultra.tcl creates the
> ibm,secure-mem reserve.
Is ultra.tcl supposed to be included in this patch?
> For BML, the BML script parses secure memory from the Cronus config file
> and creates the secure-memory-ranges proprty.
Is this still true? Looks like secure-memory-ranges went away with the
switch to secure-memory@<addr> nodes.
>
> A new type of system call called the ultra call is used to get the
> services of the ultravisor. This ultracall support is needed in skiboot
> to access the xscoms which are in the secure memory area.
Move the ultra-call stuff into 4/6 where it's actually used.
> For more details, see doc/opal-uv-api.rst.
>
> Signed-off-by: Ryan Grimm <grimm at linux.ibm.com>
> [ maddy: Initial implementation]
> [ maddy: ultra-call support for skiboot ]
> Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
> [ santosh: Initial implementation]
> [ santosh: ultra-call support for skiboot ]
> Signed-off-by: Santosh Sivaraj <santosh at fossix.org>
> [ andmike: Split init and start of ultravisor ]
> [ andmike: ABI change to switch from r0 to r3 ]
> [ andmike: Remove stb header prior to decompress ]
> [ andmike: Pickup wrapkey data from mambo ]
> Signed-off-by: Michael Anderson <andmike at linux.ibm.com>
> [ cclaudio: Fix compilation for GCC9 ]
> Signed-off-by: Claudio Carvalho <cclaudio at linux.ibm.com>
> [ linuxram: populate the UV FDT after the password is generated ]
> Signed-off-by: Ram Pai <linuxram at us.ibm.com>
> ---
> asm/head.S | 54 +++++
> core/flash.c | 1 +
> core/init.c | 11 +
> core/mem_region.c | 32 +++
> hdata/memory.c | 23 ++-
> hdata/test/hdata_to_dt.c | 1 +
> hw/Makefile.inc | 1 +
> hw/fsp/fsp.c | 2 +
> hw/ultravisor.c | 395 ++++++++++++++++++++++++++++++++++++
> include/mem_region-malloc.h | 3 +
> include/platform.h | 1 +
> include/processor.h | 12 ++
> include/ultravisor-api.h | 19 ++
> include/ultravisor.h | 27 +++
> 14 files changed, 576 insertions(+), 6 deletions(-)
Too big. At the very least the HDAT parsing and the ucall support can
go into seperate patches. The memory region changes probably be in a
seperate patch too.
> create mode 100644 hw/ultravisor.c
> create mode 100644 include/ultravisor-api.h
> create mode 100644 include/ultravisor.h
>
> diff --git a/asm/head.S b/asm/head.S
> index b565f6c9..49a62ce5 100644
> --- a/asm/head.S
> +++ b/asm/head.S
> @@ -1039,3 +1039,57 @@ start_kernel_secondary:
> mtspr SPR_HSRR1,%r10
> mfspr %r3,SPR_PIR
> hrfid
> +
> +/* start_uv register usage:
> + *
> + * r3 is base address of UV
> + * r4 is ptr to struct uv_opal
> + */
> +.global start_uv
> +start_uv:
> + mflr %r0
> + std %r0,16(%r1)
> + sync
> + icbi 0,%r3
> + sync
> + isync
> + mtctr %r3
> + mr %r3,%r4
> + LOAD_IMM64(%r8,SKIBOOT_BASE);
> + LOAD_IMM32(%r10, opal_entry - __head)
> + add %r9,%r8,%r10
> + LOAD_IMM32(%r6, EPAPR_MAGIC)
> + addi %r7,%r5,1
> + li %r4,0
> + li %r5,0
> + bctrl /* branch to UV here */
> + ld %r0,16(%r1)
> + mtlr %r0
> + blr
> +
> +.global ucall
> +ucall:
> + mflr %r0
> + stdu %r1,-STACK_FRAMESIZE(%r1)
> + std %r0,STACK_LR(%r1)
> + mfcr %r0
> + stw %r0,STACK_CR(%r1)
> + std %r4,STACK_GPR4(%r1) /* Save ret buffer */
> + mr %r4,%r5
> + mr %r5,%r6
> + mr %r6,%r7
> + mr %r7,%r8
> + mr %r8,%r9
> + mr %r9,%r10
> + sc 2 /* invoke the ultravisor */
> + ld %r12,STACK_GPR4(%r1)
> + std %r4, 0(%r12)
> + std %r5, 8(%r12)
> + std %r6, 16(%r12)
> + std %r7, 24(%r12)
> + lwz %r0,STACK_CR(%r1)
> + mtcrf 0xff,%r0
> + ld %r0,STACK_LR(%r1)
> + mtlr %r0
> + addi %r1,%r1,STACK_FRAMESIZE
> + blr /* return r3 = status */
put this stuff in asm/misc.S unless there's a compelling reason it
needs to be in head.S
> diff --git a/core/flash.c b/core/flash.c
> index de748641..247aec5c 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -45,6 +45,7 @@ static struct {
> { RESOURCE_ID_INITRAMFS,RESOURCE_SUBID_NONE, "ROOTFS" },
> { RESOURCE_ID_CAPP, RESOURCE_SUBID_SUPPORTED, "CAPP" },
> { RESOURCE_ID_IMA_CATALOG, RESOURCE_SUBID_SUPPORTED, "IMA_CATALOG" },
> + { RESOURCE_ID_UV_IMAGE, RESOURCE_SUBID_NONE, "UVISOR" },
> { RESOURCE_ID_VERSION, RESOURCE_SUBID_NONE, "VERSION" },
> { RESOURCE_ID_KERNEL_FW, RESOURCE_SUBID_NONE, "BOOTKERNFW" },
> };
> diff --git a/core/init.c b/core/init.c
> index 339462e5..4160e68d 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -46,6 +46,7 @@
> #include <debug_descriptor.h>
> #include <occ.h>
> #include <opal-dump.h>
> +#include <ultravisor.h>
>
> enum proc_gen proc_gen;
> unsigned int pcie_max_link_speed;
> @@ -558,6 +559,8 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
>
> trustedboot_exit_boot_services();
>
> + start_ultravisor();
> +
> ipmi_set_fw_progress_sensor(IPMI_FW_OS_BOOT);
>
>
> @@ -1296,6 +1299,11 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
> pci_nvram_init();
>
> preload_capp_ucode();
> +
> + /* preload and decompress ultravisor image */
> + uv_preload_image();
> + uv_decompress_image();
What's the point of pre-loading it if we're just going to wait around
until it's loaded so that we can start decompressing it?
> +
> start_preload_kernel();
>
> /* Catalog decompression routine */
> @@ -1354,6 +1362,9 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
> /* Add the list of interrupts going to OPAL */
> add_opal_interrupts();
>
> + /* Init ultravisor software */
> + init_uv();
> +
> /* Now release parts of memory nodes we haven't used ourselves... */
> mem_region_release_unused();
>
> diff --git a/core/mem_region.c b/core/mem_region.c
> index eb48a1a1..91bfa1f1 100644
> --- a/core/mem_region.c
> +++ b/core/mem_region.c
> @@ -851,6 +851,38 @@ static bool matches_chip_id(const __be32 ids[], size_t num, u32 chip_id)
> return false;
> }
>
> +void __local_free(void *p, const char *location)
> +{
> + struct mem_region *region;
> + struct alloc_hdr *hdr;
> +
> + if (!p)
> + return;
> +
> + lock(&mem_region_lock);
> +
> + list_for_each(®ions, region, list) {
> + /* localr_alloc doesn't use heap */
> + if (region == &skiboot_heap)
> + continue;
> +
> + if (p >= region_start(region) &&
> + (p < (region_start(region) + region->len))) {
> + hdr = p - sizeof(*hdr);
> +
> + if (hdr->free)
> + bad_header(region, hdr, "re-freed", location);
> +
> + lock(®ion->free_list_lock);
> + make_free(region, (struct free_hdr *)hdr, location, false);
> + unlock(®ion->free_list_lock);
> + }
> +
> + }
> +
> + unlock(&mem_region_lock);
> +}
> +
> void *__local_alloc(unsigned int chip_id, size_t size, size_t align,
> const char *location)
> {
Crack this into a seperate patch and add a unit test in core/test/run-
mem_region.c
> diff --git a/hdata/memory.c b/hdata/memory.c
> index 9c588ff6..99d54298 100644
> --- a/hdata/memory.c
> +++ b/hdata/memory.c
> @@ -32,7 +32,7 @@ struct HDIF_ms_area_address_range {
> __be64 start;
> __be64 end;
> __be32 chip;
> - __be32 mirror_attr;
> + __be32 memory_attr;
> __be64 mirror_start;
> __be32 controller_id;
> __be32 phys_attr;
> @@ -59,6 +59,9 @@ struct HDIF_ms_area_address_range {
> #define MS_CONTROLLER_MCS_ID(id) GETFIELD(PPC_BITMASK32(4, 7), id)
> #define MS_CONTROLLER_MCA_ID(id) GETFIELD(PPC_BITMASK32(8, 15), id)
>
> +#define MS_ATTR_PEF (PPC_BIT32(23))
> +#define UV_SECURE_MEM_BIT (PPC_BIT(15))
doesn't need parens
> +
> struct HDIF_ms_area_id {
> __be16 id;
> #define MS_PTYPE_RISER_CARD 0x8000
> @@ -128,11 +131,11 @@ static bool add_address_range(struct dt_node *root,
>
> chip_id = pcid_to_chip_id(be32_to_cpu(arange->chip));
>
> - prlog(PR_DEBUG, " Range: 0x%016llx..0x%016llx "
> - "on Chip 0x%x mattr: 0x%x pattr: 0x%x status:0x%x\n",
> + prlog(PR_INFO, " Range: 0x%016llx..0x%016llx "
> + "on Chip 0x%x memattr: 0x%08x pattr: 0x%x status:0x%x\n",
> (long long)be64_to_cpu(arange->start),
> (long long)be64_to_cpu(arange->end),
> - chip_id, be32_to_cpu(arange->mirror_attr),
> + chip_id, be32_to_cpu(arange->memory_attr),
> mem_type, mem_status);
>
> /* reg contains start and length */
> @@ -161,6 +164,13 @@ static bool add_address_range(struct dt_node *root,
> return false;
> }
>
> + if (be32_to_cpu(arange->memory_attr) & MS_ATTR_PEF) {
> + prlog(PR_DEBUG, "HDAT: Found secure memory\n");
> + name = "secure-memory";
> + dev_type = "secure-memory";
> + compat = "ibm,secure-memory";
> + }
> +
> if (be16_to_cpu(id->flags) & MS_AREA_SHARED) {
> mem = dt_find_by_name_addr(dt_root, name, reg[0]);
> if (mem) {
> @@ -674,9 +684,10 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>
> /*
> * Workaround broken HDAT reserve regions which are
> - * bigger than 512MB
> + * bigger than 512MB and not secure memory
> */
> - if ((end_addr - start_addr) > 0x20000000) {
> + if (((end_addr - start_addr) > 0x20000000) &&
> + !(start_addr & UV_SECURE_MEM_BIT)) {
> prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve: too big\n");
> continue;
> }
I think you can drop this hunk since secure memory isn't in the
reserved memory ranges any more. The definition of UV_SECURE_MEM_BIT in
hdata/memory.c can go too since it's only used here.
> diff --git a/hdata/test/hdata_to_dt.c b/hdata/test/hdata_to_dt.c
> index 11b7a3ac..d487e272 100644
> --- a/hdata/test/hdata_to_dt.c
> +++ b/hdata/test/hdata_to_dt.c
> @@ -64,6 +64,7 @@ unsigned long tb_hz = 512000000;
> #define PVR_P8NVL 0x004c0100
> #define PVR_P9 0x004e0200
> #define PVR_P9P 0x004f0100
> +#define MSR_S PPC_BIT(41) /* Secure Mode enable */
>
> #define SPR_PVR 0x11f /* RO: Processor version register */
>
> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
> index b708bdfe..848898b9 100644
> --- a/hw/Makefile.inc
> +++ b/hw/Makefile.inc
> @@ -9,6 +9,7 @@ HW_OBJS += fake-nvram.o lpc-mbox.o npu2.o npu2-hw-procedures.o
> HW_OBJS += npu2-common.o npu2-opencapi.o phys-map.o sbe-p9.o capp.o
> HW_OBJS += occ-sensor.o vas.o sbe-p8.o dio-p9.o lpc-port80h.o cache-p9.o
> HW_OBJS += npu-opal.o npu3.o npu3-nvlink.o npu3-hw-procedures.o
> +HW_OBJS += ultravisor.o
> HW=hw/built-in.a
>
> include $(SRC)/hw/fsp/Makefile.inc
> diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c
> index 7592ee07..0411f035 100644
> --- a/hw/fsp/fsp.c
> +++ b/hw/fsp/fsp.c
> @@ -114,6 +114,7 @@ static u64 fsp_hir_timeout;
> #define KERNEL_LID_PHYP 0x80a00701
> #define KERNEL_LID_OPAL 0x80f00101
> #define INITRAMFS_LID_OPAL 0x80f00102
> +#define ULTRA_LID_OPAL 0x80f00105
>
> /*
> * We keep track on last logged values for some things to print only on
> @@ -2372,6 +2373,7 @@ static struct {
> } fsp_lid_map[] = {
> { RESOURCE_ID_KERNEL, RESOURCE_SUBID_NONE, KERNEL_LID_OPAL },
> { RESOURCE_ID_INITRAMFS,RESOURCE_SUBID_NONE, INITRAMFS_LID_OPAL },
> + { RESOURCE_ID_UV_IMAGE, RESOURCE_SUBID_NONE, ULTRA_LID_OPAL },
> { RESOURCE_ID_IMA_CATALOG,IMA_CATALOG_NIMBUS, 0x80f00103 },
> { RESOURCE_ID_CAPP, CAPP_IDX_MURANO_DD20, 0x80a02002 },
> { RESOURCE_ID_CAPP, CAPP_IDX_MURANO_DD21, 0x80a02001 },
> diff --git a/hw/ultravisor.c b/hw/ultravisor.c
> new file mode 100644
> index 00000000..4222f212
> --- /dev/null
> +++ b/hw/ultravisor.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: Apache-2.0
> +/* Copyright 2018-2019 IBM Corp. */
> +
> +#include <skiboot.h>
> +#include <xscom.h>
> +#include <chip.h>
> +#include <device.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <inttypes.h>
> +#include <ultravisor.h>
> +#include <mem_region.h>
> +#include <ultravisor-api.h>
> +#include <libfdt/libfdt.h>
> +#include <libstb/container.h>
> +
> +static char *uv_image = NULL;
> +static size_t uv_image_size;
> +struct xz_decompress *uv_xz = NULL;
> +static struct uv_opal *uv_opal;
> +
> +const char * wrap_key_prop_str[] = {
no space between the * and the variable name.
> + "wrapping-key-passwd",
> + "wrapping-key-publicname",
> + "wrapping-key-policy-a",
> + "wrapping-key-policy-b",
> + NULL
> +};
> +
> +static void create_uv_fw_node(struct dt_node *uv_node, uint64_t addr, uint32_t size) {
brace goes on a new line.
> + struct dt_node *uv_fw_node;
> + char fw_name[64];
> +
> + snprintf(fw_name, 64, "firmware@%llx", addr);
> + uv_fw_node = dt_new_check(uv_node, fw_name);
> + dt_add_property_string(uv_fw_node, "compatible", "ibm,uv-firmware");
> + dt_add_property_cells(uv_fw_node, "reg", addr >> 32, addr & 0xfffffff, size);
Hmm, in the event the uv_fw_node already exists this is probably going
to blow up on since the properties added below will already exist.
Also, the having #address-cells = 2 and #size-cells = 1 makes it
awkward to work with since u64 helpers. There's no point in saving the
extra four bytes so bump #size-cells to 2 and use
dt_add_property_u64s()
> +}
> +
> +static uint64_t find_secure_mem(void)
> +{
> + struct dt_node *node;
> + const struct dt_property *reg;
> + uint64_t secure_mem;
declarations should be in reverse christmas tree order
> + dt_for_each_compatible(dt_root, node, "ibm,secure-memory")
> + if (dt_get_chip_id(node) == 0)
> + break;
> +
> + reg = dt_find_property(node, "reg");
> + secure_mem = dt_get_number(reg->prop, 2);
Use dt_get_address()
> + return secure_mem;
> +}
> +
> +static struct dt_node *add_uv_dt_node(void)
> +{
> + struct dt_node *uv_node;
> + uint64_t uv_fw_start;
> +
> + uv_node = dt_new_check(dt_root, "ibm,ultravisor");
> + dt_add_property_string(uv_node, "compatible", "ibm,ultravisor");
> + dt_add_property_cells(uv_node, "#address-cells", 2);
> + dt_add_property_cells(uv_node, "#size-cells", 1);
as mentioned above, make it 2
> + uv_fw_start = find_secure_mem();
> + create_uv_fw_node(uv_node, uv_fw_start, UV_LOAD_MAX_SIZE);
> + return uv_node;
> +}
> +
> +static struct dt_node *find_tpm_sim_node(void)
> +{
> + struct dt_node *tpm_sim_node;
> +
> + tpm_sim_node = dt_find_compatible_node(dt_root, NULL, "uv,tpm_sim");
> + if (!tpm_sim_node) {
> + prlog(PR_INFO, "uv,tpm_sim compatible node not found\n");
> + return NULL;
> + }
> +
> + return tpm_sim_node;
> +}
Fold this into the one place it's called from.
> +
> +/* create ibm,ultravisor node if it does not exist. Mambo and BML pass it in.
> + * For HB, hdata/memory.c code parses HDAT and creates secure-memory nodes and
> + * find_secure_mem will return the reg property */
/*
* Use this style for block comments.
*/
> +static struct dt_node *get_uv_fw_node(void)
> +{
> + struct dt_node *uv_fw_node;
> +
> + uv_fw_node = dt_find_compatible_node(dt_root, NULL, "ibm,uv-firmware");
> + if (!uv_fw_node) {
> + prlog(PR_DEBUG, "UV: ibm,uv-firmware compatible node not found, creating\n");
> + add_uv_dt_node();
> + uv_fw_node = dt_find_compatible_node(dt_root, NULL, "ibm,uv-firmware");
> + }
> +
> + return uv_fw_node;
> +}
> +static bool get_uv_fw_region(uint64_t *addr, uint32_t *size)
> +{
> + struct dt_node *node;
> + const struct dt_property *reg;
> +
> + node = get_uv_fw_node();
> +
> + reg = dt_find_property(node, "reg");
> + *addr = dt_get_number(reg->prop, 2);
> + *size = dt_get_number(reg->prop + sizeof(u64), 1);
dt_get_address()
> +
> + return true;
> +}
> +
> +static void update_uv_fw_node(uint64_t addr, uint32_t size)
> +{
> + struct dt_node *uv_node, *uv_fw_node;
> +
> + if (!(uv_fw_node = dt_find_compatible_node(dt_root, NULL, "ibm,uv-firmware"))) {
Don't do assignments inside a conditional. It usually winds up looking
awkward, especially if it's split across multiple lines, and I think
this one should be. Seperating the assignment and test usually ends up
looking cleaner anyway:
uv_fw_node = dt_find_compatible_node(dt_root, NULL, "ibm,uv-firmware");
if (!uv_fw_node) {
/* blah */
}
It's even 80 cols compliant.
> + prerror("UV: No ibm,uv-firmware node found\n");
> + return;
> + }
> +
> + uv_node = uv_fw_node->parent;
> + dt_free(uv_fw_node);
> + create_uv_fw_node(uv_node, addr, size);
If you find yourself hacking the DT at runtime it's usually a sign that
you need to rethink what you're doing. The parts of the DT that are
provided to skiboot in the input DT (or HDAT) should be treated as
read-only by the rest of skiboot since a) We don't do any refcounting
of DT nodes, so we have no idea what else is using it. And b) working
out where a DT node / property is generated is relatively
straightforward if there is a single producer. It pretty rapidly
becomes a PATA when you start having to think about what random hacks
are applied along the way so I strongly prefer that we don't.
We have done some runtime modifications to the DT in the past, but
that's usually because we need to work around bugs in an input DT that
we don't have control over (i.e P8 hostboot). That reasoning doesn't
really apply since we've got full control over how the input DT is
generated (BML or skiboot.tcl).
Further comments below where this is called.
> +static int add_wrapping_key_mambo(void *fdt)
I'd rather mambo specific code went into platforms/mambo/*.c
> +{
> + struct dt_node *tpm_sim_node;
> + const struct dt_property *property = NULL;
> +
> + if ((tpm_sim_node = find_tpm_sim_node())) {
> + int i;
> +
> + fdt_begin_node(fdt, "ibm,uv-tpm");
> + fdt_property_string(fdt, "compatible", "ibm,uv-tpm");
> +
> + for (i = 0; wrap_key_prop_str[i] != NULL; i++) {
> + property = dt_find_property(tpm_sim_node,
> + wrap_key_prop_str[i]);
> + if (property) {
> + fdt_property(fdt, wrap_key_prop_str[i],
> + property->prop,
> + property->len);
> + }
> + }
Move the definition of wrap_key_prop_str[] to above this function or
make it a local variable.
> +
> + fdt_end_node(fdt);
> + }
> +
> + return 0;
> +}
> +
> +static int fdt_add_wrapping_key(void *fdt)
> +{
> + return add_wrapping_key_mambo(fdt);
what's this supposed to be doing?
> +}
> +
> +static int create_dtb_uv(void *uv_fdt)
> +{
> + if (fdt_create(uv_fdt, UV_FDT_MAX_SIZE)) {
> + prerror("UV: Failed to create uv_fdt\n");
> + return 1;
> + }
> +
> + fdt_finish_reservemap(uv_fdt);
> + fdt_begin_node(uv_fdt, "");
> + fdt_property_string(uv_fdt, "description", "Ultravisor fdt");
> + fdt_begin_node(uv_fdt, "ibm,uv-fdt");
> + fdt_property_string(uv_fdt, "compatible", "ibm,uv-fdt");
> + if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS) {
> + if (add_wrapping_key_mambo(uv_fdt))
> + prlog(PR_ERR, "Failed to add the mambo wrapping key"
> + " to dt\n");
> + } else
> + if (fdt_add_wrapping_key(uv_fdt))
> + prlog(PR_ERR, "Failed to add the wrapping key"
> + " to dt\n");
Don't split prlog()s, use prerror(), and if any arm of an if-else block
has braces then every arm should.
> + fdt_end_node(uv_fdt);
> + fdt_end_node(uv_fdt);
> + fdt_finish(uv_fdt);
> +
> + return OPAL_SUCCESS;
> +}
> +
> +static void cpu_start_ultravisor(void *data)
> +{
> + struct uv_opal *ptr = (struct uv_opal *)data;
don't cast void pointers.
> + start_uv(ptr->uv_base_addr, ptr);
> +}
> +
> +int start_ultravisor(void)
> +{
> + struct cpu_thread *cpu;
> + struct cpu_job **jobs;
> + uint64_t uv_fdt_addr;
> + uint64_t addr;
> + uint32_t size;
> + struct dt_node *uv_fw_node;
> + int i = 0;
> +
> + /* init_uv should have made the ibm,ultravisor node by now so don't
> + * start if something went wrong */
fix the comment style
> + if (!dt_find_compatible_node(dt_root, NULL, "ibm,ultravisor")) {
> + prlog(PR_DEBUG, "UV: No ibm,ultravisor found, won't start ultravisor\n");
> + return OPAL_HARDWARE;
> + }
> +
> + get_uv_fw_region(&addr, &size);
> +
> + uv_fdt_addr = addr + UV_LOAD_MAX_SIZE;
> +
> + if (create_dtb_uv((void *)uv_fdt_addr))
> + return OPAL_NO_MEM;
> +
> + uv_fw_node = get_uv_fw_node();
> +
> + dt_add_property_u64s(uv_fw_node, "uv-fdt", uv_fdt_addr);
> +
> + uv_opal->uv_fdt = (__be64)uv_fdt_addr;
> +
> + prlog(PR_NOTICE, "UV: Starting Ultravisor at 0x%llx sys_fdt 0x%llx uv_fdt 0x%0llx\n",
> + addr, uv_opal->sys_fdt, uv_opal->uv_fdt);
> +
> + if (!addr || !uv_opal->sys_fdt || !uv_opal->uv_fdt) {
> + prerror("UV: Bad ptrs, not starting\n");
> + return OPAL_INTERNAL_ERROR;
> + }
> +
> + jobs = zalloc(sizeof(struct cpu_job*) * cpu_max_pir);
space between cpu_job and *
> +
> + for_each_available_cpu(cpu) {
> + if (cpu == this_cpu())
> + continue;
> + jobs[i++] = cpu_queue_job(cpu, "start_ultravisor",
> + cpu_start_ultravisor, (void *)uv_opal);
> + }
> +
> + cpu_start_ultravisor((void *)uv_opal);
> +
> + while (i > 0)
> + cpu_wait_job(jobs[--i], true);
> +
> + free(jobs);
> +
> + return OPAL_SUCCESS;
> +}
> +
> +static bool alloc_uv(void)
> +{
> + struct proc_chip *chip = next_chip(NULL);
> +
> + uv_image_size = MAX_COMPRESSED_UV_IMAGE_SIZE;
Shouldn't this be probed based on the size of the UV partition?
> + if (!(uv_image = local_alloc(chip->id, uv_image_size, uv_image_size)))
> + return false;
> + memset(uv_image, 0, uv_image_size);
> + return true;
> +}
Is uv_image being freed anywhere?
> +
> +static int tpm_nv_init(void)
> +{
> + prlog(PR_INFO, "%s begin\n", __func__);
> + return 0;
> +}
> +
> +/* For HB, check for uv_xz != NULL and wait for decompress. UV should have
> + * been loaded from PNOR or FSP and decompressed earlier via uv_preload_image
> + * and uv_decompress_image. The secure location of the UV provided by those
> + * functions is in the xz struct.
> + *
> + * If uv_xz == NULL, Mambo and BML are expected to provide the source address
> + * in the device tree. Cronus can't use putmemdma to secure memory (or secure
> + * memory might be inaccessible to Cronus) so Skiboot copies the UV image from
> + * insecure memory to secure and updates the device tree.
> + */
Put this documentation below around where the if (uv_xz) check is
done. It's not helping anyone up here.
> +void init_uv()
void init_uv(void)
> +{
> + struct dt_node *uv_fw_node;
> + uint64_t uv_dt_src, uv_dt_reg, sys_fdt_addr;
> + uint32_t uv_fw_sz;
> +
> + prlog(PR_DEBUG, "UV: Init starting\n");
> +
> + if (!is_msr_bit_set(MSR_S)) {
> + prlog(PR_DEBUG, "UV: S bit not set\n");
> + return;
> + }
> +
> + if (!(uv_fw_node = get_uv_fw_node())) {
> + prerror("UV: ibm,uv-firmware node creation failed\n");
> + return;
> + }
> +
> + uv_opal = local_alloc(this_cpu()->chip_id, sizeof(struct uv_opal), 8);
use malloc() or memalign(), uv_opal can live in the skiboot heap.
> +
> + tpm_nv_init();
The implementation above is a no-op, so what's this supposed to be
doing and is there going to be a conflict with the TPM NV stuff being
added for secure boot?
> + get_uv_fw_region(&uv_dt_reg, &uv_fw_sz);
> +
> + if (uv_xz) {
> + /* Hostboot path */
> + wait_xz_decompress(uv_xz);
> + if (uv_xz->status) {
> + prerror("UV: Compressed Ultravisor image failed to decompress\n");
> + local_free(uv_xz);
> + local_free(uv_opal);
> + return;
> + }
> + local_free(uv_xz);
> + } else {
> + prlog(PR_INFO, "UV: Platform load failed. You're using Mambo or Cronus.\n");
> + /* use addr provided by reg as source and update with secure memory */
> + uv_dt_src = uv_dt_reg;
> + uv_dt_reg = find_secure_mem();
> + prlog(PR_INFO, "UV: Copying Ultravisor to protected memory 0x%llx from 0x%llx\n", uv_dt_reg, uv_dt_src);
> + memcpy((void *)uv_dt_reg, (void *)uv_dt_src, uv_fw_sz);
> + update_uv_fw_node(uv_dt_reg, uv_fw_sz);
As I said above runtime hacking the DT isn't a good idea. If you want
to support a pre-loaded image then put it into a /reserved-memory/
node and scan the DT for that instead of doing this dual-purpose thing.
> + }
> +
> + uv_opal->uv_base_addr = uv_dt_reg;
> +
> + if (!(sys_fdt_addr = (__be64)create_dtb(dt_root, false)))
> + prerror("UV: Failed to create system fdt\n");
Why are we doing this here and not just prior to entering the UV? Can
we use the same FDT blob that we would normally give to the OS?
> +
> + uv_opal->sys_fdt = sys_fdt_addr;
> + dt_add_property_u64s(uv_fw_node, "sys-fdt", sys_fdt_addr);
> +
> +}
> +
> +/*
> + * Preload the UV image from PNOR partition
> + */
> +void uv_preload_image(void)
> +{
> + int ret;
> +
> + prlog(PR_DEBUG, "UV: Preload starting\n");
> +
> + if (!alloc_uv()) {
> + prerror("UV: Memory allocation failed\n");
> + return;
> + }
> +
> + ret = start_preload_resource(RESOURCE_ID_UV_IMAGE, RESOURCE_SUBID_NONE,
> + uv_image, &uv_image_size);
> +
> + if (ret != OPAL_SUCCESS)
> + prerror("UV: platform load failed: %d\n", ret);
> +}
> +
> +/*
> + * Decompress the UV image
> + *
> + * This function modifies the uv_image variable to point to the decompressed
> + * image location.
> + */
> +void uv_decompress_image(void)
> +{
> + uint64_t uv_fw_addr;
> + uint32_t uv_fw_size;
> +
> + if (uv_image == NULL) {
!var is preferred to var == NULL
> + prerror("UV: Preload hasn't started yet! Aborting.\n");
> + return;
> + }
> +
> + if (wait_for_resource_loaded(RESOURCE_ID_UV_IMAGE,
> + RESOURCE_SUBID_NONE) != OPAL_SUCCESS) {
> + prerror("UV: Ultravisor image load failed\n");
> + return;
> + }
> + uv_xz = local_alloc(this_cpu()->chip_id, sizeof(struct xz_decompress), 8);
> + if (!uv_xz) {
> + prerror("UV: Cannot allocate memory for decompression of UV\n");
> + return;
> + }
Use malloc() for uv_xz. It's a small structure and it'll only ever be
used inside of skiboot so it can go on the skiboot heap.
> +
> + /* the load area is the first secure memory range */
> + get_uv_fw_region(&uv_fw_addr, &uv_fw_size);
> + uv_xz->dst = (void *)uv_fw_addr;
> + uv_xz->dst_size = uv_fw_size;
> + uv_xz->src = uv_image;
> + uv_xz->src_size = uv_image_size;
> +
> + if (stb_is_container((void*)uv_xz->src, uv_xz->src_size))
> + uv_xz->src = uv_xz->src + SECURE_BOOT_HEADERS_SIZE;
> +
> + xz_start_decompress(uv_xz);
> + if ((uv_xz->status != OPAL_PARTIAL) && (uv_xz->status != OPAL_SUCCESS))
> + prerror("UV: XZ decompression failed status 0x%x\n", uv_xz->status);
> +}
> diff --git a/include/mem_region-malloc.h b/include/mem_region-malloc.h
> index 4350c564..c1a6b886 100644
> --- a/include/mem_region-malloc.h
> +++ b/include/mem_region-malloc.h
> @@ -28,4 +28,7 @@ void *__local_alloc(unsigned int chip, size_t size, size_t align,
> #define local_alloc(chip_id, size, align) \
> __local_alloc((chip_id), (size), (align), __location__)
>
> +void __local_free(void *ptr, const char *location);
> +#define local_free(ptr) __local_free(ptr, __location__);
> +
> #endif /* __MEM_REGION_MALLOC_H */
> diff --git a/include/platform.h b/include/platform.h
> index 6ecdbe47..04491d6a 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -17,6 +17,7 @@ enum resource_id {
> RESOURCE_ID_INITRAMFS,
> RESOURCE_ID_CAPP,
> RESOURCE_ID_IMA_CATALOG,
> + RESOURCE_ID_UV_IMAGE,
> RESOURCE_ID_VERSION,
> RESOURCE_ID_KERNEL_FW,
> };
> diff --git a/include/processor.h b/include/processor.h
> index a0c2864a..de0ad6bf 100644
> --- a/include/processor.h
> +++ b/include/processor.h
> @@ -11,6 +11,7 @@
> #define MSR_HV PPC_BIT(3) /* Hypervisor mode */
> #define MSR_VEC PPC_BIT(38) /* VMX enable */
> #define MSR_VSX PPC_BIT(40) /* VSX enable */
> +#define MSR_S PPC_BIT(41) /* Secure Mode enable */
> #define MSR_EE PPC_BIT(48) /* External Int. Enable */
> #define MSR_PR PPC_BIT(49) /* Problem state */
> #define MSR_FP PPC_BIT(50) /* Floating Point Enable */
> @@ -371,6 +372,17 @@ static inline void st_le32(uint32_t *addr, uint32_t val)
> asm volatile("stwbrx %0,0,%1" : : "r"(val), "r"(addr), "m"(*addr));
> }
>
> +/*
> + * MSR bit check
> + */
> +static inline bool is_msr_bit_set(uint64_t bit)
> +{
> + if (mfmsr() & bit)
> + return true;
> +
> + return false;
> +}
> +
> #endif /* __TEST__ */
>
> #endif /* __ASSEMBLY__ */
> diff --git a/include/ultravisor-api.h b/include/ultravisor-api.h
> new file mode 100644
> index 00000000..0301fdd9
> --- /dev/null
> +++ b/include/ultravisor-api.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: Apache-2.0
> +/* Copyright 2018-2019 IBM Corp. */
> +
> +#ifndef __ULTRAVISOR_API_H
> +#define __ULTRAVISOR_API_H
> +
> +#include <types.h>
> +
> +struct uv_opal {
> + __be32 magic; /**< 'OPUV' 0x4F505556 OPUV_MAGIC */
> + __be32 version; /**< uv_opal struct version */
> + __be32 uv_api_ver; /**< Current uv api version. */
> + __be64 uv_base_addr; /**< Base address of UV in secure memory. */
> + __be64 sys_fdt; /**< System FDT. */
> + __be64 uv_fdt; /**< UV FDT in secure memory. */
> + __be64 uv_mem; /**< struct memcons */
> +};
I still don't understand why this exists.
> +
> +#endif /* __ULTRAVISOR_API_H */
> diff --git a/include/ultravisor.h b/include/ultravisor.h
> new file mode 100644
> index 00000000..daeb99b6
> --- /dev/null
> +++ b/include/ultravisor.h
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: Apache-2.0
> +/* Copyright 2018-2019 IBM Corp. */
> +
> +#ifndef __ULTRAVISOR_H
> +#define __ULTRAVISOR_H
> +
> +#include <ultravisor-api.h>
> +#include <processor.h>
> +#include <types.h>
> +
> +/* Bit 15 of an address should be set for it to be used as a secure memory area
> + * for the secure virtual machines */
comment style
> +#define UV_SECURE_MEM_BIT (PPC_BIT(15))
doesn't need parens
> +#define MAX_COMPRESSED_UV_IMAGE_SIZE 0x40000 /* 256 Kilobytes */
> +#define UV_ACCESS_BIT 0x1ULL << 48
does need parens
> +#define UV_LOAD_MAX_SIZE 0x200000
> +#define UV_FDT_MAX_SIZE 0x100000
> +
> +extern int start_uv(uint64_t entry, struct uv_opal *uv_opal);
> +extern bool uv_add_mem_range(__be64 start, __be64 end);
> +extern void uv_preload_image(void);
> +extern void uv_decompress_image(void);
> +extern void init_uv(void);
> +extern int start_ultravisor(void);
> +extern long ucall(unsigned long opcode, unsigned long *retbuf, ...);
> +
> +#endif /* __ULTRAVISOR_H */
More information about the Skiboot
mailing list