Date:   Tue, 14 Mar 2023 09:53:46 +0100
From:   Lorenzo Pieralisi <lpieralisi@kernel.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Shawn Guo <shawn.guo@linaro.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: PCI: Add quirk for platforms running Windows
Message-ID: <ZBA2Gl5xCjk7mMoW@lpieralisi>
References: <ZAswHyaYjeqjW/+A@lpieralisi>
 <20230310230539.GA1289856@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230310230539.GA1289856@bhelgaas>
Precedence: bulk

On Fri, Mar 10, 2023 at 05:05:39PM -0600, Bjorn Helgaas wrote:
> On Fri, Mar 10, 2023 at 02:26:55PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Mar 08, 2023 at 12:53:10PM -0600, Bjorn Helgaas wrote:
> > > On Mon, Feb 27, 2023 at 10:12:21AM +0800, Shawn Guo wrote:
> > > > Commit 8fd4391ee717 ("arm64: PCI: Exclude ACPI "consumer" resources from
> > > > host bridge windows") introduced a check to remove host bridge register
> > > > resources for all arm64 platforms, with the assumption that the PNP0A03
> > > > _CRS resources would always be host bridge registers and never as windows
> > > > on arm64.
> > > > 
> > > > The assumption stands true until Qualcomm Snapdragon Windows laptops
> > > > emerge.  These laptops describe host bridge windows in PNP0A03 _CRS
> > > > resources instead.  For example, the Microsoft Surface Pro X has host
> > > > bridges defined as
> > > > 
> > > >     Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
> > > >     Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID
> > > > 
> > > >     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> > > >     {
> > > >         Name (RBUF, ResourceTemplate ()
> > > >         {
> > > >             Memory32Fixed (ReadWrite,
> > > >                 0x60200000,         // Address Base
> > > >                 0x01DF0000,         // Address Length
> > > >                 )
> > > >             WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> > > >                 0x0000,             // Granularity
> > > >                 0x0000,             // Range Minimum
> > > >                 0x0001,             // Range Maximum
> > > >                 0x0000,             // Translation Offset
> > > >                 0x0002,             // Length
> > > >                 ,, )
> > > >         })
> > > >         Return (RBUF) /* \_SB_.PCI0._CRS.RBUF */
> > > >     }
> > > > 
> > > > The Memory32Fixed holds a host bridge window, but it's not properly
> > > > defined as a "producer" resource.  Consequently the resource gets
> > > > removed by kernel, and the BAR allocation fails later on:
> > > > 
> > > >     [ 0.150731] pci 0002:00:00.0: BAR 14: no space for [mem size 0x00100000]
> > > >     [ 0.150744] pci 0002:00:00.0: BAR 14: failed to assign [mem size 0x00100000]
> > > >     [ 0.150758] pci 0002:01:00.0: BAR 0: no space for [mem size 0x00004000 64bit]
> > > >     [ 0.150769] pci 0002:01:00.0: BAR 0: failed to assign [mem size 0x00004000 64bit]
> > > > 
> > > > This eventually prevents the PCIe NVME drive from being accessible.
> > > > 
> > > > Add a quirk for these platforms to avoid the resource being removed.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > > We are running into the issue on more devices than just Surface Pro X
> > > > now, so trying to sort it out with a quirk as suggested by Lorenzo [1].
> > > 
> > > One thing I don't like about this application of quirks is that the
> > > list of affected platforms is likely to grow, which is an ongoing
> > > burden for users and developers.
> > > 
> > > Can we have a conversation with Qualcomm about how they *intend* this
> > > to work?  Linux is probably doing something wrong (interpreting
> > > something differently than Windows does), and if we could fix that, we
> > > have a better chance of future platforms working without quirks.
> > 
> > Catch-22. What if some firmware would add host bridge MMIO register
> > space (marked as consumer) in the _CRS ? We would end up allocating
> > BAR regions in there, which is not right, so your commit:
> > 
> > 8fd4391ee717 ("arm64: PCI: Exclude ACPI "consumer" resources from host bridge windows")
> > 
> > is correct and if we revert it we would trigger regressions on some
> > arm64 platforms for the reason I mention above.
> > 
> > We can look for clarification at ACPI specs level but for firmware
> > that is out there I am not sure what options we have.
> 
> I don't remember why 8fd4391ee717 exists; I assume there was some
> platform that needed it.  I should have included that in the commit
> log; mea culpa.

I believe it is because there were arm64 platforms (early) that added a
consumer descriptor in the host bridge CRS with MMIO registers space in
it (I am not sure I can find the bug report - it has been a while,
remember the issue with non-ECAM config space and where to add the MMIO
resource required to "extend" MCFG config space ? I will never forget
that :)).

> In any event, I assume Windows works on both that platform and the
> ones mentioned in this quirk, and I assume Windows doesn't require
> platform-specific quirks for something like this.

I don't think it can work *without* quirks.

If, for reasons that I don't understand, anyone would add consumer
resources in the host bridge CRS that include at the same time PCI
memory windows and bridge MMIO registers, how can Windows detect which
one is what ? There is no way - at least none I can see.

Most likely, on platforms on which Windows boots, nobody ever added
in FW MMIO register space as a consumer resource in the host bridge
CRS (or Windows has a quirk mechanism for those).

> I admit that's a lot of assuming, but if Windows can do it, Linux
> should be able to do it, too.

I don't think it can do it, would be happy to be wrong.

I can ask, that will not solve anything but at least we know.

Lorenzo

> > > > +static struct acpi_platform_list qcom_platlist[] = {
> > > > +	/* Thinkpad X13s */
> > > > +	{ "LENOVO", "SDM8280 ", 0, ACPI_SIG_DSDT, all_versions, QCOM_DSDT_QUIRK },
> > > > +	/* Microsoft Surface Pro 9 (5G) and Windows Dev Kit 2023 */
> > > > +	{ "QCOMM ", "SDM8280 ", 0, ACPI_SIG_DSDT, all_versions, QCOM_DSDT_QUIRK },
> > > > +	/* Microsoft Surface Pro X */
> > > > +	{ "QCOMM ", "SDM8180 ", 0, ACPI_SIG_DSDT, all_versions, QCOM_DSDT_QUIRK },