Re: [PATCH v2 7/9] cpu: Introduce cpu_get_phys_bits()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Cédric,

On 30/1/25 14:43, Cédric Le Goater wrote:
> The Intel CPU has a complex history regarding setting of the physical
> address space width on KVM. A 'phys_bits' field and a "phys-bits"
> property were added by commit af45907a1328 ("target-i386: Allow
> physical address bits to be set") to tune this value.
> 
> In certain circumstances, it is interesting to know this value to
> check that all the conditions are met for optimal operation. For
> instance, when the system has a 39-bit IOMMU address space width and a
> larger CPU physical address space, we expect issues when mapping the
> MMIO regions of passthrough devices and it would good to report to the
> user. These hybrid HW configs can be found on some consumer grade
> processors or when using a vIOMMU device with default settings.
> 
> For this purpose, add an helper routine and a CPUClass callback to
> return the physical address space width of a CPU.
> 
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Cc: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/core/cpu.h            |  9 +++++++++
>   include/hw/core/sysemu-cpu-ops.h |  6 ++++++
>   cpu-target.c                     |  5 +++++
>   hw/core/cpu-system.c             | 11 +++++++++++
>   target/i386/cpu.c                |  6 ++++++
>   5 files changed, 37 insertions(+)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index fb397cdfc53d12d40d3e4e7f86251fc31c48b9f6..1b3eead102ce62fcee55ab0ed5e0dff327fa2fc5 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -748,6 +748,14 @@ int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs);
>    */
>   bool cpu_virtio_is_big_endian(CPUState *cpu);
>   
> +/**
> + * cpu_get_phys_bits:
> + * @cpu: CPU
> + *
> + * Return the physical address space width of the CPU @cpu.
> + */
> +uint32_t cpu_get_phys_bits(const CPUState *cpu);
> +
>   #endif /* CONFIG_USER_ONLY */
>   
>   /**
> @@ -1168,6 +1176,7 @@ void cpu_exec_unrealizefn(CPUState *cpu);
>   void cpu_exec_reset_hold(CPUState *cpu);
>   
>   const char *target_name(void);
> +uint32_t target_phys_bits(void);
>   
>   #ifdef COMPILING_PER_TARGET
>   
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index 0df5b058f50073e47d2a6b8286be5204776520d2..210b3ed57985525795b81559e41e0085969210d5 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -81,6 +81,12 @@ typedef struct SysemuCPUOps {
>        */
>       bool (*virtio_is_big_endian)(CPUState *cpu);
>   
> +    /**
> +     * @get_phys_bits: Callback to return the physical address space
> +     * width of a CPU.
> +     */
> +    uint32_t (*get_phys_bits)(const CPUState *cpu);

Using 32-bit isn't really relevant IMHO, I'd return an unsigned type.

> +
>       /**
>        * @legacy_vmsd: Legacy state for migration.
>        *               Do not use in new targets, use #DeviceClass::vmsd instead.
> diff --git a/cpu-target.c b/cpu-target.c
> index 667688332c929aa53782c94343def34571272d5f..88158272c06cc42424d435b9701e33735f080239 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -472,3 +472,8 @@ const char *target_name(void)
>   {
>       return TARGET_NAME;
>   }
> +
> +uint32_t target_phys_bits(void)
> +{
> +    return TARGET_PHYS_ADDR_SPACE_BITS;
> +}
> diff --git a/hw/core/cpu-system.c b/hw/core/cpu-system.c
> index 6aae28a349a7a377d010ff9dcab5ebc29e1126ca..05067d84f4258facf4252216f17729e390d38eae 100644
> --- a/hw/core/cpu-system.c
> +++ b/hw/core/cpu-system.c
> @@ -60,6 +60,17 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>       return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>   }
>   
> +uint32_t cpu_get_phys_bits(const CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->sysemu_ops->get_phys_bits) {
> +        return cc->sysemu_ops->get_phys_bits(cpu);
> +    }
> +
> +    return target_phys_bits();

With heterogeneous emulation in mind, I'd rather this to be a mandatory
CPUClass handler.

> +}
> +
>   hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>   {
>       MemTxAttrs attrs = {};
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b5dd60d2812e0c3d13c1743fd485a9068ab29c4f..01cf9a44963710a415c755c17582730f75233143 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -8393,6 +8393,11 @@ static bool x86_cpu_get_paging_enabled(const CPUState *cs)
>   
>       return cpu->env.cr[0] & CR0_PG_MASK;
>   }
> +
> +static uint32_t x86_cpu_get_phys_bits(const CPUState *cs)
> +{
> +    return X86_CPU(cs)->phys_bits;
> +}
>   #endif /* !CONFIG_USER_ONLY */
>   
>   static void x86_cpu_set_pc(CPUState *cs, vaddr value)
> @@ -8701,6 +8706,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
>       .get_memory_mapping = x86_cpu_get_memory_mapping,
>       .get_paging_enabled = x86_cpu_get_paging_enabled,
>       .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
> +    .get_phys_bits = x86_cpu_get_phys_bits,
>       .asidx_from_attrs = x86_asidx_from_attrs,
>       .get_crash_info = x86_cpu_get_crash_info,
>       .write_elf32_note = x86_cpu_write_elf32_note,