[Skiboot] [RFC PATCH v2 2/8] Add ultravisor support in OPAL
Oliver O'Halloran
oohall at gmail.com
Thu Nov 21 15:26:22 AEDT 2019
On Thu, Nov 21, 2019 at 8:49 AM Ryan Grimm <grimm at linux.vnet.ibm.com> wrote:
>
> Oliver,
>
> On Mon, 2019-11-18 at 10:49 +1100, Oliver O'Halloran wrote:
> <snip>
> > > diff --git a/hdata/memory.c b/hdata/memory.c
> > > index 9af7ae71..25b8088d 100644
> > > --- a/hdata/memory.c
> > > +++ b/hdata/memory.c
> > > @@ -10,6 +10,7 @@
> > > #include <types.h>
> > > #include <inttypes.h>
> > > #include <processor.h>
> > > +#include <ultravisor.h>
> > >
> > > #include "spira.h"
> > > #include "hdata.h"
> > > @@ -59,6 +60,8 @@ 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_SMF (PPC_BIT32(23))
> > > +
> > > struct HDIF_ms_area_id {
> > > __be16 id;
> > > #define MS_PTYPE_RISER_CARD 0x8000
> > > @@ -163,6 +166,16 @@ static bool add_address_range(struct dt_node
> > > *root,
> > > return false;
> > > }
> > >
> > > + if (arange->mirror_attr & MS_ATTR_SMF) {
> > > + prlog(PR_DEBUG, "Found secure memory");
> > > + if (!uv_add_mem_range(reg[0],
> > > cleanup_addr(be64_to_cpu(arange->end)))) {
> > > + prlog(PR_INFO, "Failed to add secure memory
> > > range to DT\n");
> > > + mem_reserve_fw(name, reg[0], reg[1]);
> >
> > The reservation facility is there to allow marking bits of otherwise
> > normal memory as "special" so that skiboot and the kernel won't
> > allocate over them and trash their contents. The secure memory ranges
> > are completely disjoint from normal memory by design so IMO they
> > should be top-level nodes, similar to the normal memory@<addr> nodes.
> > There's a lot of code in this patch that exists purely to work around
> > the descision to abuse reserved memory this way, so it should
> > simplify
> > the skiboot changes too.
> >
>
> OK, we have been using the reservation system improperly. And, yeah,
> we do have little pieces of code here and there to fix things up, which
> are sure to be fragile.
>
> How about we have something like this in the doc, and use device_type
> "secure_memory" so we don't have the kernel try to use it as regular
> memory:
>
> Skiboot parses secure memory from the HDAT tables and creates the
> secure-memory device tree node, similar to a memory@ node except the
> device_type is "secure_memory". For example:
>
> .. code-block:: dts
>
> secure-memory at 100fe00000000 {
> device_type = "secure_memory";
> ibm,chip-id = <0>;
> reg = < 0x100fe 0x0 0x2 0x0>;
> }
>
> Regions of secure memory will be reserved by hostboot such as OCC,
> HOMER, and SBE. Skiboot will use the existing reserve infrastructure
> to reserve them.
> For example:
>
> .. code-block::
>
> ibm,HCODE at 100fffcaf0000
> ibm,OCC at 100fffcdd0000
> ibm,RINGOVD at 100fffcae0000
> ibm,WOFDATA at 100fffcb90000
> ibm,arch-reg-data at 100fffd700000
> ibm,hbrt-code-image at 100fffcec0000
> ibm,hbrt-data at 100fffd420000
> ibm,homer-image at 100fffd800000
> ibm,homer-image at 100fffdc00000
> ibm,occ-common-area at 100ffff800000
> ibm,sbe-comm at 100fffce90000
> ibm,sbe-comm at 100fffceb0000
> ibm,sbe-ffdc at 100fffce80000
> ibm,sbe-ffdc at 100fffcea0000
> ibm,secure-crypt-algo-code at 100fffce70000
> ibm,uvbwlist at 100fffcad0000
>
> For Mambo, ultra.tcl creates the secure-memory device tree node and is
> currently defined at 8GB with size 8GB. Mambo has no protection on
> securememory, so a watchpoint could be used to ensure Skiboot does not
> touch secure memory.
>
> For BML, the BML script parses secure memory from the Cronus config
> file and creates the secure-memory device tree node.
Looks ok to me. mpe might have some comments.
> > > *snip*
> > >
> > > +static void free_uv(void)
> > > +{
> > > + struct mem_region *region = find_mem_region("ibm,
> > > firmware-allocs-memory at 0");
> > > +
> > > + lock(®ion->free_list_lock);
> > > + mem_free(region, uv_image, __location__);
> > > + unlock(®ion->free_list_lock);
> > > +}
> >
> > ibm,firmware-allocs-memory at 0 contains everything allocated by skiboot
> > on node 0 using local_alloc()
> >
>
> OK, I think I wrote that code being unsure about how to do this.
>
> So, we can use local_alloc, but I'm struggling a little bit to figure
> out how to free it. I looked at examples of local_alloc, but it
> doesn't seem any of the code in occ, phb4, or xive free their
> allocations.
>
> Is it up to the caller to deal with mem_regions and call mem_free with
> the mem_region?
Historically local_alloc() has only ever been used for static
allocations of things like the in-memory PHB tables. Up until now we
haven't needed the free those, so a local_free() just never got
written. Feel free to add one, but if you do then make sure you handle
returning the allocated memory back into the region it was allocated
from (i.e. fold it into an adjacent region with type ==
REGION_MEMORY).
> > > *snip*
> > > +{
> > > + struct dt_node *node;
> > > + const struct dt_property *base;
> > > + uint64_t uv_src_addr, uv_pef_reg, uv_pef_size;
> > > + void *uv_fdt;
> > > +
> > > + prlog(PR_DEBUG, "UV: Init starting\n");
> > > +
> > > + if (!is_msr_bit_set(MSR_S)) {
> > > + prerror("UV: S bit not set\n");
> > > + goto load_error;
> >
> > that's not an error.
> >
>
> OK, we plan on looking at these paths closely, as we have not tested or
> thought about when we're running withour MSR_S.
That's what I figured. It's not a big deal, we try not to add nuisance
prints and the testers treat skiboot printing anything at PR_ERR or
lower as an excuse to file a bug report. Make this sort of FYI print a
PR_DEBUG or higher so we don't see it in the normal case.
> > > + }
> > > +
> > > + if (!(uv_opal = zalloc(sizeof(struct uv_opal)))) {
> > > + prerror("UV: Failed to allocate uv_opal\n");
> > > + goto load_error;
> > > + }
> > > +
> > > +
> > > + if (!(node = find_uv_node())) {
> > > + prerror("UV: Device tree node not found\n");
> > > + goto load_error;
> > > + }
> > > +
> > > + if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS) {
> > > + prlog(PR_INFO, "UV: Mambo simulator detected\n");
> > > +
> > > + if (!find_secure_mem_to_copy(&uv_pef_reg,
> > > &uv_pef_size)) {
> > > + prerror("UV: No secure memory configured,
> > > exiting\n");
> > > + goto load_error;
> > > + }
> > > +
> > > + goto start;
> > > + }
> >
> > Seems like a weird hack.
> >
>
> Yeah, this is how the code evolved supporting Mambo, Cronus, and HB.
> Do you think we should break each up into its own function?
I think mambo should be handled the same way cronus is. The only
justification for the current hack seems to be that it lets you skip
copying the UV blob from insecure to secure memory. It's pretty hard
to care about the overhead of the copy and I'd rather keep the
differences between platforms to a minimum.
> > > diff --git a/include/processor.h b/include/processor.h
> > > index 352fd1ec..0a552998 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 */
> > > @@ -368,6 +369,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__ */
> >
> > I'm going to take a stab in the dark and say this is going to break
> > all of our unit tests.
> >
>
> OK, we can start running the unit tests, we haven't been doing that.
Nobody ever does :(
More information about the Skiboot
mailing list