[v2] acpi: reboot: log reset parameters

[PATCH v2] acpi: reboot: log reset parameters

Posted by dmukhin@ford.com 1 week, 1 day ago

From: Denis Mukhin <dmukhin@ford.com> 

Xen does not provide much details for system reset debugging in case
system reset happens via ACPI subsystem.

Log reset I/O address and reset value.

While here, add the missing default case, add breaks between case
statements and drop full stops in the loglines.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
- v1: https://lore.kernel.org/xen-devel/20260730001854.905354-2-dmukhin@ford.com/ 
- CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/2723268852

Changes since v1:
- removed wrong ASSERT_UNREACHABLE()
- switched formatting to %#x
- fixed indentation
---
 xen/drivers/acpi/reboot.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/acpi/reboot.c b/xen/drivers/acpi/reboot.c
index f6345be8749f..dc5671f9b42a 100644
--- a/xen/drivers/acpi/reboot.c
+++ b/xen/drivers/acpi/reboot.c
@@ -6,6 +6,7 @@ void acpi_reboot(void)
 {
 	struct acpi_generic_address *rr;
 	u8 reset_value;
+	pci_sbdf_t sbdf;
 
 	rr = &acpi_gbl_FADT.reset_register;
 
@@ -21,17 +22,30 @@ void acpi_reboot(void)
 	 * on a device on bus 0. */
 	switch (rr->space_id) {
 	case ACPI_ADR_SPACE_PCI_CONFIG:
-		printk("Resetting with ACPI PCI RESET_REG.\n");
+		sbdf = PCI_SBDF(0, 0, rr->address >> 32, rr->address >> 16);
+		printk("Resetting with ACPI PCI %pp RESET_REG at %#lx (%#x)\n",
+		       &sbdf, rr->address & 0xff, reset_value);
 		/* Write the value that resets us. */
-		pci_conf_write8(PCI_SBDF(0, 0, rr->address >> 32,
-					 rr->address >> 16),
-				(rr->address & 255),
-				reset_value);
+		pci_conf_write8(sbdf, rr->address & 0xff, reset_value);
 		break;
+
 	case ACPI_ADR_SPACE_SYSTEM_MEMORY:
-	case ACPI_ADR_SPACE_SYSTEM_IO:
-		printk("Resetting with ACPI MEMORY or I/O RESET_REG.\n");
+		printk("Resetting with ACPI MEMORY at %#lx (%#x)\n",
+		       rr->address, reset_value);
 		acpi_hw_low_level_write(8, reset_value, rr);
 		break;
+
+	case ACPI_ADR_SPACE_SYSTEM_IO:
+		printk("Resetting with I/O RESET_REG at %#lx (%#x)\n",
+		       rr->address, reset_value);
+		acpi_hw_low_level_write(8, reset_value, rr);
+		break;
+
+	default:
+		/* Fallback to alternative reboot methods */
+		printk(XENLOG_WARNING
+		       "Resetting with ACPI method failed: bad ADR %#x\n",
+		       rr->space_id);
+		break;
 	}
 }
-- 
2.54.0

Re: [PATCH v2] acpi: reboot: log reset parameters

Posted by Jan Beulich 3 days, 17 hours ago

On 01.08.2026 05:17, dmukhin@ford.com wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> Xen does not provide much details for system reset debugging in case
> system reset happens via ACPI subsystem.
> 
> Log reset I/O address and reset value.
> 
> While here, add the missing default case, add breaks between case
> statements and drop full stops in the loglines.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> - v1: https://lore.kernel.org/xen-devel/20260730001854.905354-2-dmukhin@ford.com/ 
> - CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/2723268852
> 
> Changes since v1:
> - removed wrong ASSERT_UNREACHABLE()

And you replaced it with a printk(), which I don't view as helpful. If we
want to diagnose the address violating the spec, that should be done
elsewhere.

> @@ -21,17 +22,30 @@ void acpi_reboot(void)
>  	 * on a device on bus 0. */
>  	switch (rr->space_id) {
>  	case ACPI_ADR_SPACE_PCI_CONFIG:
> -		printk("Resetting with ACPI PCI RESET_REG.\n");
> +		sbdf = PCI_SBDF(0, 0, rr->address >> 32, rr->address >> 16);
> +		printk("Resetting with ACPI PCI %pp RESET_REG at %#lx (%#x)\n",
> +		       &sbdf, rr->address & 0xff, reset_value);

rr->address is u64, and the code here isn't arch-specific. Yes, the file is
built for x86 only right now, so 'l' as format modifier is kind of okay for
the time being. But really PRIx64 would want using. (I'm sorry for not
noticing this on v1 already.)

Preferably with that adjustment and with the excess log message dropped
again (I can certainly do so while committing):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2] acpi: reboot: log reset parameters

Posted by dmukhin@ford.com 1 day, 9 hours ago

On Wed, Aug 05, 2026 at 12:18:33PM +0200, Jan Beulich wrote:
> On 01.08.2026 05:17, dmukhin@ford.com wrote:
> > From: Denis Mukhin <dmukhin@ford.com> 
> > 
> > Xen does not provide much details for system reset debugging in case
> > system reset happens via ACPI subsystem.
> > 
> > Log reset I/O address and reset value.
> > 
> > While here, add the missing default case, add breaks between case
> > statements and drop full stops in the loglines.
> > 
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > - v1: https://lore.kernel.org/xen-devel/20260730001854.905354-2-dmukhin@ford.com/ 
> > - CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/2723268852
> > 
> > Changes since v1:
> > - removed wrong ASSERT_UNREACHABLE()
> 
> And you replaced it with a printk(), which I don't view as helpful. If we
> want to diagnose the address violating the spec, that should be done
> elsewhere.

Ack.

> 
> > @@ -21,17 +22,30 @@ void acpi_reboot(void)
> >  	 * on a device on bus 0. */
> >  	switch (rr->space_id) {
> >  	case ACPI_ADR_SPACE_PCI_CONFIG:
> > -		printk("Resetting with ACPI PCI RESET_REG.\n");
> > +		sbdf = PCI_SBDF(0, 0, rr->address >> 32, rr->address >> 16);
> > +		printk("Resetting with ACPI PCI %pp RESET_REG at %#lx (%#x)\n",
> > +		       &sbdf, rr->address & 0xff, reset_value);
> 
> rr->address is u64, and the code here isn't arch-specific. Yes, the file is
> built for x86 only right now, so 'l' as format modifier is kind of okay for
> the time being. But really PRIx64 would want using. (I'm sorry for not
> noticing this on v1 already.)

I had doubts on that one, but decided to go with 'l' in v2.

> 
> Preferably with that adjustment and with the excess log message dropped
> again (I can certainly do so while committing):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you!

> 
> Jan
>