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

["Cédric Le Goater" <clg@redhat.com>]

On 3/6/25 11:37, Philippe Mathieu-Daudé wrote:
> 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);
>> +
>>       /**
>>        * @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();
> 
> I might have suggested to return TARGET_PHYS_ADDR_SPACE_BITS
> by default in v1, I'm not sure about it anymore. Maybe default
> to 0 and have each target register its helper if necessary?

sure. No problem. I can change that.


Thanks,

C.


> 
>> +}
>> +
>>   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,
>