[v1] acpi: reboot: log reset parameters

[PATCH] acpi: reboot: log reset parameters

Posted by dmukhin@ford.com 3 weeks, 5 days 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, fix the missing default case, guard it with
ASSERT_UNREACHABLE() and drop full stops in the loglines.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/2716843348 
---
 xen/drivers/acpi/reboot.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/acpi/reboot.c b/xen/drivers/acpi/reboot.c
index f6345be8749f..3b0437395674 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,24 @@ 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 0x%"PRIx64" (0x%x)\n",
+			&sbdf, rr->address & 0xffu, 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 & 0xffu, 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 0x%"PRIx64" (0x%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 0x%"PRIx64" (0x%x)\n",
+			rr->address, reset_value);
+		acpi_hw_low_level_write(8, reset_value, rr);
+		break;
+	default:
+		ASSERT_UNREACHABLE();
+		break;
 	}
 }
-- 
2.54.0

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

Posted by Jan Beulich 3 weeks, 5 days ago

On 30.07.2026 02:18, 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, fix the missing default case, guard it with
> ASSERT_UNREACHABLE() and drop full stops in the loglines.

On what basis (i.e. thanks to which earlier checks) would this assertion be
legitimate to add? Besides being a wrong use of an assertion, it also breaks
fallback to alternative reboot methods in case one doesn't work.

> --- 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,24 @@ 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 0x%"PRIx64" (0x%x)\n",
> +			&sbdf, rr->address & 0xffu, reset_value);

As indicated on other occasions - %#x and alike please in favor of 0x%x.

I also see no reason for the 'u' suffix on the literal number. Plus if one
was wanted, it would want to be 'U', to match the Misra-demanded 'L'.

Also - nit: Indentation.

>  		/* 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 & 0xffu, 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 0x%"PRIx64" (0x%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 0x%"PRIx64" (0x%x)\n",
> +			rr->address, reset_value);
> +		acpi_hw_low_level_write(8, reset_value, rr);
> +		break;
> +	default:
> +		ASSERT_UNREACHABLE();
> +		break;
>  	}
>  }

As you're already touching the entire switch(), would you mind also inserting
the missing blank lines between case blocks?

Jan

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

Posted by dmukhin@ford.com 3 weeks, 4 days ago

On Thu, Jul 30, 2026 at 08:26:38AM +0200, Jan Beulich wrote:
> On 30.07.2026 02:18, 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, fix the missing default case, guard it with
> > ASSERT_UNREACHABLE() and drop full stops in the loglines.
> 
> On what basis (i.e. thanks to which earlier checks) would this assertion be
> legitimate to add? Besides being a wrong use of an assertion, it also breaks
> fallback to alternative reboot methods in case one doesn't work.
> 
> > --- 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,24 @@ 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 0x%"PRIx64" (0x%x)\n",
> > +			&sbdf, rr->address & 0xffu, reset_value);
> 
> As indicated on other occasions - %#x and alike please in favor of 0x%x.
> 
> I also see no reason for the 'u' suffix on the literal number. Plus if one
> was wanted, it would want to be 'U', to match the Misra-demanded 'L'.
> 
> Also - nit: Indentation.

Thanks for taking a look!

This file uses tabs - I can convert to spaces, but in separate patch.
What do you think?

> 
> >  		/* 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 & 0xffu, 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 0x%"PRIx64" (0x%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 0x%"PRIx64" (0x%x)\n",
> > +			rr->address, reset_value);
> > +		acpi_hw_low_level_write(8, reset_value, rr);
> > +		break;
> > +	default:
> > +		ASSERT_UNREACHABLE();
> > +		break;
> >  	}
> >  }
> 
> As you're already touching the entire switch(), would you mind also inserting
> the missing blank lines between case blocks?

Yes, will do.

> 
> Jan
>

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

Posted by Jan Beulich 3 weeks, 4 days ago

On 31.07.2026 06:42, dmukhin@ford.com wrote:
> On Thu, Jul 30, 2026 at 08:26:38AM +0200, Jan Beulich wrote:
>> On 30.07.2026 02:18, dmukhin@ford.com wrote:
>>> --- 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,24 @@ 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 0x%"PRIx64" (0x%x)\n",
>>> +			&sbdf, rr->address & 0xffu, reset_value);
>>
>> As indicated on other occasions - %#x and alike please in favor of 0x%x.
>>
>> I also see no reason for the 'u' suffix on the literal number. Plus if one
>> was wanted, it would want to be 'U', to match the Misra-demanded 'L'.
>>
>> Also - nit: Indentation.
> 
> Thanks for taking a look!
> 
> This file uses tabs - I can convert to spaces, but in separate patch.

That wasn't the point of my remark though. Merely switching to all-blanks
indentation would be wrong. Converting altogether to Xen style would be
an option. My remark was about you not using the necessary mix of tabs
and blanks when the wrapped part of a statement want to align with the
respective part on the earlier line. I.e. here two tabs followed by 7
blanks.

Jan