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

[Steffen Eiden <seiden@linux.ibm.com>]

On 10/22/24 2:16 PM, Janosch Frank wrote:
> 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?
yes.
> It might still make sense to pack them, no?
It is unnecessary to pack the struct as it is naturally aligned (at least on s390).
While, processing your comment I noticed I unnecessarily packed uv_secret_list as well.
Probably because nearly every struct in that header is (IMO unnecessarily) packed.
IIRC packing may restrict optimizations done by the compiler.

However, to stay consistent with the rest of that file I will pack this struct as well
(+ uv_secret_list_item_hdr). Those are not hot paths anyways.


> 
>>   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().
>
Yes, that makes sense. I'll change that to struct uv_secret_list *


> 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
> 
Thanks for the feedback.

Steffen