gpex build_host_bridge_osc() and x86 originated
build_pci_host_bridge_osc_method() are mostly identical.
In GPEX, SUPP is set to CDW2 but is not further used. CTRL
is same as Local0.
So let gpex code reuse build_pci_host_bridge_osc_method()
and remove build_host_bridge_osc().
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
The DSDT diff is given below:
diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
index 3224a56..fa7558e 100644
--- a/dsdt.dsl_before
+++ b/dsdt.dsl_after_osc_change
@@ -5,13 +5,13 @@
*
* Disassembling to symbolic ASL+ operators
*
- * Disassembly of dsdt.dat, Mon Apr 7 05:33:06 2025
+ * Disassembly of dsdt.dat, Mon Apr 7 05:37:20 2025
*
* Original Table Header:
* Signature "DSDT"
- * Length 0x00001A4F (6735)
+ * Length 0x00001A35 (6709)
* Revision 0x02
- * Checksum 0xBF
+ * Checksum 0xDD
* OEM ID "BOCHS "
* OEM Table ID "BXPC "
* OEM Revision 0x00000001 (1)
@@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ", "BXPC ", 0x00000001)
{
CreateDWordField (Arg3, 0x04, CDW2)
CreateDWordField (Arg3, 0x08, CDW3)
- SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
- CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
- CTRL &= 0x1F
+ Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
+ Local0 &= 0x1F
If ((Arg1 != One))
{
CDW1 |= 0x08
}
- If ((CDW3 != CTRL))
+ If ((CDW3 != Local0))
{
CDW1 |= 0x10
}
- CDW3 = CTRL /* \_SB_.PCI0.CTRL */
- Return (Arg3)
+ CDW3 = Local0
}
Else
{
CDW1 |= 0x04
- Return (Arg3)
}
+
+ Return (Arg3)
}
Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
---
hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
1 file changed, 4 insertions(+), 56 deletions(-)
diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
index f1ab30f3d5..98c9868c3f 100644
--- a/hw/pci-host/gpex-acpi.c
+++ b/hw/pci-host/gpex-acpi.c
@@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq,
}
}
-static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
-{
- Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
-
- method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
- aml_append(method,
- aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
-
- /* PCI Firmware Specification 3.0
- * 4.5.1. _OSC Interface for PCI Host Bridge Devices
- * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
- * identified by the Universal Unique IDentifier (UUID)
- * 33DB4D5B-1FF7-401C-9657-7441C03DD766
- */
- UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
- ifctx = aml_if(aml_equal(aml_arg(0), UUID));
- aml_append(ifctx,
- aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
- aml_append(ifctx,
- aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
- aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
- aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
-
- /*
- * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
- * and PCIeHotplug depending on enable_native_pcie_hotplug
- */
- aml_append(ifctx, aml_and(aml_name("CTRL"),
- aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)),
- aml_name("CTRL")));
-
- ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
- aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
- aml_name("CDW1")));
- aml_append(ifctx, ifctx1);
-
- ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"), aml_name("CTRL"))));
- aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
- aml_name("CDW1")));
- aml_append(ifctx, ifctx1);
-
- aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
- aml_append(ifctx, aml_return(aml_arg(3)));
- aml_append(method, ifctx);
-
- elsectx = aml_else();
- aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
- aml_name("CDW1")));
- aml_append(elsectx, aml_return(aml_arg(3)));
- aml_append(method, elsectx);
- return method;
-}
-
-static Aml *build_host_bridge_dsm(void)
+static Aml *build_pci_host_bridge_dsm_method(void)
{
Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
Aml *UUID, *ifctx, *ifctx1, *buf;
@@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
/* Declare an _OSC (OS Control Handoff) method */
- aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
- aml_append(dev, build_host_bridge_dsm());
+ aml_append(dev,
+ build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
+ aml_append(dev, build_pci_host_bridge_dsm_method());
}
void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
--
2.49.0Hi Eric,
On 5/14/25 14:00, Eric Auger wrote:
> gpex build_host_bridge_osc() and x86 originated
> build_pci_host_bridge_osc_method() are mostly identical.
>
> In GPEX, SUPP is set to CDW2 but is not further used. CTRL
> is same as Local0.
>
> So let gpex code reuse build_pci_host_bridge_osc_method()
> and remove build_host_bridge_osc().
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> The DSDT diff is given below:
> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> index 3224a56..fa7558e 100644
> --- a/dsdt.dsl_before
> +++ b/dsdt.dsl_after_osc_change
> @@ -5,13 +5,13 @@
> *
> * Disassembling to symbolic ASL+ operators
> *
> - * Disassembly of dsdt.dat, Mon Apr 7 05:33:06 2025
> + * Disassembly of dsdt.dat, Mon Apr 7 05:37:20 2025
> *
> * Original Table Header:
> * Signature "DSDT"
> - * Length 0x00001A4F (6735)
> + * Length 0x00001A35 (6709)
> * Revision 0x02
> - * Checksum 0xBF
> + * Checksum 0xDD
> * OEM ID "BOCHS "
> * OEM Table ID "BXPC "
> * OEM Revision 0x00000001 (1)
> @@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ", "BXPC ", 0x00000001)
> {
> CreateDWordField (Arg3, 0x04, CDW2)
> CreateDWordField (Arg3, 0x08, CDW3)
> - SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
> - CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> - CTRL &= 0x1F
> + Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> + Local0 &= 0x1F
> If ((Arg1 != One))
> {
> CDW1 |= 0x08
> }
>
> - If ((CDW3 != CTRL))
> + If ((CDW3 != Local0))
> {
> CDW1 |= 0x10
> }
>
> - CDW3 = CTRL /* \_SB_.PCI0.CTRL */
> - Return (Arg3)
> + CDW3 = Local0
> }
> Else
> {
> CDW1 |= 0x04
> - Return (Arg3)
> }
> +
> + Return (Arg3)
> }
>
> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
The problem I face with diffs in the commit body is that tools like b4, which are
based on git am, get very confused on how to handle it. I'm surprised nobody ever
complained about it. I'm wondering if there is any catch on it, because I have to
edit commits like this manually, removing the diff, to make it finally apply to
the series. Anyways, do you mind at least removing the valid diff header, like:
> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> index 3224a56..fa7558e 100644
> --- a/dsdt.dsl_before
> +++ b/dsdt.dsl_after_osc_change
from the commit message so it doesn't confuse b4?
> ---
> hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
> 1 file changed, 4 insertions(+), 56 deletions(-)
>
> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> index f1ab30f3d5..98c9868c3f 100644
> --- a/hw/pci-host/gpex-acpi.c
> +++ b/hw/pci-host/gpex-acpi.c
> @@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq,
> }
> }
>
> -static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
> -{
> - Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
> -
> - method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> - aml_append(method,
> - aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> -
> - /* PCI Firmware Specification 3.0
> - * 4.5.1. _OSC Interface for PCI Host Bridge Devices
> - * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
> - * identified by the Universal Unique IDentifier (UUID)
> - * 33DB4D5B-1FF7-401C-9657-7441C03DD766
> - */
> - UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
> - ifctx = aml_if(aml_equal(aml_arg(0), UUID));
> - aml_append(ifctx,
> - aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
> - aml_append(ifctx,
> - aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
> - aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
> - aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
> -
> - /*
> - * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
> - * and PCIeHotplug depending on enable_native_pcie_hotplug
> - */
> - aml_append(ifctx, aml_and(aml_name("CTRL"),
> - aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)),
> - aml_name("CTRL")));
> -
> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
> - aml_name("CDW1")));
> - aml_append(ifctx, ifctx1);
> -
> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"), aml_name("CTRL"))));
> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
> - aml_name("CDW1")));
> - aml_append(ifctx, ifctx1);
> -
> - aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
> - aml_append(ifctx, aml_return(aml_arg(3)));
> - aml_append(method, ifctx);
> -
> - elsectx = aml_else();
> - aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
> - aml_name("CDW1")));
> - aml_append(elsectx, aml_return(aml_arg(3)));
> - aml_append(method, elsectx);
> - return method;
> -}
> -
> -static Aml *build_host_bridge_dsm(void)
> +static Aml *build_pci_host_bridge_dsm_method(void)
> {
> Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
> Aml *UUID, *ifctx, *ifctx1, *buf;
> @@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
> aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
> aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> /* Declare an _OSC (OS Control Handoff) method */
> - aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
> - aml_append(dev, build_host_bridge_dsm());
> + aml_append(dev,
> + build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
> + aml_append(dev, build_pci_host_bridge_dsm_method());
> }
>
> void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
Otherwise:
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
Cheers,
Gustavo
Hi Gustavo,
On 5/20/25 4:09 PM, Gustavo Romero wrote:
> Hi Eric,
>
> On 5/14/25 14:00, Eric Auger wrote:
>> gpex build_host_bridge_osc() and x86 originated
>> build_pci_host_bridge_osc_method() are mostly identical.
>>
>> In GPEX, SUPP is set to CDW2 but is not further used. CTRL
>> is same as Local0.
>>
>> So let gpex code reuse build_pci_host_bridge_osc_method()
>> and remove build_host_bridge_osc().
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> The DSDT diff is given below:
>> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
>> index 3224a56..fa7558e 100644
>> --- a/dsdt.dsl_before
>> +++ b/dsdt.dsl_after_osc_change
>> @@ -5,13 +5,13 @@
>> *
>> * Disassembling to symbolic ASL+ operators
>> *
>> - * Disassembly of dsdt.dat, Mon Apr 7 05:33:06 2025
>> + * Disassembly of dsdt.dat, Mon Apr 7 05:37:20 2025
>> *
>> * Original Table Header:
>> * Signature "DSDT"
>> - * Length 0x00001A4F (6735)
>> + * Length 0x00001A35 (6709)
>> * Revision 0x02
>> - * Checksum 0xBF
>> + * Checksum 0xDD
>> * OEM ID "BOCHS "
>> * OEM Table ID "BXPC "
>> * OEM Revision 0x00000001 (1)
>> @@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ",
>> "BXPC ", 0x00000001)
>> {
>> CreateDWordField (Arg3, 0x04, CDW2)
>> CreateDWordField (Arg3, 0x08, CDW3)
>> - SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
>> - CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
>> - CTRL &= 0x1F
>> + Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
>> + Local0 &= 0x1F
>> If ((Arg1 != One))
>> {
>> CDW1 |= 0x08
>> }
>>
>> - If ((CDW3 != CTRL))
>> + If ((CDW3 != Local0))
>> {
>> CDW1 |= 0x10
>> }
>>
>> - CDW3 = CTRL /* \_SB_.PCI0.CTRL */
>> - Return (Arg3)
>> + CDW3 = Local0
>> }
>> Else
>> {
>> CDW1 |= 0x04
>> - Return (Arg3)
>> }
>> +
>> + Return (Arg3)
>> }
>>
>> Method (_DSM, 4, NotSerialized) // _DSM:
>> Device-Specific Method
>
> The problem I face with diffs in the commit body is that tools like
> b4, which are
> based on git am, get very confused on how to handle it. I'm surprised
> nobody ever
> complained about it. I'm wondering if there is any catch on it,
> because I have to
> edit commits like this manually, removing the diff, to make it finally
> apply to
> the series. Anyways, do you mind at least removing the valid diff
> header, like:
>
>> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
>> index 3224a56..fa7558e 100644
>> --- a/dsdt.dsl_before
>> +++ b/dsdt.dsl_after_osc_change
>
> from the commit message so it doesn't confuse b4?
Thank you for reporting the issue. in tests/qtest/bios-tables-test.c it
is written at the top that we shall put the diffs in disasembled ACPI
content in the commit msg. I will look for previously landed patches and
see whether the current layout can be fixed.
Cheers
Eric
>
>
>> ---
>> hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
>> 1 file changed, 4 insertions(+), 56 deletions(-)
>>
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index f1ab30f3d5..98c9868c3f 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml
>> *dev, uint32_t irq,
>> }
>> }
>> -static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
>> -{
>> - Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
>> -
>> - method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>> - aml_append(method,
>> - aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
>> -
>> - /* PCI Firmware Specification 3.0
>> - * 4.5.1. _OSC Interface for PCI Host Bridge Devices
>> - * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
>> - * identified by the Universal Unique IDentifier (UUID)
>> - * 33DB4D5B-1FF7-401C-9657-7441C03DD766
>> - */
>> - UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
>> - ifctx = aml_if(aml_equal(aml_arg(0), UUID));
>> - aml_append(ifctx,
>> - aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
>> - aml_append(ifctx,
>> - aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
>> - aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
>> - aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
>> -
>> - /*
>> - * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
>> - * and PCIeHotplug depending on enable_native_pcie_hotplug
>> - */
>> - aml_append(ifctx, aml_and(aml_name("CTRL"),
>> - aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 :
>> 0x0)),
>> - aml_name("CTRL")));
>> -
>> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
>> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
>> - aml_name("CDW1")));
>> - aml_append(ifctx, ifctx1);
>> -
>> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"),
>> aml_name("CTRL"))));
>> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
>> - aml_name("CDW1")));
>> - aml_append(ifctx, ifctx1);
>> -
>> - aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
>> - aml_append(ifctx, aml_return(aml_arg(3)));
>> - aml_append(method, ifctx);
>> -
>> - elsectx = aml_else();
>> - aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
>> - aml_name("CDW1")));
>> - aml_append(elsectx, aml_return(aml_arg(3)));
>> - aml_append(method, elsectx);
>> - return method;
>> -}
>> -
>> -static Aml *build_host_bridge_dsm(void)
>> +static Aml *build_pci_host_bridge_dsm_method(void)
>> {
>> Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>> Aml *UUID, *ifctx, *ifctx1, *buf;
>> @@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml
>> *dev,
>> aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>> /* Declare an _OSC (OS Control Handoff) method */
>> - aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
>> - aml_append(dev, build_host_bridge_dsm());
>> + aml_append(dev,
>> +
>> build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
>> + aml_append(dev, build_pci_host_bridge_dsm_method());
>> }
>> void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>
> Otherwise:
>
> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>
>
> Cheers,
> Gustavo
>
On Wed, May 21, 2025 at 06:12:34PM +0200, Eric Auger wrote:
> Hi Gustavo,
>
> On 5/20/25 4:09 PM, Gustavo Romero wrote:
> > Hi Eric,
> >
> > On 5/14/25 14:00, Eric Auger wrote:
> >> gpex build_host_bridge_osc() and x86 originated
> >> build_pci_host_bridge_osc_method() are mostly identical.
> >>
> >> In GPEX, SUPP is set to CDW2 but is not further used. CTRL
> >> is same as Local0.
> >>
> >> So let gpex code reuse build_pci_host_bridge_osc_method()
> >> and remove build_host_bridge_osc().
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >>
> >> The DSDT diff is given below:
> >> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> >> index 3224a56..fa7558e 100644
> >> --- a/dsdt.dsl_before
> >> +++ b/dsdt.dsl_after_osc_change
> >> @@ -5,13 +5,13 @@
> >> *
> >> * Disassembling to symbolic ASL+ operators
> >> *
> >> - * Disassembly of dsdt.dat, Mon Apr 7 05:33:06 2025
> >> + * Disassembly of dsdt.dat, Mon Apr 7 05:37:20 2025
> >> *
> >> * Original Table Header:
> >> * Signature "DSDT"
> >> - * Length 0x00001A4F (6735)
> >> + * Length 0x00001A35 (6709)
> >> * Revision 0x02
> >> - * Checksum 0xBF
> >> + * Checksum 0xDD
> >> * OEM ID "BOCHS "
> >> * OEM Table ID "BXPC "
> >> * OEM Revision 0x00000001 (1)
> >> @@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ",
> >> "BXPC ", 0x00000001)
> >> {
> >> CreateDWordField (Arg3, 0x04, CDW2)
> >> CreateDWordField (Arg3, 0x08, CDW3)
> >> - SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
> >> - CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> >> - CTRL &= 0x1F
> >> + Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> >> + Local0 &= 0x1F
> >> If ((Arg1 != One))
> >> {
> >> CDW1 |= 0x08
> >> }
> >>
> >> - If ((CDW3 != CTRL))
> >> + If ((CDW3 != Local0))
> >> {
> >> CDW1 |= 0x10
> >> }
> >>
> >> - CDW3 = CTRL /* \_SB_.PCI0.CTRL */
> >> - Return (Arg3)
> >> + CDW3 = Local0
> >> }
> >> Else
> >> {
> >> CDW1 |= 0x04
> >> - Return (Arg3)
> >> }
> >> +
> >> + Return (Arg3)
> >> }
> >>
> >> Method (_DSM, 4, NotSerialized) // _DSM:
> >> Device-Specific Method
> >
> > The problem I face with diffs in the commit body is that tools like
> > b4, which are
> > based on git am, get very confused on how to handle it. I'm surprised
> > nobody ever
> > complained about it. I'm wondering if there is any catch on it,
> > because I have to
> > edit commits like this manually, removing the diff, to make it finally
> > apply to
> > the series. Anyways, do you mind at least removing the valid diff
> > header, like:
> >
> >> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
> >> index 3224a56..fa7558e 100644
> >> --- a/dsdt.dsl_before
> >> +++ b/dsdt.dsl_after_osc_change
> >
> > from the commit message so it doesn't confuse b4?
> Thank you for reporting the issue. in tests/qtest/bios-tables-test.c it
> is written at the top that we shall put the diffs in disasembled ACPI
> content in the commit msg. I will look for previously landed patches and
> see whether the current layout can be fixed.
>
> Cheers
>
> Eric
Eric, to clarify, the diff is supposed to go into commit log,
not after ---.
This will make it apply cleanly.
Also, removing the "index" line as well as date diff at least is a good
idea: the diff should be clean, not include irrelevant information.
Pls feel free to clarify the text in tests/qtest/bios-tables-test.c
> >
> >
> >> ---
> >> hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
> >> 1 file changed, 4 insertions(+), 56 deletions(-)
> >>
> >> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> >> index f1ab30f3d5..98c9868c3f 100644
> >> --- a/hw/pci-host/gpex-acpi.c
> >> +++ b/hw/pci-host/gpex-acpi.c
> >> @@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml
> >> *dev, uint32_t irq,
> >> }
> >> }
> >> -static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
> >> -{
> >> - Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
> >> -
> >> - method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> >> - aml_append(method,
> >> - aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> >> -
> >> - /* PCI Firmware Specification 3.0
> >> - * 4.5.1. _OSC Interface for PCI Host Bridge Devices
> >> - * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
> >> - * identified by the Universal Unique IDentifier (UUID)
> >> - * 33DB4D5B-1FF7-401C-9657-7441C03DD766
> >> - */
> >> - UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
> >> - ifctx = aml_if(aml_equal(aml_arg(0), UUID));
> >> - aml_append(ifctx,
> >> - aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
> >> - aml_append(ifctx,
> >> - aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
> >> - aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
> >> - aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
> >> -
> >> - /*
> >> - * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
> >> - * and PCIeHotplug depending on enable_native_pcie_hotplug
> >> - */
> >> - aml_append(ifctx, aml_and(aml_name("CTRL"),
> >> - aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 :
> >> 0x0)),
> >> - aml_name("CTRL")));
> >> -
> >> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
> >> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
> >> - aml_name("CDW1")));
> >> - aml_append(ifctx, ifctx1);
> >> -
> >> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"),
> >> aml_name("CTRL"))));
> >> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
> >> - aml_name("CDW1")));
> >> - aml_append(ifctx, ifctx1);
> >> -
> >> - aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
> >> - aml_append(ifctx, aml_return(aml_arg(3)));
> >> - aml_append(method, ifctx);
> >> -
> >> - elsectx = aml_else();
> >> - aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
> >> - aml_name("CDW1")));
> >> - aml_append(elsectx, aml_return(aml_arg(3)));
> >> - aml_append(method, elsectx);
> >> - return method;
> >> -}
> >> -
> >> -static Aml *build_host_bridge_dsm(void)
> >> +static Aml *build_pci_host_bridge_dsm_method(void)
> >> {
> >> Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
> >> Aml *UUID, *ifctx, *ifctx1, *buf;
> >> @@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml
> >> *dev,
> >> aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
> >> aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> >> /* Declare an _OSC (OS Control Handoff) method */
> >> - aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
> >> - aml_append(dev, build_host_bridge_dsm());
> >> + aml_append(dev,
> >> +
> >> build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
> >> + aml_append(dev, build_pci_host_bridge_dsm_method());
> >> }
> >> void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
> >
> > Otherwise:
> >
> > Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> >
> >
> > Cheers,
> > Gustavo
> >
Hi Michael, Gustavo,
On 5/24/25 12:54 PM, Michael S. Tsirkin wrote:
> On Wed, May 21, 2025 at 06:12:34PM +0200, Eric Auger wrote:
>> Hi Gustavo,
>>
>> On 5/20/25 4:09 PM, Gustavo Romero wrote:
>>> Hi Eric,
>>>
>>> On 5/14/25 14:00, Eric Auger wrote:
>>>> gpex build_host_bridge_osc() and x86 originated
>>>> build_pci_host_bridge_osc_method() are mostly identical.
>>>>
>>>> In GPEX, SUPP is set to CDW2 but is not further used. CTRL
>>>> is same as Local0.
>>>>
>>>> So let gpex code reuse build_pci_host_bridge_osc_method()
>>>> and remove build_host_bridge_osc().
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> ---
>>>>
>>>> The DSDT diff is given below:
>>>> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
>>>> index 3224a56..fa7558e 100644
>>>> --- a/dsdt.dsl_before
>>>> +++ b/dsdt.dsl_after_osc_change
>>>> @@ -5,13 +5,13 @@
>>>> *
>>>> * Disassembling to symbolic ASL+ operators
>>>> *
>>>> - * Disassembly of dsdt.dat, Mon Apr 7 05:33:06 2025
>>>> + * Disassembly of dsdt.dat, Mon Apr 7 05:37:20 2025
>>>> *
>>>> * Original Table Header:
>>>> * Signature "DSDT"
>>>> - * Length 0x00001A4F (6735)
>>>> + * Length 0x00001A35 (6709)
>>>> * Revision 0x02
>>>> - * Checksum 0xBF
>>>> + * Checksum 0xDD
>>>> * OEM ID "BOCHS "
>>>> * OEM Table ID "BXPC "
>>>> * OEM Revision 0x00000001 (1)
>>>> @@ -1849,27 +1849,26 @@ DefinitionBlock ("", "DSDT", 2, "BOCHS ",
>>>> "BXPC ", 0x00000001)
>>>> {
>>>> CreateDWordField (Arg3, 0x04, CDW2)
>>>> CreateDWordField (Arg3, 0x08, CDW3)
>>>> - SUPP = CDW2 /* \_SB_.PCI0._OSC.CDW2 */
>>>> - CTRL = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
>>>> - CTRL &= 0x1F
>>>> + Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
>>>> + Local0 &= 0x1F
>>>> If ((Arg1 != One))
>>>> {
>>>> CDW1 |= 0x08
>>>> }
>>>>
>>>> - If ((CDW3 != CTRL))
>>>> + If ((CDW3 != Local0))
>>>> {
>>>> CDW1 |= 0x10
>>>> }
>>>>
>>>> - CDW3 = CTRL /* \_SB_.PCI0.CTRL */
>>>> - Return (Arg3)
>>>> + CDW3 = Local0
>>>> }
>>>> Else
>>>> {
>>>> CDW1 |= 0x04
>>>> - Return (Arg3)
>>>> }
>>>> +
>>>> + Return (Arg3)
>>>> }
>>>>
>>>> Method (_DSM, 4, NotSerialized) // _DSM:
>>>> Device-Specific Method
>>> The problem I face with diffs in the commit body is that tools like
>>> b4, which are
>>> based on git am, get very confused on how to handle it. I'm surprised
>>> nobody ever
>>> complained about it. I'm wondering if there is any catch on it,
>>> because I have to
>>> edit commits like this manually, removing the diff, to make it finally
>>> apply to
>>> the series. Anyways, do you mind at least removing the valid diff
>>> header, like:
>>>
>>>> diff --git a/dsdt.dsl_before b/dsdt.dsl_after_osc_change
>>>> index 3224a56..fa7558e 100644
>>>> --- a/dsdt.dsl_before
>>>> +++ b/dsdt.dsl_after_osc_change
>>> from the commit message so it doesn't confuse b4?
>> Thank you for reporting the issue. in tests/qtest/bios-tables-test.c it
>> is written at the top that we shall put the diffs in disasembled ACPI
>> content in the commit msg. I will look for previously landed patches and
>> see whether the current layout can be fixed.
>>
>> Cheers
>>
>> Eric
>
> Eric, to clarify, the diff is supposed to go into commit log,
> not after ---.
> This will make it apply cleanly.
> Also, removing the "index" line as well as date diff at least is a good
> idea: the diff should be clean, not include irrelevant information.
>
> Pls feel free to clarify the text in tests/qtest/bios-tables-test.c
Of thanks. I applied your suggestions.
Eric
>
>
>
>>>
>>>> ---
>>>> hw/pci-host/gpex-acpi.c | 60 +++--------------------------------------
>>>> 1 file changed, 4 insertions(+), 56 deletions(-)
>>>>
>>>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>>>> index f1ab30f3d5..98c9868c3f 100644
>>>> --- a/hw/pci-host/gpex-acpi.c
>>>> +++ b/hw/pci-host/gpex-acpi.c
>>>> @@ -50,60 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml
>>>> *dev, uint32_t irq,
>>>> }
>>>> }
>>>> -static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
>>>> -{
>>>> - Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
>>>> -
>>>> - method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>>>> - aml_append(method,
>>>> - aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
>>>> -
>>>> - /* PCI Firmware Specification 3.0
>>>> - * 4.5.1. _OSC Interface for PCI Host Bridge Devices
>>>> - * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
>>>> - * identified by the Universal Unique IDentifier (UUID)
>>>> - * 33DB4D5B-1FF7-401C-9657-7441C03DD766
>>>> - */
>>>> - UUID = aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766");
>>>> - ifctx = aml_if(aml_equal(aml_arg(0), UUID));
>>>> - aml_append(ifctx,
>>>> - aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2"));
>>>> - aml_append(ifctx,
>>>> - aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
>>>> - aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
>>>> - aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
>>>> -
>>>> - /*
>>>> - * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
>>>> - * and PCIeHotplug depending on enable_native_pcie_hotplug
>>>> - */
>>>> - aml_append(ifctx, aml_and(aml_name("CTRL"),
>>>> - aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 :
>>>> 0x0)),
>>>> - aml_name("CTRL")));
>>>> -
>>>> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
>>>> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
>>>> - aml_name("CDW1")));
>>>> - aml_append(ifctx, ifctx1);
>>>> -
>>>> - ifctx1 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"),
>>>> aml_name("CTRL"))));
>>>> - aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x10),
>>>> - aml_name("CDW1")));
>>>> - aml_append(ifctx, ifctx1);
>>>> -
>>>> - aml_append(ifctx, aml_store(aml_name("CTRL"), aml_name("CDW3")));
>>>> - aml_append(ifctx, aml_return(aml_arg(3)));
>>>> - aml_append(method, ifctx);
>>>> -
>>>> - elsectx = aml_else();
>>>> - aml_append(elsectx, aml_or(aml_name("CDW1"), aml_int(4),
>>>> - aml_name("CDW1")));
>>>> - aml_append(elsectx, aml_return(aml_arg(3)));
>>>> - aml_append(method, elsectx);
>>>> - return method;
>>>> -}
>>>> -
>>>> -static Aml *build_host_bridge_dsm(void)
>>>> +static Aml *build_pci_host_bridge_dsm_method(void)
>>>> {
>>>> Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>>>> Aml *UUID, *ifctx, *ifctx1, *buf;
>>>> @@ -134,8 +81,9 @@ static void acpi_dsdt_add_host_bridge_methods(Aml
>>>> *dev,
>>>> aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>>>> aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>>>> /* Declare an _OSC (OS Control Handoff) method */
>>>> - aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
>>>> - aml_append(dev, build_host_bridge_dsm());
>>>> + aml_append(dev,
>>>> +
>>>> build_pci_host_bridge_osc_method(enable_native_pcie_hotplug));
>>>> + aml_append(dev, build_pci_host_bridge_dsm_method());
>>>> }
>>>> void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>> Otherwise:
>>>
>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>
>>>
>>> Cheers,
>>> Gustavo
>>>
© 2016 - 2025 Red Hat, Inc.