Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState
On Fri, May 29, 2020 at 11:09:41AM +0200, Philippe Mathieu-Daudé wrote: > On 5/21/20 5:42 AM, David Gibson wrote: > > Currently SevGuestState contains only configuration information. For > > runtime state another non-QOM struct SEVState is allocated separately. > > > > Simplify things by instead embedding the SEVState structure in > > SevGuestState. > > > > Signed-off-by: David Gibson > > --- > > target/i386/sev.c | 54 +-- > > 1 file changed, 29 insertions(+), 25 deletions(-) > > > > diff --git a/target/i386/sev.c b/target/i386/sev.c > > index b6ed719fb5..b4ab9720d6 100644 > > --- a/target/i386/sev.c > > +++ b/target/i386/sev.c > > @@ -35,6 +35,22 @@ > > > > typedef struct SevGuestState SevGuestState; > > > > +struct SEVState { > > +uint8_t api_major; > > +uint8_t api_minor; > > +uint8_t build_id; > > +uint32_t policy; > > +uint64_t me_mask; > > +uint32_t cbitpos; > > +uint32_t reduced_phys_bits; > > +uint32_t handle; > > +int sev_fd; > > +SevState state; > > +gchar *measurement; > > +}; > > + > > +typedef struct SEVState SEVState; > > Maybe typedef & declaration altogether. I'd prefer not - this makes it clearer that it's a pure code motion. It's going to go away in a few patches time in any case. > > + > > /** > > * SevGuestState: > > * > > @@ -48,6 +64,7 @@ typedef struct SevGuestState SevGuestState; > > struct SevGuestState { > > Object parent_obj; > > > > +/* configuration parameters */ > > char *sev_device; > > uint32_t policy; > > uint32_t handle; > > @@ -55,25 +72,11 @@ struct SevGuestState { > > char *session_file; > > uint32_t cbitpos; > > uint32_t reduced_phys_bits; > > -}; > > > > -struct SEVState { > > -SevGuestState *sev_info; > > -uint8_t api_major; > > -uint8_t api_minor; > > -uint8_t build_id; > > -uint32_t policy; > > -uint64_t me_mask; > > -uint32_t cbitpos; > > -uint32_t reduced_phys_bits; > > -uint32_t handle; > > -int sev_fd; > > -SevState state; > > -gchar *measurement; > > +/* runtime state */ > > +SEVState state; > > }; > > > > -typedef struct SEVState SEVState; > > - > > #define DEFAULT_GUEST_POLICY0x1 /* disable debug */ > > #define DEFAULT_SEV_DEVICE "/dev/sev" > > > > @@ -506,12 +509,12 @@ sev_read_file_base64(const char *filename, guchar > > **data, gsize *len) > > } > > > > static int > > -sev_launch_start(SEVState *s) > > +sev_launch_start(SevGuestState *sev) > > { > > +SEVState *s = >state; > > gsize sz; > > int ret = 1; > > int fw_error, rc; > > -SevGuestState *sev = s->sev_info; > > struct kvm_sev_launch_start *start; > > guchar *session = NULL, *dh_cert = NULL; > > > > @@ -686,6 +689,7 @@ sev_vm_state_change(void *opaque, int running, RunState > > state) > > void * > > sev_guest_init(const char *id) > > { > > +SevGuestState *sev; > > SEVState *s; > > char *devname; > > int ret, fw_error; > > @@ -693,27 +697,27 @@ sev_guest_init(const char *id) > > uint32_t host_cbitpos; > > struct sev_user_data_status status = {}; > > > > -sev_state = s = g_new0(SEVState, 1); > > -s->sev_info = lookup_sev_guest_info(id); > > -if (!s->sev_info) { > > +sev = lookup_sev_guest_info(id); > > +if (!sev) { > > error_report("%s: '%s' is not a valid '%s' object", > > __func__, id, TYPE_SEV_GUEST); > > goto err; > > } > > > > +sev_state = s = >state; > > I was going to suggest to clean that, but I see your next patch already > does the cleanup :) > > Reviewed-by: Philippe Mathieu-Daudé > > > s->state = SEV_STATE_UNINIT; > > > > host_cpuid(0x801F, 0, NULL, , NULL, NULL); > > host_cbitpos = ebx & 0x3f; > > > > -s->cbitpos = object_property_get_int(OBJECT(s->sev_info), "cbitpos", > > NULL); > > +s->cbitpos = object_property_get_int(OBJECT(sev), "cbitpos", NULL); > > if (host_cbitpos != s->cbitpos) { > > error_report("%s: cbitpos check failed, host '%d' requested '%d'", > > __func__, host_cbitpos, s->cbitpos); > > goto err; > > } > > > > -s->reduced_phys_bits = object_property_get_int(OBJECT(s->sev_info), > > +s->reduced_phys_bits = object_property_get_int(OBJECT(sev), > > "reduced-phys-bits", NULL); > > if (s->reduced_phys_bits < 1) { > > error_report("%s: reduced_phys_bits check failed, it should be > > >=1," > > @@ -723,7 +727,7 @@ sev_guest_init(const char *id) > > > > s->me_mask = ~(1UL << s->cbitpos); > > > > -devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", > > NULL); > > +devname = object_property_get_str(OBJECT(sev), "sev-device", NULL); > > s->sev_fd = open(devname, O_RDWR); > > if (s->sev_fd < 0) { > > error_report("%s:
Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState
On 5/20/20 8:42 PM, David Gibson wrote: > Currently SevGuestState contains only configuration information. For > runtime state another non-QOM struct SEVState is allocated separately. > > Simplify things by instead embedding the SEVState structure in > SevGuestState. > > Signed-off-by: David Gibson > --- > target/i386/sev.c | 54 +-- > 1 file changed, 29 insertions(+), 25 deletions(-) Reviewed-by: Richard Henderson r~
Re: [RFC v2 04/18] target/i386: sev: Embed SEVState in SevGuestState
On 5/21/20 5:42 AM, David Gibson wrote: > Currently SevGuestState contains only configuration information. For > runtime state another non-QOM struct SEVState is allocated separately. > > Simplify things by instead embedding the SEVState structure in > SevGuestState. > > Signed-off-by: David Gibson > --- > target/i386/sev.c | 54 +-- > 1 file changed, 29 insertions(+), 25 deletions(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index b6ed719fb5..b4ab9720d6 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -35,6 +35,22 @@ > > typedef struct SevGuestState SevGuestState; > > +struct SEVState { > +uint8_t api_major; > +uint8_t api_minor; > +uint8_t build_id; > +uint32_t policy; > +uint64_t me_mask; > +uint32_t cbitpos; > +uint32_t reduced_phys_bits; > +uint32_t handle; > +int sev_fd; > +SevState state; > +gchar *measurement; > +}; > + > +typedef struct SEVState SEVState; Maybe typedef & declaration altogether. > + > /** > * SevGuestState: > * > @@ -48,6 +64,7 @@ typedef struct SevGuestState SevGuestState; > struct SevGuestState { > Object parent_obj; > > +/* configuration parameters */ > char *sev_device; > uint32_t policy; > uint32_t handle; > @@ -55,25 +72,11 @@ struct SevGuestState { > char *session_file; > uint32_t cbitpos; > uint32_t reduced_phys_bits; > -}; > > -struct SEVState { > -SevGuestState *sev_info; > -uint8_t api_major; > -uint8_t api_minor; > -uint8_t build_id; > -uint32_t policy; > -uint64_t me_mask; > -uint32_t cbitpos; > -uint32_t reduced_phys_bits; > -uint32_t handle; > -int sev_fd; > -SevState state; > -gchar *measurement; > +/* runtime state */ > +SEVState state; > }; > > -typedef struct SEVState SEVState; > - > #define DEFAULT_GUEST_POLICY0x1 /* disable debug */ > #define DEFAULT_SEV_DEVICE "/dev/sev" > > @@ -506,12 +509,12 @@ sev_read_file_base64(const char *filename, guchar > **data, gsize *len) > } > > static int > -sev_launch_start(SEVState *s) > +sev_launch_start(SevGuestState *sev) > { > +SEVState *s = >state; > gsize sz; > int ret = 1; > int fw_error, rc; > -SevGuestState *sev = s->sev_info; > struct kvm_sev_launch_start *start; > guchar *session = NULL, *dh_cert = NULL; > > @@ -686,6 +689,7 @@ sev_vm_state_change(void *opaque, int running, RunState > state) > void * > sev_guest_init(const char *id) > { > +SevGuestState *sev; > SEVState *s; > char *devname; > int ret, fw_error; > @@ -693,27 +697,27 @@ sev_guest_init(const char *id) > uint32_t host_cbitpos; > struct sev_user_data_status status = {}; > > -sev_state = s = g_new0(SEVState, 1); > -s->sev_info = lookup_sev_guest_info(id); > -if (!s->sev_info) { > +sev = lookup_sev_guest_info(id); > +if (!sev) { > error_report("%s: '%s' is not a valid '%s' object", > __func__, id, TYPE_SEV_GUEST); > goto err; > } > > +sev_state = s = >state; I was going to suggest to clean that, but I see your next patch already does the cleanup :) Reviewed-by: Philippe Mathieu-Daudé > s->state = SEV_STATE_UNINIT; > > host_cpuid(0x801F, 0, NULL, , NULL, NULL); > host_cbitpos = ebx & 0x3f; > > -s->cbitpos = object_property_get_int(OBJECT(s->sev_info), "cbitpos", > NULL); > +s->cbitpos = object_property_get_int(OBJECT(sev), "cbitpos", NULL); > if (host_cbitpos != s->cbitpos) { > error_report("%s: cbitpos check failed, host '%d' requested '%d'", > __func__, host_cbitpos, s->cbitpos); > goto err; > } > > -s->reduced_phys_bits = object_property_get_int(OBJECT(s->sev_info), > +s->reduced_phys_bits = object_property_get_int(OBJECT(sev), > "reduced-phys-bits", NULL); > if (s->reduced_phys_bits < 1) { > error_report("%s: reduced_phys_bits check failed, it should be >=1," > @@ -723,7 +727,7 @@ sev_guest_init(const char *id) > > s->me_mask = ~(1UL << s->cbitpos); > > -devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", > NULL); > +devname = object_property_get_str(OBJECT(sev), "sev-device", NULL); > s->sev_fd = open(devname, O_RDWR); > if (s->sev_fd < 0) { > error_report("%s: Failed to open %s '%s'", __func__, > @@ -754,7 +758,7 @@ sev_guest_init(const char *id) > goto err; > } > > -ret = sev_launch_start(s); > +ret = sev_launch_start(sev); > if (ret) { > error_report("%s: failed to create encryption context", __func__); > goto err; >