Re: [Qemu-devel] [RFC PATCH v1 06/22] sev: add initial SEV support

[Brijesh Singh]

> > Various commands and parameters are documented [1]
> >
> > [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf
>
> If I understand correctly, the docs describe the firmware
> interface. The interface provided by QEMU is not the same thing,
> and needs to be documented as well (even if it contains pointers
> to sections or tables in the firmware interface docs).
>
> Some of the questions I have about the fields are:
> * Do we really need the user to provide all the options below?
>   * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask,
>     for example?

Good question, I don't think we need to get this information from guest 
owner and it can be calculated from KVM. I will check with security 
folks on how this information is used in measurement generation and make 
the changes accordingly.

> * Is bit 0 (KS) the only bit that can be set on flags? If so, why
>   not a boolean "ks" option?
Agreed. I will fix in v2.

> * Is "policy" the guest policy structure described at page 23? If
>   so, why exposing the raw value instead of separate fields for
>   each bit/field in the structure? (and only for the ones that
>   are supposed to be set by the user)

Yes policy is described in chapter 3, page 23. I am open to separate the 
fields.
Let me know if something like this works

sev-launch-rule,flags.ks=0,policy.dbg=0,policy.ks=0,policy.nosend=0,...


> * If vcpu_mask is a bitmap for each VCPU, should we represent it
>   as a list of VCPU indexes?
I will check on this one.

> A good way to model this data and document it more properly is
> through a QAPI schema. grep for "opts_visitor_new()" in the code
> for examples where QEMU options are parsed according to a QAPI
> schema. The downside is that using a QAPI visitor is (AFAIK) not
> possible if using -object like I suggest below.
>
> > > > [sev-launch]
> > > >         flags = "00000000"
> > > >         policy  = "000000"
> > > >         dh_pub_qx = "0123456789abcdef0123456789abcdef"
> > > >         dh_pub_qy = "0123456789abcdef0123456789abcdef"
> > > >         nonce = "0123456789abcdef"
> > > >         vcpu_count = "1"
> > > >         vcpu_length = "30"
> > > >         vcpu_mask = "00ab"
> >
> > All these config parameters should be provided by the guest owner before
> > launching or migrating a guest. I believe we need to make changes in
> > libvirt, virsh and other upper layer software stack to communicate with
> > guest owner to get these input parameters. For development purposes I choose
> > a simple config file to get these parameters. I am not sure if we will able
> > to "add new option to a existing objects/device" but we can look into
> > creating a "new type for -object" or we can simply accept a fd from upper
> > layer and read the fd to get these parameters.
>
> I was thinking of something like:
>
>   -object 
> sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab
>  \
>   -machine pc,accel=kvm,sev=on  # see note below[1]
>
> With this, you won't need separate code for command-line, config
> files, and QMP commands. They will all be able to use the same
> mechanisms.
>
> ...but this conflicts with the idea of using QAPI. So I am not
> sure which way to go. (But either way we go, we need a clearer
> and better documented set of parameters).

I am open to idea and need direction on which way to go. I will  work on 
documenting the parameters and usages. Should I consider implementing 
your below approach in v2 ?

-object 
sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab 
\ -machine pc,accel=kvm,sev=on

Any tips on which qemu file i can use as reference during 
implementation. Thanks.

> [1] I would go even further and separate the accel object options
>     from the machine options, but this would require reworking
>     the accelerator configuration inside QEMU. e.g.:
>
>   -object kvm-accel,sev=on,id=kvm0
>   -machine pc,accel=kvm0
>
> [...]
> > > Do we really need to write our own parser? I wonder if we can
> > > reuse crypto/secret.c for loading the keys.
> > I just looked at crypto/secret.c for loading the keys but not sure if will
> > able to reuse the secret_load routines, this is mainly because the SEV
> > inputs parameters are different compare to what we have in crypto/secrets.c.
> > I will still look more closely and see if we can find some common code.
>
> There are other parameters, sure, but maybe it would be
> appropriate to just load nonce/dh_pub_qx/dh_pub_qy as
> TTYPE_QCRYPTO_SECRET object(s) (-object secret,...)? I am not
> sure because I don't understand the crypto part fully.
>
> Daniel, what do you think?