[PATCH 0/4] Qemu SEV reduced-phys-bits fixes

[PATCH 0/4] Qemu SEV reduced-phys-bits fixes

[Tom Lendacky]

This patch series fixes up and tries to remove some confusion around the
SEV reduced-phys-bits parameter.

Based on the "AMD64 Architecture Programmer's Manual Volume 2: System
Programming", section "15.34.6 Page Table Support" [1], a guest should
only ever see a maximum of 1 bit of physical address space reduction.

- Update the documentation, to change the default value from 5 to 1.
- Update the validation of the parameter to ensure the parameter value
  is within the range of the CPUID field that it is reported in. To allow
  for backwards compatibility, especially to support the previously
  documented value of 5, allow the full range of values from 1 to 63
  (0 was never allowed).
- Update the setting of CPUID 0x8000001F_EBX to limit the values to the
  field width that they are setting as an additional safeguard.

[1] https://www.amd.com/system/files/TechDocs/24593.pdf

Tom Lendacky (4):
  qapi, i386/sev: Change the reduced-phys-bits value from 5 to 1
  qemu-options.hx: Update the reduced-phys-bits documentation
  i386/sev: Update checks and information related to reduced-phys-bits
  i386/cpu: Update how the EBX register of CPUID 0x8000001F is set

 qapi/misc-target.json |  2 +-
 qemu-options.hx       |  4 ++--
 target/i386/cpu.c     |  4 ++--
 target/i386/sev.c     | 17 ++++++++++++++---
 4 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.37.3

[PATCH 1/4] qapi, i386/sev: Change the reduced-phys-bits value from 5 to 1

[Tom Lendacky]

A guest only ever experiences, at most, 1 bit of reduced physical
addressing. Change the query-sev-capabilities json comment to use 1.

Fixes: 31dd67f684 ("sev/i386: qmp: add query-sev-capabilities command")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 qapi/misc-target.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4944c0528f..398fd09f25 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -172,7 +172,7 @@
 # -> { "execute": "query-sev-capabilities" }
 # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
 #                  "cpu0-id": "2lvmGwo+...61iEinw==",
-#                  "cbitpos": 47, "reduced-phys-bits": 5}}
+#                  "cbitpos": 47, "reduced-phys-bits": 1}}
 #
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
-- 
2.37.3

Re: [PATCH 1/4] qapi, i386/sev: Change the reduced-phys-bits value from 5 to 1

[Dr. David Alan Gilbert]

* Tom Lendacky (thomas.lendacky@amd.com) wrote:
> A guest only ever experiences, at most, 1 bit of reduced physical
> addressing. Change the query-sev-capabilities json comment to use 1.
> 
> Fixes: 31dd67f684 ("sev/i386: qmp: add query-sev-capabilities command")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qapi/misc-target.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 4944c0528f..398fd09f25 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -172,7 +172,7 @@
>  # -> { "execute": "query-sev-capabilities" }
>  # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
>  #                  "cpu0-id": "2lvmGwo+...61iEinw==",
> -#                  "cbitpos": 47, "reduced-phys-bits": 5}}
> +#                  "cbitpos": 47, "reduced-phys-bits": 1}}
>  #
>  ##
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
> -- 
> 2.37.3
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/4] qemu-options.hx: Update the reduced-phys-bits documentation

[Tom Lendacky]

A guest only ever experiences, at most, 1 bit of reduced physical
addressing. Update the documentation to reflect this as well as change
the example value on the reduced-phys-bits option.

Fixes: a9b4942f48 ("target/i386: add Secure Encrypted Virtualization (SEV) object")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 qemu-options.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 913c71e38f..3396085cf0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5391,7 +5391,7 @@ SRST
         physical address space. The ``reduced-phys-bits`` is used to
         provide the number of bits we loose in physical address space.
         Similar to C-bit, the value is Host family dependent. On EPYC,
-        the value should be 5.
+        a guest will lose a maximum of 1 bit, so the value should be 1.
 
         The ``sev-device`` provides the device file to use for
         communicating with the SEV firmware running inside AMD Secure
@@ -5426,7 +5426,7 @@ SRST
 
              # |qemu_system_x86| \\
                  ...... \\
-                 -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \\
+                 -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \\
                  -machine ...,memory-encryption=sev0 \\
                  .....
 
-- 
2.37.3

Re: [PATCH 2/4] qemu-options.hx: Update the reduced-phys-bits documentation

[Dr. David Alan Gilbert]

* Tom Lendacky (thomas.lendacky@amd.com) wrote:
> A guest only ever experiences, at most, 1 bit of reduced physical
> addressing. Update the documentation to reflect this as well as change
> the example value on the reduced-phys-bits option.
> 
> Fixes: a9b4942f48 ("target/i386: add Secure Encrypted Virtualization (SEV) object")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qemu-options.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 913c71e38f..3396085cf0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5391,7 +5391,7 @@ SRST
>          physical address space. The ``reduced-phys-bits`` is used to
>          provide the number of bits we loose in physical address space.
>          Similar to C-bit, the value is Host family dependent. On EPYC,
> -        the value should be 5.
> +        a guest will lose a maximum of 1 bit, so the value should be 1.
>  
>          The ``sev-device`` provides the device file to use for
>          communicating with the SEV firmware running inside AMD Secure
> @@ -5426,7 +5426,7 @@ SRST
>  
>               # |qemu_system_x86| \\
>                   ...... \\
> -                 -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \\
> +                 -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \\
>                   -machine ...,memory-encryption=sev0 \\
>                   .....
>  
> -- 
> 2.37.3
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 3/4] i386/sev: Update checks and information related to reduced-phys-bits

[Tom Lendacky]

The value of the reduced-phys-bits parameter is propogated to the CPUID
information exposed to the guest. Update the current validation check to
account for the size of the CPUID field (6-bits), ensuring the value is
in the range of 1 to 63.

Maintain backward compatibility, to an extent, by allowing a value greater
than 1 (so that the previously documented value of 5 still works), but not
allowing anything over 63.

Fixes: d8575c6c02 ("sev/i386: add command to initialize the memory encryption context")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 target/i386/sev.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 32f7dbac4e..78c2d37eba 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -932,15 +932,26 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
     host_cbitpos = ebx & 0x3f;
 
+    /*
+     * The cbitpos value will be placed in bit positions 5:0 of the EBX
+     * register of CPUID 0x8000001F. No need to verify the range as the
+     * comparison against the host value accomplishes that.
+     */
     if (host_cbitpos != sev->cbitpos) {
         error_setg(errp, "%s: cbitpos check failed, host '%d' requested '%d'",
                    __func__, host_cbitpos, sev->cbitpos);
         goto err;
     }
 
-    if (sev->reduced_phys_bits < 1) {
-        error_setg(errp, "%s: reduced_phys_bits check failed, it should be >=1,"
-                   " requested '%d'", __func__, sev->reduced_phys_bits);
+    /*
+     * The reduced-phys-bits value will be placed in bit positions 11:6 of
+     * the EBX register of CPUID 0x8000001F, so verify the supplied value
+     * is in the range of 1 to 63.
+     */
+    if (sev->reduced_phys_bits < 1 || sev->reduced_phys_bits > 63) {
+        error_setg(errp, "%s: reduced_phys_bits check failed,"
+                   " it should be in the range of 1 to 63, requested '%d'",
+                   __func__, sev->reduced_phys_bits);
         goto err;
     }
 
-- 
2.37.3

Re: [PATCH 3/4] i386/sev: Update checks and information related to reduced-phys-bits

[Dr. David Alan Gilbert]

* Tom Lendacky (thomas.lendacky@amd.com) wrote:
> The value of the reduced-phys-bits parameter is propogated to the CPUID
> information exposed to the guest. Update the current validation check to
> account for the size of the CPUID field (6-bits), ensuring the value is
> in the range of 1 to 63.
> 
> Maintain backward compatibility, to an extent, by allowing a value greater
> than 1 (so that the previously documented value of 5 still works), but not
> allowing anything over 63.
> 
> Fixes: d8575c6c02 ("sev/i386: add command to initialize the memory encryption context")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 32f7dbac4e..78c2d37eba 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -932,15 +932,26 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
>      host_cbitpos = ebx & 0x3f;
>  
> +    /*
> +     * The cbitpos value will be placed in bit positions 5:0 of the EBX
> +     * register of CPUID 0x8000001F. No need to verify the range as the
> +     * comparison against the host value accomplishes that.
> +     */
>      if (host_cbitpos != sev->cbitpos) {
>          error_setg(errp, "%s: cbitpos check failed, host '%d' requested '%d'",
>                     __func__, host_cbitpos, sev->cbitpos);
>          goto err;
>      }
>  
> -    if (sev->reduced_phys_bits < 1) {
> -        error_setg(errp, "%s: reduced_phys_bits check failed, it should be >=1,"
> -                   " requested '%d'", __func__, sev->reduced_phys_bits);
> +    /*
> +     * The reduced-phys-bits value will be placed in bit positions 11:6 of
> +     * the EBX register of CPUID 0x8000001F, so verify the supplied value
> +     * is in the range of 1 to 63.
> +     */
> +    if (sev->reduced_phys_bits < 1 || sev->reduced_phys_bits > 63) {
> +        error_setg(errp, "%s: reduced_phys_bits check failed,"
> +                   " it should be in the range of 1 to 63, requested '%d'",
> +                   __func__, sev->reduced_phys_bits);
>          goto err;
>      }
>  
> -- 
> 2.37.3
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 4/4] i386/cpu: Update how the EBX register of CPUID 0x8000001F is set

[Tom Lendacky]

Update the setting of CPUID 0x8000001F EBX to clearly document the ranges
associated with fields being set.

Fixes: 6cb8f2a663 ("cpu/i386: populate CPUID 0x8000_001F when SEV is active")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 target/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1db1278a59..d4b806cfec 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5853,8 +5853,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (sev_enabled()) {
             *eax = 0x2;
             *eax |= sev_es_enabled() ? 0x8 : 0;
-            *ebx = sev_get_cbit_position();
-            *ebx |= sev_get_reduced_phys_bits() << 6;
+            *ebx = sev_get_cbit_position() & 0x3f; /* EBX[5:0] */
+            *ebx |= (sev_get_reduced_phys_bits() & 0x3f) << 6; /* EBX[11:6] */
         }
         break;
     default:
-- 
2.37.3

Re: [PATCH 4/4] i386/cpu: Update how the EBX register of CPUID 0x8000001F is set

[Dr. David Alan Gilbert]

* Tom Lendacky (thomas.lendacky@amd.com) wrote:
> Update the setting of CPUID 0x8000001F EBX to clearly document the ranges
> associated with fields being set.
> 
> Fixes: 6cb8f2a663 ("cpu/i386: populate CPUID 0x8000_001F when SEV is active")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1db1278a59..d4b806cfec 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5853,8 +5853,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          if (sev_enabled()) {
>              *eax = 0x2;
>              *eax |= sev_es_enabled() ? 0x8 : 0;
> -            *ebx = sev_get_cbit_position();
> -            *ebx |= sev_get_reduced_phys_bits() << 6;
> +            *ebx = sev_get_cbit_position() & 0x3f; /* EBX[5:0] */
> +            *ebx |= (sev_get_reduced_phys_bits() & 0x3f) << 6; /* EBX[11:6] */
>          }
>          break;
>      default:
> -- 
> 2.37.3
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/4] Qemu SEV reduced-phys-bits fixes

[Tom Lendacky]

On 9/30/22 10:14, Tom Lendacky wrote:
> This patch series fixes up and tries to remove some confusion around the
> SEV reduced-phys-bits parameter.
> 
> Based on the "AMD64 Architecture Programmer's Manual Volume 2: System
> Programming", section "15.34.6 Page Table Support" [1], a guest should
> only ever see a maximum of 1 bit of physical address space reduction.
> 
> - Update the documentation, to change the default value from 5 to 1.
> - Update the validation of the parameter to ensure the parameter value
>    is within the range of the CPUID field that it is reported in. To allow
>    for backwards compatibility, especially to support the previously
>    documented value of 5, allow the full range of values from 1 to 63
>    (0 was never allowed).
> - Update the setting of CPUID 0x8000001F_EBX to limit the values to the
>    field width that they are setting as an additional safeguard.
> 
> [1] https://www.amd.com/system/files/TechDocs/24593.pdf

Ping, any concerns with this series?

Thanks,
Tom

> 
> Tom Lendacky (4):
>    qapi, i386/sev: Change the reduced-phys-bits value from 5 to 1
>    qemu-options.hx: Update the reduced-phys-bits documentation
>    i386/sev: Update checks and information related to reduced-phys-bits
>    i386/cpu: Update how the EBX register of CPUID 0x8000001F is set
> 
>   qapi/misc-target.json |  2 +-
>   qemu-options.hx       |  4 ++--
>   target/i386/cpu.c     |  4 ++--
>   target/i386/sev.c     | 17 ++++++++++++++---
>   4 files changed, 19 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/4] Qemu SEV reduced-phys-bits fixes

[Daniel P. Berrangé]

On Wed, Jan 04, 2023 at 12:13:09PM -0600, Tom Lendacky wrote:
> On 9/30/22 10:14, Tom Lendacky wrote:
> > This patch series fixes up and tries to remove some confusion around the
> > SEV reduced-phys-bits parameter.
> > 
> > Based on the "AMD64 Architecture Programmer's Manual Volume 2: System
> > Programming", section "15.34.6 Page Table Support" [1], a guest should
> > only ever see a maximum of 1 bit of physical address space reduction.
> > 
> > - Update the documentation, to change the default value from 5 to 1.
> > - Update the validation of the parameter to ensure the parameter value
> >    is within the range of the CPUID field that it is reported in. To allow
> >    for backwards compatibility, especially to support the previously
> >    documented value of 5, allow the full range of values from 1 to 63
> >    (0 was never allowed).
> > - Update the setting of CPUID 0x8000001F_EBX to limit the values to the
> >    field width that they are setting as an additional safeguard.
> > 
> > [1] https://www.amd.com/system/files/TechDocs/24593.pdf
> 
> Ping, any concerns with this series?

Looks like you got postive review from David in Oct, so this
needs one of the x86 maintainers to queue the series.

> > Tom Lendacky (4):
> >    qapi, i386/sev: Change the reduced-phys-bits value from 5 to 1
> >    qemu-options.hx: Update the reduced-phys-bits documentation
> >    i386/sev: Update checks and information related to reduced-phys-bits
> >    i386/cpu: Update how the EBX register of CPUID 0x8000001F is set
> > 
> >   qapi/misc-target.json |  2 +-
> >   qemu-options.hx       |  4 ++--
> >   target/i386/cpu.c     |  4 ++--
> >   target/i386/sev.c     | 17 ++++++++++++++---
> >   4 files changed, 19 insertions(+), 8 deletions(-)
> > 
> 

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 0/4] Qemu SEV reduced-phys-bits fixes

[Paolo Bonzini]

Queued, thanks.

Paolo