[PATCH 0/2] expose host-phys-bits to guest

[PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

When the guest (firmware specifically) knows how big
the address space actually is it can be used better.

Some more background:
  https://bugzilla.redhat.com/show_bug.cgi?id=2084533

This is a RfC series exposes the information via cpuid.

take care,
  Gerd

Gerd Hoffmann (2):
  [hack] reserve bit KVM_HINTS_HOST_PHYS_BITS
  [RfC] expose host-phys-bits to guest

 include/standard-headers/asm-x86/kvm_para.h | 3 ++-
 target/i386/cpu.h                           | 3 ---
 hw/i386/microvm.c                           | 6 +++++-
 target/i386/cpu.c                           | 3 +--
 target/i386/host-cpu.c                      | 4 +++-
 target/i386/kvm/kvm.c                       | 1 +
 6 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.37.2

[PATCH 1/2] [hack] reserve bit KVM_HINTS_HOST_PHYS_BITS

[Gerd Hoffmann]

The KVM_HINTS_HOST_PHYS_BITS bit indicates that qemu has host-phys-bits
turned on.  This implies the guest can actually work with the full
available physical address space as advertised by CPUID(0x80000008).

Temporary hack for RfC patch and testing.  This change must actually be
done in the linux kernel, then picked up by qemu via header file sync.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/standard-headers/asm-x86/kvm_para.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/asm-x86/kvm_para.h b/include/standard-headers/asm-x86/kvm_para.h
index f0235e58a1d3..105b958c0f56 100644
--- a/include/standard-headers/asm-x86/kvm_para.h
+++ b/include/standard-headers/asm-x86/kvm_para.h
@@ -37,7 +37,8 @@
 #define KVM_FEATURE_HC_MAP_GPA_RANGE	16
 #define KVM_FEATURE_MIGRATION_CONTROL	17
 
-#define KVM_HINTS_REALTIME      0
+#define KVM_HINTS_REALTIME              0
+#define KVM_HINTS_HOST_PHYS_BITS        1
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.
-- 
2.37.2

[PATCH 2/2] [RfC] expose host-phys-bits to guest

[Gerd Hoffmann]

Move "host-phys-bits" property from cpu->host_phys_bits to
cpu->env.features[FEAT_KVM_HINTS] (KVM_HINTS_HOST_PHYS_BITS).

This has the effect that the guest can see whenever host-phys-bits
is turned on or not and act accordingly.

Current mode of operation for firmware is to be conservative with
address space usage because is impossible to figure how much is
actually available.  This patch allows the firmware to use the full
physical address space available (with host-phys-bits=on).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 target/i386/cpu.h      | 3 ---
 hw/i386/microvm.c      | 6 +++++-
 target/i386/cpu.c      | 3 +--
 target/i386/host-cpu.c | 4 +++-
 target/i386/kvm/kvm.c  | 1 +
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 82004b65b944..b9c6d3d9cac6 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1898,9 +1898,6 @@ struct ArchCPU {
     /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
     bool fill_mtrr_mask;
 
-    /* if true override the phys_bits value with a value read from the host */
-    bool host_phys_bits;
-
     /* if set, limit maximum value for phys_bits when host_phys_bits is true */
     uint8_t host_phys_bits_limit;
 
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 7fe8cce03e92..edb1d4cbcbc1 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -54,6 +54,8 @@
 #include "kvm/kvm_i386.h"
 #include "hw/xen/start_info.h"
 
+#include "standard-headers/asm-x86/kvm_para.h"
+
 #define MICROVM_QBOOT_FILENAME "qboot.rom"
 #define MICROVM_BIOS_FILENAME  "bios-microvm.bin"
 
@@ -424,7 +426,9 @@ static void microvm_device_pre_plug_cb(HotplugHandler *hotplug_dev,
 {
     X86CPU *cpu = X86_CPU(dev);
 
-    cpu->host_phys_bits = true; /* need reliable phys-bits */
+    /* need reliable phys-bits */
+    cpu->env.features[FEAT_KVM_HINTS] |= (1 << KVM_HINTS_HOST_PHYS_BITS);
+
     x86_cpu_pre_plug(hotplug_dev, dev, errp);
 }
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1db1278a599b..d60f4498a3c3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -778,7 +778,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
     [FEAT_KVM_HINTS] = {
         .type = CPUID_FEATURE_WORD,
         .feat_names = {
-            "kvm-hint-dedicated", NULL, NULL, NULL,
+            "kvm-hint-dedicated", "host-phys-bits", NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
@@ -7016,7 +7016,6 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
-    DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
     DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU, host_phys_bits_limit, 0),
     DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
     DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 10f8aba86e53..30e9dd9f66f1 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -13,6 +13,8 @@
 #include "qapi/error.h"
 #include "sysemu/sysemu.h"
 
+#include "standard-headers/asm-x86/kvm_para.h"
+
 /* Note: Only safe for use on x86(-64) hosts */
 static uint32_t host_cpu_phys_bits(void)
 {
@@ -68,7 +70,7 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
         warned = true;
     }
 
-    if (cpu->host_phys_bits) {
+    if (cpu->env.features[FEAT_KVM_HINTS] & (1 << KVM_HINTS_HOST_PHYS_BITS)) {
         /* The user asked for us to use the host physical bits */
         phys_bits = host_phys_bits;
         if (cpu->host_phys_bits_limit &&
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index f148a6d52fa4..182a70c98d35 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -459,6 +459,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         }
     } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
         ret |= 1U << KVM_HINTS_REALTIME;
+        ret |= 1U << KVM_HINTS_HOST_PHYS_BITS;
     }
 
     return ret;
-- 
2.37.2

Re: [PATCH 2/2] [RfC] expose host-phys-bits to guest

[Claudio Fontana]

Ccing some folks, since we are just now adding host bits awareness through the higher level tools.

On 8/31/22 14:50, Gerd Hoffmann wrote:
> Move "host-phys-bits" property from cpu->host_phys_bits to
> cpu->env.features[FEAT_KVM_HINTS] (KVM_HINTS_HOST_PHYS_BITS).
> 
> This has the effect that the guest can see whenever host-phys-bits
> is turned on or not and act accordingly.
> 
> Current mode of operation for firmware is to be conservative with
> address space usage because is impossible to figure how much is
> actually available.  This patch allows the firmware to use the full
> physical address space available (with host-phys-bits=on).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  target/i386/cpu.h      | 3 ---
>  hw/i386/microvm.c      | 6 +++++-
>  target/i386/cpu.c      | 3 +--
>  target/i386/host-cpu.c | 4 +++-
>  target/i386/kvm/kvm.c  | 1 +
>  5 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 82004b65b944..b9c6d3d9cac6 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1898,9 +1898,6 @@ struct ArchCPU {
>      /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
>      bool fill_mtrr_mask;
>  
> -    /* if true override the phys_bits value with a value read from the host */
> -    bool host_phys_bits;
> -
>      /* if set, limit maximum value for phys_bits when host_phys_bits is true */
>      uint8_t host_phys_bits_limit;
>  
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 7fe8cce03e92..edb1d4cbcbc1 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -54,6 +54,8 @@
>  #include "kvm/kvm_i386.h"
>  #include "hw/xen/start_info.h"
>  
> +#include "standard-headers/asm-x86/kvm_para.h"
> +
>  #define MICROVM_QBOOT_FILENAME "qboot.rom"
>  #define MICROVM_BIOS_FILENAME  "bios-microvm.bin"
>  
> @@ -424,7 +426,9 @@ static void microvm_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>  {
>      X86CPU *cpu = X86_CPU(dev);
>  
> -    cpu->host_phys_bits = true; /* need reliable phys-bits */
> +    /* need reliable phys-bits */
> +    cpu->env.features[FEAT_KVM_HINTS] |= (1 << KVM_HINTS_HOST_PHYS_BITS);
> +
>      x86_cpu_pre_plug(hotplug_dev, dev, errp);
>  }
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1db1278a599b..d60f4498a3c3 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -778,7 +778,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>      [FEAT_KVM_HINTS] = {
>          .type = CPUID_FEATURE_WORD,
>          .feat_names = {
> -            "kvm-hint-dedicated", NULL, NULL, NULL,
> +            "kvm-hint-dedicated", "host-phys-bits", NULL, NULL,
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
> @@ -7016,7 +7016,6 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
>      DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
> -    DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>      DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU, host_phys_bits_limit, 0),
>      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
>      DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 10f8aba86e53..30e9dd9f66f1 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -13,6 +13,8 @@
>  #include "qapi/error.h"
>  #include "sysemu/sysemu.h"
>  
> +#include "standard-headers/asm-x86/kvm_para.h"
> +
>  /* Note: Only safe for use on x86(-64) hosts */
>  static uint32_t host_cpu_phys_bits(void)
>  {
> @@ -68,7 +70,7 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
>          warned = true;
>      }
>  
> -    if (cpu->host_phys_bits) {
> +    if (cpu->env.features[FEAT_KVM_HINTS] & (1 << KVM_HINTS_HOST_PHYS_BITS)) {
>          /* The user asked for us to use the host physical bits */
>          phys_bits = host_phys_bits;
>          if (cpu->host_phys_bits_limit &&
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index f148a6d52fa4..182a70c98d35 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -459,6 +459,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>          }
>      } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
>          ret |= 1U << KVM_HINTS_REALTIME;
> +        ret |= 1U << KVM_HINTS_HOST_PHYS_BITS;
>      }
>  
>      return ret;

Re: [PATCH 1/2] [hack] reserve bit KVM_HINTS_HOST_PHYS_BITS

[Michael S. Tsirkin]

On Wed, Aug 31, 2022 at 02:50:58PM +0200, Gerd Hoffmann wrote:
> The KVM_HINTS_HOST_PHYS_BITS bit indicates that qemu has host-phys-bits
> turned on.  This implies the guest can actually work with the full
> available physical address space as advertised by CPUID(0x80000008).
> 
> Temporary hack for RfC patch and testing.  This change must actually be
> done in the linux kernel, then picked up by qemu via header file sync.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/standard-headers/asm-x86/kvm_para.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/standard-headers/asm-x86/kvm_para.h b/include/standard-headers/asm-x86/kvm_para.h
> index f0235e58a1d3..105b958c0f56 100644
> --- a/include/standard-headers/asm-x86/kvm_para.h
> +++ b/include/standard-headers/asm-x86/kvm_para.h
> @@ -37,7 +37,8 @@
>  #define KVM_FEATURE_HC_MAP_GPA_RANGE	16
>  #define KVM_FEATURE_MIGRATION_CONTROL	17
>  
> -#define KVM_HINTS_REALTIME      0
> +#define KVM_HINTS_REALTIME              0
> +#define KVM_HINTS_HOST_PHYS_BITS        1
>  
>  /* The last 8 bits are used to indicate how to interpret the flags field
>   * in pvclock structure. If no bits are set, all flags are ignored.


Just a clarification.
I think what the bit means is that physical address size
data in cpuid is valid.
Accordingly I would rename it to

+#define KVM_HINTS_PHYS_ADDRESS_SIZE_DATA_VALID        1


or some such.


> -- 
> 2.37.2

Re: [PATCH 0/2] expose host-phys-bits to guest

[Xiaoyao Li]

On 8/31/2022 8:50 PM, Gerd Hoffmann wrote:
> When the guest (firmware specifically) knows how big
> the address space actually is it can be used better.
> 
> Some more background:
>    https://bugzilla.redhat.com/show_bug.cgi?id=2084533

QEMU enables host-phys-bits for "-cpu host/max" in 
host_cpu_max_instance_init();

I think the problem is for all the named CPU model, that they don't have 
phys_bits defined. Thus they all have "cpu->phys-bits == 0", which leads 
to cpu->phys_bits = TCG_PHYS_ADDR_BITS (36 for 32-bits build and 40 for 
64-bits build)

Anyway, IMO, guest including guest firmware, should always consult from 
CPUID leaf 0x80000008 for physical address length. Tt is the duty of 
userspace VMM, here QEMU, to ensure VM's host physical address length 
not exceeding host's. If userspace VMM cannot ensure this, guest is 
likely hitting problem.

> This is a RfC series exposes the information via cpuid.
> 
> take care,
>    Gerd
> 
> Gerd Hoffmann (2):
>    [hack] reserve bit KVM_HINTS_HOST_PHYS_BITS
>    [RfC] expose host-phys-bits to guest
> 
>   include/standard-headers/asm-x86/kvm_para.h | 3 ++-
>   target/i386/cpu.h                           | 3 ---
>   hw/i386/microvm.c                           | 6 +++++-
>   target/i386/cpu.c                           | 3 +--
>   target/i386/host-cpu.c                      | 4 +++-
>   target/i386/kvm/kvm.c                       | 1 +
>   6 files changed, 12 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

  Hi,

> I think the problem is for all the named CPU model, that they don't have
> phys_bits defined. Thus they all have "cpu->phys-bits == 0", which leads to
> cpu->phys_bits = TCG_PHYS_ADDR_BITS (36 for 32-bits build and 40 for 64-bits
> build)

Exactly.  And if you run on hardware with phys-bits being 36 or 39
(common for intel desktop processors) things explode when the guest
tries to use the whole range.

> Anyway, IMO, guest including guest firmware, should always consult from
> CPUID leaf 0x80000008 for physical address length.

It simply can't for the reason outlined above.  Even if we fix qemu
today that doesn't solve the problem for the firmware because we want
backward compatibility with older qemu versions.  Thats why I want the
extra bit which essentially says "CPUID leaf 0x80000008 actually works".

take care,
  Gerd

Re: [PATCH 0/2] expose host-phys-bits to guest

[Xiaoyao Li]

On 9/1/2022 9:58 PM, Gerd Hoffmann wrote:

>> Anyway, IMO, guest including guest firmware, should always consult from
>> CPUID leaf 0x80000008 for physical address length.
> 
> It simply can't for the reason outlined above.  Even if we fix qemu
> today that doesn't solve the problem for the firmware because we want
> backward compatibility with older qemu versions.  Thats why I want the
> extra bit which essentially says "CPUID leaf 0x80000008 actually works".

I don't understand how it backward compatible with older qemu version. 
Old QEMU won't set the extra bit you introduced in this series, and all 
the guest created with old QEMU will become untrusted on CPUID leaf 
0x80000008 ?

> take care,
>    Gerd
> 

Re: [PATCH 0/2] expose host-phys-bits to guest

[Claudio Fontana]

On 9/1/22 08:07, Xiaoyao Li wrote:
> On 8/31/2022 8:50 PM, Gerd Hoffmann wrote:
>> When the guest (firmware specifically) knows how big
>> the address space actually is it can be used better.
>>
>> Some more background:
>>    https://bugzilla.redhat.com/show_bug.cgi?id=2084533
> 
> QEMU enables host-phys-bits for "-cpu host/max" in 
> host_cpu_max_instance_init();

No, in host_cpu_max_instance_init the default for host-phys-bits is set to on.

You can still get the phys bits adjusted if you set the property to on manually for other cpu models.

> 
> I think the problem is for all the named CPU model, that they don't have 
> phys_bits defined. Thus they all have "cpu->phys-bits == 0", which leads 
> to cpu->phys_bits = TCG_PHYS_ADDR_BITS (36 for 32-bits build and 40 for 
> 64-bits build)
> 
> Anyway, IMO, guest including guest firmware, should always consult from 
> CPUID leaf 0x80000008 for physical address length. Tt is the duty of 
> userspace VMM, here QEMU, to ensure VM's host physical address length 
> not exceeding host's. If userspace VMM cannot ensure this, guest is 
> likely hitting problem.
> 
>> This is a RfC series exposes the information via cpuid.
>>
>> take care,
>>    Gerd
>>
>> Gerd Hoffmann (2):
>>    [hack] reserve bit KVM_HINTS_HOST_PHYS_BITS
>>    [RfC] expose host-phys-bits to guest
>>
>>   include/standard-headers/asm-x86/kvm_para.h | 3 ++-
>>   target/i386/cpu.h                           | 3 ---
>>   hw/i386/microvm.c                           | 6 +++++-
>>   target/i386/cpu.c                           | 3 +--
>>   target/i386/host-cpu.c                      | 4 +++-
>>   target/i386/kvm/kvm.c                       | 1 +
>>   6 files changed, 12 insertions(+), 8 deletions(-)
>>
> 
> 

Re: [PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

On Thu, Sep 01, 2022 at 10:36:19PM +0800, Xiaoyao Li wrote:
> On 9/1/2022 9:58 PM, Gerd Hoffmann wrote:
> 
> > > Anyway, IMO, guest including guest firmware, should always consult from
> > > CPUID leaf 0x80000008 for physical address length.
> > 
> > It simply can't for the reason outlined above.  Even if we fix qemu
> > today that doesn't solve the problem for the firmware because we want
> > backward compatibility with older qemu versions.  Thats why I want the
> > extra bit which essentially says "CPUID leaf 0x80000008 actually works".
> 
> I don't understand how it backward compatible with older qemu version. Old
> QEMU won't set the extra bit you introduced in this series, and all the
> guest created with old QEMU will become untrusted on CPUID leaf 0x80000008 ?

Correct, on old qemu firmware will not trust CPUID leaf 0x80000008.
That is not worse than the situation we have today, currently the
firmware never trusts CPUID leaf 0x80000008.

So the patches will improves the situation for new qemu only, but I
don't see a way around that.

take care,
  Gerd

Re: [PATCH 0/2] expose host-phys-bits to guest

[Xiaoyao Li]

On 9/2/2022 12:17 AM, Gerd Hoffmann wrote:
> On Thu, Sep 01, 2022 at 10:36:19PM +0800, Xiaoyao Li wrote:
>> On 9/1/2022 9:58 PM, Gerd Hoffmann wrote:
>>
>>>> Anyway, IMO, guest including guest firmware, should always consult from
>>>> CPUID leaf 0x80000008 for physical address length.
>>>
>>> It simply can't for the reason outlined above.  Even if we fix qemu
>>> today that doesn't solve the problem for the firmware because we want
>>> backward compatibility with older qemu versions.  Thats why I want the
>>> extra bit which essentially says "CPUID leaf 0x80000008 actually works".
>>
>> I don't understand how it backward compatible with older qemu version. Old
>> QEMU won't set the extra bit you introduced in this series, and all the
>> guest created with old QEMU will become untrusted on CPUID leaf 0x80000008 ?
> 
> Correct, on old qemu firmware will not trust CPUID leaf 0x80000008.
> That is not worse than the situation we have today, currently the
> firmware never trusts CPUID leaf 0x80000008.
> 
> So the patches will improves the situation for new qemu only, but I
> don't see a way around that.
> 

I see.

But IMHO, I don't think it's good that guest firmware workaround the 
issue on its own. Instead, it's better to just trust CPUID leaf 
0x80000008 and fail if the given physical address length cannot be 
virtualized/supported.

It's just the bug of VMM to virtualize the physical address length. The 
correction direction is to fix the bug not the workaround to hide the bug.

Re: [PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

On Fri, Sep 02, 2022 at 08:10:00AM +0800, Xiaoyao Li wrote:
> On 9/2/2022 12:17 AM, Gerd Hoffmann wrote:
> > On Thu, Sep 01, 2022 at 10:36:19PM +0800, Xiaoyao Li wrote:
> > > On 9/1/2022 9:58 PM, Gerd Hoffmann wrote:
> > > 
> > > > > Anyway, IMO, guest including guest firmware, should always consult from
> > > > > CPUID leaf 0x80000008 for physical address length.
> > > > 
> > > > It simply can't for the reason outlined above.  Even if we fix qemu
> > > > today that doesn't solve the problem for the firmware because we want
> > > > backward compatibility with older qemu versions.  Thats why I want the
> > > > extra bit which essentially says "CPUID leaf 0x80000008 actually works".
> > > 
> > > I don't understand how it backward compatible with older qemu version. Old
> > > QEMU won't set the extra bit you introduced in this series, and all the
> > > guest created with old QEMU will become untrusted on CPUID leaf 0x80000008 ?
> > 
> > Correct, on old qemu firmware will not trust CPUID leaf 0x80000008.
> > That is not worse than the situation we have today, currently the
> > firmware never trusts CPUID leaf 0x80000008.
> > 
> > So the patches will improves the situation for new qemu only, but I
> > don't see a way around that.
> > 
> 
> I see.
> 
> But IMHO, I don't think it's good that guest firmware workaround the issue
> on its own. Instead, it's better to just trust CPUID leaf 0x80000008 and
> fail if the given physical address length cannot be virtualized/supported.
> 
> It's just the bug of VMM to virtualize the physical address length. The
> correction direction is to fix the bug not the workaround to hide the bug.

I'm starting to repeat myself. "just trust CPUID leaf 0x80000008"
doesn't work because you simply can't with current qemu versions.

I don't like the dance with the new bit very much either, but I don't
see a better way without massive fallout due to compatibility problems.
I'm open to suggestions though.

take care,
  Gerd

Re: [PATCH 0/2] expose host-phys-bits to guest

[Michael S. Tsirkin]

On Fri, Sep 02, 2022 at 08:07:20AM +0200, Gerd Hoffmann wrote:
> On Fri, Sep 02, 2022 at 08:10:00AM +0800, Xiaoyao Li wrote:
> > On 9/2/2022 12:17 AM, Gerd Hoffmann wrote:
> > > On Thu, Sep 01, 2022 at 10:36:19PM +0800, Xiaoyao Li wrote:
> > > > On 9/1/2022 9:58 PM, Gerd Hoffmann wrote:
> > > > 
> > > > > > Anyway, IMO, guest including guest firmware, should always consult from
> > > > > > CPUID leaf 0x80000008 for physical address length.
> > > > > 
> > > > > It simply can't for the reason outlined above.  Even if we fix qemu
> > > > > today that doesn't solve the problem for the firmware because we want
> > > > > backward compatibility with older qemu versions.  Thats why I want the
> > > > > extra bit which essentially says "CPUID leaf 0x80000008 actually works".
> > > > 
> > > > I don't understand how it backward compatible with older qemu version. Old
> > > > QEMU won't set the extra bit you introduced in this series, and all the
> > > > guest created with old QEMU will become untrusted on CPUID leaf 0x80000008 ?
> > > 
> > > Correct, on old qemu firmware will not trust CPUID leaf 0x80000008.
> > > That is not worse than the situation we have today, currently the
> > > firmware never trusts CPUID leaf 0x80000008.
> > > 
> > > So the patches will improves the situation for new qemu only, but I
> > > don't see a way around that.
> > > 
> > 
> > I see.
> > 
> > But IMHO, I don't think it's good that guest firmware workaround the issue
> > on its own. Instead, it's better to just trust CPUID leaf 0x80000008 and
> > fail if the given physical address length cannot be virtualized/supported.
> > 
> > It's just the bug of VMM to virtualize the physical address length. The
> > correction direction is to fix the bug not the workaround to hide the bug.
> 
> I'm starting to repeat myself. "just trust CPUID leaf 0x80000008"
> doesn't work because you simply can't with current qemu versions.
> 
> I don't like the dance with the new bit very much either, but I don't
> see a better way without massive fallout due to compatibility problems.
> I'm open to suggestions though.
> 
> take care,
>   Gerd


I feel there are three major sources of controversy here

0. the cover letter and subject don't do such a good job
   explaining that what we are doing is just telling guest
   CPUID is not broken. we are not exposing anything new
   and not exposing host capability to guest, for example,
   if cpuid phys address is smaller than host things also
   work fine.

1. really the naming.  We need to be more explicit that it's just a bugfix.

2. down the road we will want to switch the default when no PV. however,
   some hosts might still want conservative firmware for compatibility
   reasons, so I think we need a way to tell firmware
   "ignore phys address width in CPUID like you did in the past".
   let's add a flag for that?
   and if none are set firmware should print a warning, though I
   do not know how many people will see that. Maybe some ;)

along the lines of:

/*
 * Old KVM hosts often reported incorrect phys address width,
 * so firmware had to be very conservative in its use of physical
 * addresses. 
 * One of the two following flags should be set.
 * If none are set firmware is for now conservative, but that will
 * likely change in the future, hosts should not rely on that.
 */
/* 
/* KVM with non broken phys address width should set this flag
 * firmware will be allowed to use all phys address bits
 */
#define KVM_BUG_PHYS_ADDRESS_WIDTH_NONBROKEN 1
/*
 * Force firmware to be very conservative in its use of physical
 * addresses, ignoring phys address width in CPUID.
 * Helpful for migration between hosts with different capabilities.
 */
#define KVM_BUG_PHYS_ADDRESS_WIDTH_BROKEN 2

-- 
MST

Re: [PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

  Hi,
 
> I feel there are three major sources of controversy here
> 
> 0. the cover letter and subject don't do such a good job
>    explaining that what we are doing is just telling guest
>    CPUID is not broken. we are not exposing anything new
>    and not exposing host capability to guest, for example,
>    if cpuid phys address is smaller than host things also
>    work fine.
> 
> 1. really the naming.  We need to be more explicit that it's just a bugfix.

Yep, I'll go improve that for v2.

> 2. down the road we will want to switch the default when no PV. however,
>    some hosts might still want conservative firmware for compatibility
>    reasons, so I think we need a way to tell firmware
>    "ignore phys address width in CPUID like you did in the past".
>    let's add a flag for that?
>    and if none are set firmware should print a warning, though I
>    do not know how many people will see that. Maybe some ;)

> /*
>  * Force firmware to be very conservative in its use of physical
>  * addresses, ignoring phys address width in CPUID.
>  * Helpful for migration between hosts with different capabilities.
>  */
> #define KVM_BUG_PHYS_ADDRESS_WIDTH_BROKEN 2

I don't see a need for that.  Live migration compatibility can be
handled just fine today using
	'host-phys-bits=on,host-phys-bits-limit=<xx>'

Which is simliar to 'phys-bits=<xx>'.

The important difference is that phys-bits allows pretty much anything
whereas host-phys-bits-limit applies sanity checks against the host
supported phys bits and throws error on invalid values.

take care,
  Gerd

Re: [PATCH 0/2] expose host-phys-bits to guest

[Michael S. Tsirkin]

On Fri, Sep 02, 2022 at 10:44:20AM +0200, Gerd Hoffmann wrote:
>   Hi,
>  
> > I feel there are three major sources of controversy here
> > 
> > 0. the cover letter and subject don't do such a good job
> >    explaining that what we are doing is just telling guest
> >    CPUID is not broken. we are not exposing anything new
> >    and not exposing host capability to guest, for example,
> >    if cpuid phys address is smaller than host things also
> >    work fine.
> > 
> > 1. really the naming.  We need to be more explicit that it's just a bugfix.
> 
> Yep, I'll go improve that for v2.
> 
> > 2. down the road we will want to switch the default when no PV. however,
> >    some hosts might still want conservative firmware for compatibility
> >    reasons, so I think we need a way to tell firmware
> >    "ignore phys address width in CPUID like you did in the past".
> >    let's add a flag for that?
> >    and if none are set firmware should print a warning, though I
> >    do not know how many people will see that. Maybe some ;)
> 
> > /*
> >  * Force firmware to be very conservative in its use of physical
> >  * addresses, ignoring phys address width in CPUID.
> >  * Helpful for migration between hosts with different capabilities.
> >  */
> > #define KVM_BUG_PHYS_ADDRESS_WIDTH_BROKEN 2
> 
> I don't see a need for that.  Live migration compatibility can be
> handled just fine today using
> 	'host-phys-bits=on,host-phys-bits-limit=<xx>'
> 
> Which is simliar to 'phys-bits=<xx>'.

yes but what if user did not configure anything?

the point of the above is so we can eventually, in X years, change the guests
to trust CPUID by default.

> The important difference is that phys-bits allows pretty much anything
> whereas host-phys-bits-limit applies sanity checks against the host
> supported phys bits and throws error on invalid values.
> 
> take care,
>   Gerd

Re: [PATCH 0/2] expose host-phys-bits to guest

[Gerd Hoffmann]

  Hi,

> > I don't see a need for that.  Live migration compatibility can be
> > handled just fine today using
> > 	'host-phys-bits=on,host-phys-bits-limit=<xx>'
> > 
> > Which is simliar to 'phys-bits=<xx>'.
> 
> yes but what if user did not configure anything?

I expect that'll be less and less common.  The phys-bits=40 default used
by qemu becomes increasingly problematic for large guests which need
more than that, and we see activity implementing support for that in
libvirt.

> the point of the above is so we can eventually, in X years, change the guests
> to trust CPUID by default.

Well, we can flip host-phys-bits to default to 'on' in qemu for new
machine types (or new cpu versions).  That'll have the very same effect.

take care,
  Gerd