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

[Steffen Eiden]

Thanks for your comments Janosch.

On 10/7/24 2:31 PM, Janosch Frank wrote:
> On 10/2/24 6:05 PM, 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@xxxxxxxxxxxxx>
> > ---
> >   arch/s390/include/asm/uv.h | 131 ++++++++++++++++++++++++++++++++++++-
> >   arch/s390/kernel/uv.c      | 127 ++++++++++++++++++++++++++++++++++-
> >   2 files changed, 256 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> > index 94ff58336e8e..aef333aaaef4 100644
> > --- a/arch/s390/include/asm/uv.h
> > +++ b/arch/s390/include/asm/uv.h
> > @@ -62,6 +62,7 @@
> >   #define UVC_CMD_ADD_SECRET        0x1031
> >   #define UVC_CMD_LIST_SECRETS        0x1033
> >   #define UVC_CMD_LOCK_SECRETS        0x1034
> > +#define UVC_CMD_RETR_SECRET        0x1035
> >   /* Bits in installed uv calls */
> >   enum uv_cmds_inst {
> > @@ -95,6 +96,7 @@ enum uv_cmds_inst {
> >       BIT_UVC_CMD_ADD_SECRET = 29,
> >       BIT_UVC_CMD_LIST_SECRETS = 30,
> >       BIT_UVC_CMD_LOCK_SECRETS = 31,
> > +    BIT_UVC_CMD_RETR_SECRETS = 33,
>
> One is SECRET and the other is SECRET_S_.
> I know that you coded this according to spec, but sometimes we need to fix the spec. I've contacted the spec authors to fix it on their end if possible.
Yes we should fix the specs.
I will use singular forms on both constants.

> [...]

> > + * Do the actual search for `uv_get_secret_metadata`
> > + *
> > + * Context: might sleep
> > + */
> > +static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
> > +               struct uv_secret_list *list,
> > +               struct uv_secret_list_item_hdr *secret)
> > +{
> > +    u16 start_idx = 0;
> > +    u16 list_rc;
> > +    int ret;
> > +
> > +    do {
> > +        uv_list_secrets((u8 *)list, start_idx, &list_rc, NULL);
> > +        if (!(list_rc == UVC_RC_EXECUTED || list_rc == UVC_RC_MORE_DATA)) {
>
> Inverting this conditional would get rid of 3 characters.
> Did you chose to implement it like this on purpose?
No special purpose behind that. In fact, I prefer your shorter variant.
Thanks, I'll change that.

> > +            if (list_rc == UVC_RC_INV_CMD)
> > +                return -ENODEV;
> > +            else
> > +                return -EIO;
> > +        }
> > +        ret = find_secret_in_page(secret_id, list, secret);
> > +        if (ret == 0)
> > +            return ret;
> > +        start_idx = list->next_secret_idx;
> > +    } while (list_rc == UVC_RC_MORE_DATA && start_idx < list->next_secret_idx);
> > +
> > +    return -ENOENT;

Steffen