[Skiboot] [RFC PATCH 3/8] pef: Load ultravisor from pnor/flash

[Ryan Grimm]

Thanks for reviewing!  I was subscribed to the skiboot list in digest
mode so I'm fixing up replies by hand for this thread, apologies if it
looks weird.  

>> + * Preload the uv image from SMF pnor partition
>> + */

>SMF isn't a term that's used elsewhere

Oops, we don't want to use that term, I thought I grep'd for them, will
remove/fix.

>> +	uv_image = malloc(MAX_COMPRESSED_UV_IMAGE_SIZE);
>> +	if (!uv_image) {
>> +		prerror("Memory allocation for ultravisor failed\n");
>> +		return;
>> +	}

>Should this be on the heap or should we allocate it off a main memory
>region somewhere (and subsequently free it)?

>What if the UV grows?

Good point.  You asked below but to answer here the UV partition is
1MB.  

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?

>> +
>> +	ret = start_preload_resource(RESOURCE_ID_UV_IMAGE,
RESOURCE_SUBID_NONE,
>> +				     uv_image, &uv_image_size);
>> +
>> +	if (ret != OPAL_SUCCESS) {
>> +		prerror("UV: Failed to preload Ultravisor image: %d\n",
ret);
>> +		free(uv_image);
>> +		uv_image = NULL;
>> +	}

>All of this code looks familiar to other resources, there should >
>probably be just some common code o do this.

K, it is similar to loading KERNEL and INITRAMFS, but, I thought we
were doing it right.  I'll look again.

>> +	if (uv_on_hw() == false)
>> +		return;

>why can't we run in sim? What prevents us from loading and running it
>with mambo?

Good point, we don't need that.  I removed uv_on_hw and calls to it and
things are OK.

>> +
>> +	if (uv_image == NULL) {
>> +		prerror("UV: Preload hasn't started yet! Aborting.\n");

>or did it error out?

K, will check what is done with kernel and try to be consistent.

>> +		return;
>> +	}
>> +
>> +	if (wait_for_resource_loaded(RESOURCE_ID_UV_IMAGE,
>> +				     RESOURCE_SUBID_NONE) !=
OPAL_SUCCESS) {
>> +		prerror("UV: Ultravisor image load failed\n");
>> +		return;
>> +	}
>> +
>> +	uv_node = dt_find_by_name(dt_root, "ibm,uv-firmware");
>> +	if (!uv_node) {
>> +		prerror("UV: Cannot find ibm,uv-firmware node\n");
>> +		return;
>> +	}
>> +
>> +	ranges = dt_find_property(uv_node, "secure-memory-ranges");
>> +	if (!ranges) {
>> +		prerror("UV: Cannot find secure-memory-ranges");
>> +		return;
>> +	}
>> +
>> +	uv_xz = malloc(sizeof(struct xz_decompress));
>> +	if (!uv_xz) {
>> +		prerror("UV: Cannot allocate memory for decompression
of UV\n");
>> +		return;
>> +	}

>in all of these cases, memory allocated for the UV image isn't freed.

K, right, will fix it up.

>> +	/* the load area is the first secure memory range */
>> +	range = (void *)ranges->prop;
>> +	uv_xz->dst = (void *)dt_get_number(range, 2);
>> +	uv_xz->dst_size = dt_get_number(range + 1, 2);
>> +	uv_xz->src = uv_image;
>> +	uv_xz->src_size = uv_image_size;
>> +
>> +	/* TODO security and integrity checks? */

>I think it should certainly verify and measure loading the UV before
>any UV patch is merged.

K, will do.

Thanks,
Ryan