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

deller@kernel.org posted 9 patches 8 months, 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>
[PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by deller@kernel.org 8 months, 1 week ago
From: Helge Deller <deller@gmx.de>

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>
---
 hw/hppa/machine.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 54ca2fd91a..9e611620cc 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;
 
 #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);
@@ -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(soft_power_reg);
     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);
 
-- 
2.43.0
Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by Richard Henderson 8 months, 1 week ago
On 1/12/24 21:29, deller@kernel.org wrote:
> +static hwaddr soft_power_reg;
>   
>   #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);
> @@ -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(soft_power_reg);

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.


r~
Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by Helge Deller 8 months, 1 week ago
> 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;
 
 #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
Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by Richard Henderson 8 months, 1 week ago
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
Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by Helge Deller 8 months, 1 week ago
On 1/13/24 00:11, Richard Henderson wrote:
> 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.

No... I did not forgot it.
This is still needed by other functions (e.g. hppa_powerdown_req())
in the same file.

If we want to remove it, it will become a much bigger and intrusive patch
(if there is even a nice clean way to do it).

Helge
> 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
>
Re: [PATCH v3 5/9] hw/hppa: Move software power button address back into PDC
Posted by Richard Henderson 8 months, 1 week ago
On 1/13/24 10:15, Helge Deller wrote:
> On 1/13/24 00:11, Richard Henderson wrote:
>> 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.
> 
> No... I did not forgot it.
> This is still needed by other functions (e.g. hppa_powerdown_req())
> in the same file.

Ah, I missed that.  In which case,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

to the original.


r~