On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote:
> Hi Eduardo,
>
> On 09/13/2016 10:58 AM, Eduardo Habkost wrote:
> > >
> > > A typical SEV config file looks like this:
> > >
> >
> > Are those config options documented somewhere?
> >
>
> Various commands and parameters are documented [1]
>
> [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf
Could you write a TL;DR summary?
IMHO it's not reasonable to ask that people read spec
documents in order to understand QEMU command line
parameters or config flags.
Some text should also go into docs/
> > > [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"
> >
> > Why a separate config file loading mechanism? If the user really
> > needs to load the SEV configuration data from a separate file,
> > you can just use regular config sections and use -readconfig.
> >
> I will look into -readconfig and see if we can use that.
>
> > Now, about the format of the new config sections ("sev" and
> > "sev-launch"): I am not sure adding new command-line options and
> > config sections is necessary. Is it possible to implement it as a
> > combination of:
> > * new options to existing command-line options and/or
> > * new options to existing objects and/or
> > * new options to existing devices and/or
> > * new types for -object? (see how crypto secrets and net filters
> > are configured, for an example)
> >
> >
>
> 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.
>
> > [...]
> > > extern bool kvm_allowed;
> > > +extern bool kvm_sev_allowed;
> >
> > Can we place this inside struct KVMState?
> >
> Yes, i will add this in v2.
> > > extern bool kvm_kernel_irqchip;
> > > extern bool kvm_split_irqchip;
> > > extern bool kvm_async_interrupts_allowed;
> > [...]
> > > @@ -1745,6 +1747,10 @@ static int kvm_init(MachineState *ms)
> > >
> > > kvm_state = s;
> > >
> > > + if (!sev_init(kvm_state)) {
> > > + kvm_sev_allowed = true;
> > > + }
> >
> > sev_init() errors are being ignored here. sev_init() could report
> > errors properly using Error** (and kvm_init() should not ignore
> > them).
> Thanks, will fix in v2.
> >
> > > +
> > > if (kvm_eventfds_allowed) {
> > > s->memory_listener.listener.eventfd_add = kvm_mem_ioeventfd_add;
> > > s->memory_listener.listener.eventfd_del = kvm_mem_ioeventfd_del;
> > [...]
> > > +
> > > +struct SEVInfo {
> > > + uint8_t state; /* guest current state */
> > > + uint8_t type; /* guest type (encrypted, unencrypted) */
> > > + struct kvm_sev_launch_start *launch_start;
> > > + struct kvm_sev_launch_update *launch_update;
> > > + struct kvm_sev_launch_finish *launch_finish;
> > > +};
> > > +
> > > +typedef struct SEVInfo SEVInfo;
> > > +static SEVInfo *sev_info;
> >
> > Can we place this pointer inside struct KVMState?
> >
> Will try to move into KVMState.
> > [...]
> > > +static unsigned int read_config_u32(QemuOpts *opts, const char *key)
> > > +{
> > > + unsigned int val;
> > > +
> > > + val = qemu_opt_get_number_del(opts, key, -1);
> > > + DPRINTF(" %s = %08x\n", key, val);
> > > +
> > > + return val;
> > > +}
> > > +
> > > +static int read_config_u8(QemuOpts *opts, const char *key, uint8_t *val)
> >
> > Function name confused me (it seemed to read only one u8 value).
> >
> I will fix the name, the function converts string into a hex bytes array.
> e.g "12aabbccdd" is converted to val[] = {0x12,0xaa,0xbb,0xcc, 0xdd}.
>
> > > +{
> > > + int i;
> > > + const char *v;
> > > +
> > > + v = qemu_opt_get(opts, key);
> > > + if (!v) {
> > > + return 0;
> > > + }
> > > +
> > > + DPRINTF(" %s = ", key);
> > > + i = 0;
> > > + while (*v) {
> > > + sscanf(v, "%2hhx", &val[i]);
> >
> > Function doesn't check for array length.
> Thanks, i will fix this.
> >
> > > + DPRINTF("%02hhx", val[i]);
> > > + v += 2;
> > > + i++;
> > > + }
> > > + DPRINTF("\n");
> > > +
> > > + return i;
> >
> > 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.
> > > +}
> > > +
> > [...]
> >
