Message-ID: <YzqXdBFMNvhZMMCN@lpieralisi>
Date:   Mon, 3 Oct 2022 10:04:04 +0200
From:   Lorenzo Pieralisi <lpieralisi@...nel.org>
To:     William McVicker <willmcvicker@...gle.com>
Cc:     Robin Murphy <robin.murphy@....com>,
        Serge Semin <fancer.lancer@...il.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, kernel-team@...roid.com,
        Vidya Sagar <vidyas@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Isaac J . Manjarres" <isaacmanjarres@...gle.com>
Subject: Re: [PATCH v5 1/2] PCI: dwc: Drop dependency on ZONE_DMA32

On Fri, Sep 30, 2022 at 10:02:22AM -0700, William McVicker wrote:

[...]

> > > So now we have what we have. The DMA-mask is pointlessly changed for
> > > something not really implying any DMA. That's why I insisted on
> > > dropping it at the very least. Another reason I thought was also
> > > appropriate was the default DMA-mask being set to 32bits anyway.
> > > But you said we shouldn't rely on the default DMA-mask setting. So
> > > ok, it doesn't count then. But it doesn't change the uselessness of the
> > > DMA-mask change in the current driver.
> > 
> > As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be
> > accessing memory, but it is still a thing that occupies DMA address space
> > like a mapping of memory does, and DMA masks are how we control how DMA
> > address space is allocated. Unless and until we have an API for arbitrarily
> > reserving DMA address space within a given range, the approach used here and
> > in other drivers is the next best thing, however much you don't like it.
> > 
> > > > AFAIU the correct PCI device
> > > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > > I'm not sure how to easily solve this :/
> > > 
> > > Walk over all PCIe devices detected on the PCIe-bus. Check their
> > > MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> > > make sure the MSI-base address is allocated within 4GB memory region.
> > > It isn't that easy to implement though...
> > 
> > It has nothing to do with capabilities (but also: consider hotplug). We
> > simply need the host bridge PCI device to pass to the DMA API to ensure that
> > the mapping/allocation is relative to PCI Mem space rather than system
> > physical address space, because the two don't have to be identical. The
> > challenge is how to reliably pick up that device and set up the doorbell
> > *before* any other PCI devices also discovered by pci_host_probe() have a
> > chance to start binding drivers and trying to request MSIs.
> 
> Maybe I can provide some more insights on my patches that may help you guys
> understand the idea behind the MSI capabilities flag. Basically, on a given
> Android phone there are going to be multiple PCIe endpoints -- wifi and modem
> are good examples. Some of these PCIe endpoints may only support a 32-bit MSI
> capability structure, but others could support a 64-bit MSI capability
> structure. So my intent was to have the PCIe RC driver detect the endpoint
> device's MSI capabilites during the EP device probe and then set the
> PCI_MSI_FLAGS_64BIT accordingly before calling dw_pcie_host_init(). Since the
> PCIe RC drivers are the ones to call dw_pcie_host_init(), we can dynamically
> change the DMA mask and allocate the doorbell target address based on the
> PCI_MSI_FLAGS_64BIT.

It seems to me we are all talking past each others to solve different
problems, so I am going to reset this discussion given that we are
in the merge window and I must finalise the PCI patch queue.

Patch (2/2) should be dropped IMO - I don't think the host bridge
platform device DMA mask should depend on the root port MSI cap
64/32 bit addressing - I don't think that's the right thing to do.

We should keep this discussion going for the next cycle, I will
drop patch (2/2) for the time being, sorry.

Thanks,
Lorenzo

> This all really gets a lot more complicated with the introduction of Serge's
> eDMA device. So I full well expect dw_pcie_msi_host_init() to be re-factored
> for that.
> 
> > 
> > > > Of course *this* patch doesn't change any of that either, so it's no worse
> > > > than the existing code and I don't see that dropping it helps you at all;
> > > > the current driver is already trampling your 64-bit mask back to 32 bits
> > > 
> > > Yes, and by this pathset @William intend to fix the DMA-mask-override
> > > behaviour by using the dma_alloc_coherent() method.
> > 
> > No, that is effectively unchanged. Whether it's a streaming mapping with
> > dma_mask or a coherent allocation with coherent_dma_mask, masks are getting
> > set way, it's the fact that it's on the wrong device that's the real
> > problem.
> > 
> > If you expose the eDMA as a generic dmaengine device then there's every
> > chance some consumer would make a streaming mapping with it, so even though
> > the current code happens to not override the coherent mask it's still biting
> > you in the streaming mask.
> > 
> > > So any
> > > platform-specific DMA-mask setting will be overwritten, and the
> > > DMA-mask setting won't be able to be moved/dropped due to the
> > > dma_alloc_coherent() method usage.
> > > 
> > > I have added a DW eDMA-engine support to the DW PCIe driver (you've
> > > already seen my patches) and the engine initialization is supposed to
> > > be performed after any basic initializations like CSRs mapping, data
> > > allocations, MSI, etc. Since DMA is performed by the controller itself
> > > it's required to have a correct DMA-mask set to the DW PCIe platform
> > > device otherwise any consequent mapping will be bounce buffered to the
> > > lowest 4GB even though the corresponding platform can support more
> > > than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> > > that memory. What would help me in this case if the MSI-buffer
> > > allocation procedure wouldn't change the device DMA-mask.  As an
> > > alternative to completely dropping the DMA-mask setting, the DMA-mask
> > > setup process could be moved to the low-level platform device drivers.
> > > It would be even more justified since the platform-specific device
> > > capabilities (like DW PCIe AXI-interface address-bus width) are
> > > unknown in the generic code.
> > > 
> > > As another alternative I could override the DMA-mask within the DW
> > > eDMA probe procedure. But that would make things more complicated than
> > > relying on the low-level platform drivers doing that.
> > > 
> > > > and
> > > > reserving the doorbell address in the wrong DMA address space (modulo the
> > > > other dma-ranges bug which also took far too long to figure out).
> > > 
> > > Actually current driver (without William patch) reserve the doorbell
> > > address in the correct DMA address space (if we don't take the
> > > dma-ranges settings into account).
> > 
> > No it does not. With or without this patch it is still passing the *platform
> > device* to the DMA API, which means the mapping is relative to the platform
> > address space, not PCI Mem space on the other side of the iATU. The fact
> > that the iATU's dma-ranges translation is erroneously applied to the
> > platform device at the moment is, as I have said, a bug.
> 
> Thanks for pointing this out. I agree this is a bug and I guess it hasn't
> really been a problem because there isn't really any DMA'ing going on. With the
> new eDMA device being introduced, this bug will likely need to be fixed.
> 
> > 
> > > It works as expected in case if the
> > > PCIe<->CPU space has one-on-one mapping (which is true in the most of
> > > the cases). The only thing which is wrong is the pointless DMA-mask
> > > update. I could have easily dropped it in my patchset. But after the
> > > @William patchset is applied I won't be able to do that due to using
> > > the dma_alloc_coherent() here.
> > 
> > Once again, it is not pointless. There is no guarantee that __GFP_DMA32 does
> > anything, since ZONE_DMA32 may not exist (per this patch), and the zones may
> > not be as expected anyway (look at what arm64 currently does if all RAM is
> > above 32 bits, but save those complaints for another thread).
> > 
> > > > At this
> > > > point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> > > > objectively good. If losing one page of coherent memory is a measurably
> > > > significant problem for T1 once the other issues are worked out and that
> > > > series lands, then you're welcome to propose a change on top (but I would
> > > > prefer that all the drivers using this trick are changed consistently).
> > > 
> > > Regarding DMA-coherent allocation. I am not happy with losing a whole
> > > page of the dma-coherent memory, but we can live with that. What give
> > > additional difficulty for our eDMA-patches is the DMA-mask override.
> > > If you still insist on preserving the @William patchset as it is,
> > > where do you suggest for me to update the DMA-mask if the low-level
> > > driver won't be suitable for that anymore?
> > 
> > I'm not insisting anything, it's just that this patch is already reviewed
> > and queued, is a small step towards being less wrong overall, and dropping
> > it wouldn't actually solve any of your problems anyway, so I just feel that
> > being obstructive because it falls short of perfection isn't helpful.
> 
> Thanks for the responses Robin! I agree that we don't have a complete solution
> and need to fix this DMA address space bug, but dont think that's enough of
> a reason to drop this patch series. At the very least I think patch 1/2 which
> removes the ZONE_DMA32 dependency is a worthy patch to take for 6.1.
> 
> Thanks,
> Will
> 
> > 
> > Robin.

Powered by blists - more mailing lists