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.au...@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.rom...@linaro.org>
>>>
>>>
>>> Cheers,
>>> Gustavo
>>>
