Re: [PATCH] hw/i386/pc: fix max_used_gpa for 32-bit systems

[Michael S. Tsirkin]

On Mon, Sep 18, 2023 at 07:24:48PM +0530, Ani Sinha wrote:
> 32-bit systems do not have a reserved memory for hole64 but they may have a
> reserved memory space for memory hotplug. Since, hole64 starts after the
> reserved hotplug memory, the unaligned hole64 start address gives us the
> end address for this memory hotplug region that the processor may use.
> Fix this. This ensures that the physical address space bound checking works
> correctly for 32-bit systems as well.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>


I doubt we can make changes like this without compat machinery. No?

> ---
>  hw/i386/pc.c | 60 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 54838c0c41..c8abcabd53 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -904,13 +904,43 @@ static uint64_t pc_get_cxl_range_end(PCMachineState 
> *pcms)
>      return start;
>  }
>  
> +/*
> + * The 64bit pci hole starts after "above 4G RAM" and
> + * potentially the space reserved for memory hotplug.
> + * This function returns unaligned start address.
> + */
> +static uint64_t pc_pci_hole64_start_unaligned(void)
> +{
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    MachineState *ms = MACHINE(pcms);
> +    uint64_t hole64_start = 0;
> +    ram_addr_t size = 0;
> +
> +    if (pcms->cxl_devices_state.is_enabled) {
> +        hole64_start = pc_get_cxl_range_end(pcms);
> +    } else if (pcmc->has_reserved_memory && (ms->ram_size < 
> ms->maxram_size)) {
> +        pc_get_device_memory_range(pcms, &hole64_start, &size);
> +        if (!pcmc->broken_reserved_end) {
> +            hole64_start += size;
> +        }
> +    } else {
> +        hole64_start = pc_above_4g_end(pcms);
> +    }
> +
> +    return hole64_start;
> +}
> +
>  static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
>  {
>      X86CPU *cpu = X86_CPU(first_cpu);
>  
> -    /* 32-bit systems don't have hole64 thus return max CPU address */
> -    if (cpu->phys_bits <= 32) {
> -        return ((hwaddr)1 << cpu->phys_bits) - 1;
> +    /*
> +     * 32-bit systems don't have hole64, but we might have a region for
> +     * memory hotplug.
> +     */
> +    if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
> +        return pc_pci_hole64_start_unaligned() - 1;
>      }
>  

I see you are changing cpu->phys_bits to a CPUID check.
Could you explain why in the commit log?

>      return pc_pci_hole64_start() + pci_hole64_size - 1;
> @@ -1147,30 +1177,10 @@ void pc_memory_init(PCMachineState *pcms,
>      pcms->memhp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
>  }
>  
> -/*
> - * The 64bit pci hole starts after "above 4G RAM" and
> - * potentially the space reserved for memory hotplug.
> - */
> +/* returns 1 GiB aligned hole64 start address */
>  uint64_t pc_pci_hole64_start(void)
>  {
> -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    MachineState *ms = MACHINE(pcms);
> -    uint64_t hole64_start = 0;
> -    ram_addr_t size = 0;
> -
> -    if (pcms->cxl_devices_state.is_enabled) {
> -        hole64_start = pc_get_cxl_range_end(pcms);
> -    } else if (pcmc->has_reserved_memory && (ms->ram_size < 
> ms->maxram_size)) {
> -        pc_get_device_memory_range(pcms, &hole64_start, &size);
> -        if (!pcmc->broken_reserved_end) {
> -            hole64_start += size;
> -        }
> -    } else {
> -        hole64_start = pc_above_4g_end(pcms);
> -    }
> -
> -    return ROUND_UP(hole64_start, 1 * GiB);
> +    return ROUND_UP(pc_pci_hole64_start_unaligned(), 1 * GiB);
>  }
>  
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus)
> -- 
> 2.39.1