Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC

[Richard Henderson <richard.henderson@linaro.org>]

On 1/13/24 09:44, Helge Deller wrote:
>> I think it would be better to pass this as a parameter to create_fw_cfg, or
>> to drop the translated FW_CFG_IO_BASE parameter and merely pass in translate
>> itself.
> 
> Like this?
> 
> 
> The various operating systems (e.g. Linux, NetBSD) have issues
> mapping the power button when it's stored in page zero.
> NetBSD even crashes, because it fails to map that page and then
> accesses unmapped memory.
> 
> Since we now have a consistent memory mapping of PDC in 32-bit
> and 64-bit address space (the lower 32-bits of the address are in
> sync) the power button can be moved back to PDC space.
> 
> This patch fixes the power button on Linux, NetBSD and HP-UX.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Tested-by: Bruno Haible <bruno@clisp.org>
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 54ca2fd91a..da85050f60 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -36,8 +36,8 @@
>   
>   #define MIN_SEABIOS_HPPA_VERSION 12 /* require at least this fw version */
>   
> -/* Power button address at &PAGE0->pad[4] */
> -#define HPA_POWER_BUTTON (0x40 + 4 * sizeof(uint32_t))
> +#define HPA_POWER_BUTTON        (FIRMWARE_END - 0x10)
> +static hwaddr soft_power_reg;

You've forgotten to remove the global.


r~

>   
>   #define enable_lasi_lan()       0
>   
> @@ -45,7 +45,6 @@ static DeviceState *lasi_dev;
>   
>   static void hppa_powerdown_req(Notifier *n, void *opaque)
>   {
> -    hwaddr soft_power_reg = HPA_POWER_BUTTON;
>       uint32_t val;
>   
>       val = ldl_be_phys(&address_space_memory, soft_power_reg);
> @@ -191,7 +190,7 @@ static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>   }
>   
>   static FWCfgState *create_fw_cfg(MachineState *ms, PCIBus *pci_bus,
> -                                 hwaddr addr)
> +                                 hwaddr addr, hwaddr pwr_button_addr)
>   {
>       FWCfgState *fw_cfg;
>       uint64_t val;
> @@ -221,7 +220,7 @@ static FWCfgState *create_fw_cfg(MachineState *ms, PCIBus *pci_bus,
>       fw_cfg_add_file(fw_cfg, "/etc/hppa/machine",
>                       g_memdup(mc->name, len), len);
>   
> -    val = cpu_to_le64(HPA_POWER_BUTTON);
> +    val = cpu_to_le64(pwr_button_addr);
>       fw_cfg_add_file(fw_cfg, "/etc/hppa/power-button-addr",
>                       g_memdup(&val, sizeof(val)), sizeof(val));
>   
> @@ -295,6 +294,8 @@ static TranslateFn *machine_HP_common_init_cpus(MachineState *machine)
>           ram_max = 0xf0000000;      /* 3.75 GB (32-bit CPU) */
>       }
>   
> +    soft_power_reg = translate(NULL, HPA_POWER_BUTTON);
> +
>       for (unsigned int i = 0; i < smp_cpus; i++) {
>           g_autofree char *name = g_strdup_printf("cpu%u-io-eir", i);
>   
> @@ -407,7 +408,8 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
>       qemu_register_powerdown_notifier(&hppa_system_powerdown_notifier);
>   
>       /* fw_cfg configuration interface */
> -    create_fw_cfg(machine, pci_bus, translate(NULL, FW_CFG_IO_BASE));
> +    create_fw_cfg(machine, pci_bus, translate(NULL, FW_CFG_IO_BASE),
> +        translate(NULL, HPA_POWER_BUTTON));
>   
>       /* Load firmware.  Given that this is not "real" firmware,
>          but one explicitly written for the emulation, we might as