Re: [PATCH v4 2/6] s390/uv: Retrieve UV secrets support

[Janosch Frank <frankja@linux.ibm.com>]

On 10/18/24 11:15 AM, Steffen Eiden wrote:
> Provide a kernel API to retrieve secrets from the UV secret store.
> Add two new functions:
> * `uv_get_secret_metadata` - get metadata for a given secret identifier
> * `uv_retrieve_secret` - get the secret value for the secret index
> 
> With those two functions one can extract the secret for a given secret
> id, if the secret is retrievable.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---

[...]

> +/**
> + * uv_secret_list_item_hdr - UV secret metadata.
> + * @index: Index of the secret in the secret list.
> + * @type: Type of the secret. See `enum uv_secret_types`.
> + * @length: Length of the stored secret.
> + */
> +struct uv_secret_list_item_hdr {
> +	u16 index;
> +	u16 type;
> +	u32 length;
> +};
> +
> +#define UV_SECRET_ID_LEN 32
> +/**
> + * uv_secret_list_item - UV secret entry.
> + * @hdr: The metadata of this secret.
> + * @id: The ID of this secret, not the secret itself.
> + */
> +struct uv_secret_list_item {
> +	struct uv_secret_list_item_hdr hdr;
> +	u64 reserverd08;
> +	u8 id[UV_SECRET_ID_LEN];
> +};

Are you skipping the size asserts since the list is asserted?
It might still make sense to pack them, no?

>   static inline int __uv_call(unsigned long r1, unsigned long r2)
>   {
>   	int cc;
> @@ -383,6 +469,47 @@ static inline int uv_cmd_nodata(u64 handle, u16 cmd, u16 *rc, u16 *rrc)
>   	return cc ? -EINVAL : 0;
>   }
>   
> +/**
> + *  uv_list_secrets() - Do a List Secrets UVC.
> + *
> + *  @buf: Buffer to write list into; size of one page.
> + *  @start_idx: The smallest index that should be included in the list.
> + *		For the fist invocation use 0.
> + *  @rc: Pointer to store the return code or NULL.
> + *  @rrc: Pointer to store the return reason code or NULL.
> + *
> + *  This function calls the List Secrets UVC. The result is written into `buf`,
> + *  that needs to be at least one page of writable memory.
> + *  `buf` consists of:
> + *  * %struct uv_secret_list_hdr
> + *  * %struct uv_secret_list_item (multiple)
> + *
> + *  For `start_idx` use _0_ for the first call. If there are more secrets available
> + *  but could not fit into the page then `rc` is `UVC_RC_MORE_DATA`.
> + *  In this case use `uv_secret_list_hdr.next_secret_idx` for `start_idx`.
> + *
> + *  Context: might sleep.
> + *
> + *  Return: The UVC condition code.
> + */
> +static inline int uv_list_secrets(u8 *buf, u16 start_idx, u16 *rc, u16 *rrc)
>

Why is buf (u8 *) if you have it as (struct uv_secret_list *) in 
find_secret()?

You have a second caller in list_secrets() but that can also be (struct 
uv_secret_list *) since copy_to_user() shouldn't care and you need to 
cast it anyway for alloc_page().

If you'd be passing buf as u64 and not as a pointer it would make sense 
but you're casting it to u64 here