Re: [PATCH v3 23/49] i386/sev: Add a sev_snp_enabled() helper
On Wed, Mar 20, 2024 at 12:35:09PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:19AM -0500, Michael Roth wrote:
> > Add a simple helper to check if the current guest type is SNP. Also have
> > SNP-enabled imply that SEV-ES is enabled as well, and fix up any places
> > where the sev_es_enabled() check is expecting a pure/non-SNP guest.
> >
> > Signed-off-by: Michael Roth
> > ---
> > target/i386/sev.c | 13 -
> > target/i386/sev.h | 2 ++
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 7e6dab642a..2eb13ba639 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
>
>
> > @@ -933,7 +942,9 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs,
> > Error **errp)
> > __func__);
> > goto err;
> > }
> > +}
> >
> > +if (sev_es_enabled() && !sev_snp_enabled()) {
> > if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
> > error_report("%s: guest policy requires SEV-ES, but "
> > "host SEV-ES support unavailable",
>
> Opps, pre-existing bug here - this method has an 'Error **errp'
> parameter, so should be using 'error_report'.
>
> There are several more examples of this in this method that
> predate your patch series. Can you put a patch at the start
> of this series that fixes them before introducing SNP.
Sure, will add a pre-patch to fix up all the pre-existing issues
you've noted.
-Mike
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
Re: [PATCH v3 23/49] i386/sev: Add a sev_snp_enabled() helper
On Wed, Mar 20, 2024 at 03:39:19AM -0500, Michael Roth wrote:
> Add a simple helper to check if the current guest type is SNP. Also have
> SNP-enabled imply that SEV-ES is enabled as well, and fix up any places
> where the sev_es_enabled() check is expecting a pure/non-SNP guest.
>
> Signed-off-by: Michael Roth
> ---
> target/i386/sev.c | 13 -
> target/i386/sev.h | 2 ++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 7e6dab642a..2eb13ba639 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -933,7 +942,9 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs,
> Error **errp)
> __func__);
> goto err;
> }
> +}
>
> +if (sev_es_enabled() && !sev_snp_enabled()) {
> if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
> error_report("%s: guest policy requires SEV-ES, but "
> "host SEV-ES support unavailable",
Opps, pre-existing bug here - this method has an 'Error **errp'
parameter, so should be using 'error_report'.
There are several more examples of this in this method that
predate your patch series. Can you put a patch at the start
of this series that fixes them before introducing SNP.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH v3 23/49] i386/sev: Add a sev_snp_enabled() helper
Add a simple helper to check if the current guest type is SNP. Also have
SNP-enabled imply that SEV-ES is enabled as well, and fix up any places
where the sev_es_enabled() check is expecting a pure/non-SNP guest.
Signed-off-by: Michael Roth
---
target/i386/sev.c | 13 -
target/i386/sev.h | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 7e6dab642a..2eb13ba639 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -316,12 +316,21 @@ sev_enabled(void)
return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON);
}
+bool
+sev_snp_enabled(void)
+{
+ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
+
+return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST);
+}
+
bool
sev_es_enabled(void)
{
ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
-return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
+return sev_snp_enabled() ||
+(sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
}
uint32_t
@@ -933,7 +942,9 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs,
Error **errp)
__func__);
goto err;
}
+}
+if (sev_es_enabled() && !sev_snp_enabled()) {
if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
error_report("%s: guest policy requires SEV-ES, but "
"host SEV-ES support unavailable",
diff --git a/target/i386/sev.h b/target/i386/sev.h
index bedc667eeb..94295ee74f 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -45,9 +45,11 @@ typedef struct SevKernelLoaderContext {
#ifdef CONFIG_SEV
bool sev_enabled(void);
bool sev_es_enabled(void);
+bool sev_snp_enabled(void);
#else
#define sev_enabled() 0
#define sev_es_enabled() 0
+#define sev_snp_enabled() 0
#endif
uint32_t sev_get_cbit_position(void);
--
2.25.1
