| Message ID | 20200521034304.340040-5-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor configuration of guest memory protection | expand |
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 <david@gibson.dropbear.id.au> > --- > 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_POLICY 0x1 /* 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 = &sev->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 = &sev->state; I was going to suggest to clean that, but I see your next patch already does the cleanup :) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > s->state = SEV_STATE_UNINIT; > > host_cpuid(0x8000001F, 0, NULL, &ebx, 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; >
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 <david@gibson.dropbear.id.au> > --- > target/i386/sev.c | 54 +++++++++++++++++++++++++---------------------- > 1 file changed, 29 insertions(+), 25 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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 <david@gibson.dropbear.id.au> > > --- > > 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_POLICY 0x1 /* 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 = &sev->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 = &sev->state; > > I was going to suggest to clean that, but I see your next patch already > does the cleanup :) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > s->state = SEV_STATE_UNINIT; > > > > host_cpuid(0x8000001F, 0, NULL, &ebx, 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; > > >
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; + /** * 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_POLICY 0x1 /* 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 = &sev->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 = &sev->state; s->state = SEV_STATE_UNINIT; host_cpuid(0x8000001F, 0, NULL, &ebx, 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;
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 <david@gibson.dropbear.id.au> --- target/i386/sev.c | 54 +++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 25 deletions(-)