Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
On Tue, 27 Jun 2023 17:48:02 +0200 Christoph Hellwig wrote: > On Tue, Jun 27, 2023 at 01:30:06PM +0200, Petr Tesařík wrote: > > Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen > > needs bounce buffers for the PCI frontend driver, but if there is no > > other reason to have a SWIOTLB, the system does not set up one at boot. > > Please take a look at my "unexport swiotlb_active v2" series that > unfortunately missed the 6.5 merge window waiting for reviews. I noticed it, but it seems I missed the part which completely removes pci_xen_swiotlb_init_late(). Then we're left only with a reference from xen_mm_init() in arch/arm/xen/mm.c, and I believe this one can also be solved differently. Petr T
Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
On Tue, Jun 27, 2023 at 01:30:06PM +0200, Petr Tesařík wrote: > Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen > needs bounce buffers for the PCI frontend driver, but if there is no > other reason to have a SWIOTLB, the system does not set up one at boot. Please take a look at my "unexport swiotlb_active v2" series that unfortunately missed the 6.5 merge window waiting for reviews.
Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
Oops, originally sent only to Robin. Restoring the recipient list here...
On Tue, 27 Jun 2023 11:55:00 +0100
Robin Murphy wrote:
> On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote:
> > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote:
> >> +/**
> >> + * is_swiotlb_active() - check if the software IO TLB is initialized
> >> + * @dev: Device to check, or %NULL for the default IO TLB.
> >> + */
> >> bool is_swiotlb_active(struct device *dev)
> >> {
> >> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> >> + struct io_tlb_mem *mem = dev
> >> + ? dev->dma_io_tlb_mem
> >> + : _tlb_default_mem;
> >
> > That's impossible to read and maintain over time, sorry.
> >
> > Please use real "if () else" lines, so that it can be maintained over
> > time.
>
> Moreover, it makes for a horrible interface anyway. If there's a need
> for a non-specific "is SWIOTLB present at all?" check unrelated to any
> particular device (which arguably still smells of poking into
> implementation details...), please encapsulate it in its own distinct
> helper like, say, is_swiotlb_present(void).
I'm sorry for writing two replies, but I realized too late that this
part is unrelated to the MIPS Octeon platform.
Xen is the only user of an "is SWIOTLB present" interface. IIUC Xen
needs bounce buffers for the PCI frontend driver, but if there is no
other reason to have a SWIOTLB, the system does not set up one at boot.
Yeah, they should probably do things differently. At least this code in
arch/x86/kernel/pci-dma.c is fishy:
/* XXX: this switches the dma ops under live devices! */
dma_ops = _swiotlb_dma_ops;
However, I don't think it's up to me to fix that...
To sum it up, I can certainly provide a separate function instead of
overloading the is_swiotlb_active() API.
Thanks for the suggestion!
Petr T
Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
On Tue, 27 Jun 2023 11:55:00 +0100
Robin Murphy wrote:
> On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote:
> > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote:
> >> +/**
> >> + * is_swiotlb_active() - check if the software IO TLB is initialized
> >> + * @dev: Device to check, or %NULL for the default IO TLB.
> >> + */
> >> bool is_swiotlb_active(struct device *dev)
> >> {
> >> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> >> + struct io_tlb_mem *mem = dev
> >> + ? dev->dma_io_tlb_mem
> >> + : _tlb_default_mem;
> >
> > That's impossible to read and maintain over time, sorry.
> >
> > Please use real "if () else" lines, so that it can be maintained over
> > time.
>
> Moreover, it makes for a horrible interface anyway. If there's a need
> for a non-specific "is SWIOTLB present at all?" check unrelated to any
> particular device (which arguably still smells of poking into
> implementation details...), please encapsulate it in its own distinct
> helper like, say, is_swiotlb_present(void).
>
> However, the more I think about it, the more I doubt that logic like
> octeon_pci_setup() can continue to work properly at all if SWIOTLB
> allocation becomes dynamic... :/
Good, so I'm not alone. I don't know enough of the Octeon hardware to
understand how much magic is behind these PCI BARs and why one of them
should be (sometimes) programmed the way it is.
OTOH it doesn't seem to me that this platform forces DMA through
SWIOTLB. At least all calls to swiotlb_init() under arch/mips take this
form:
swiotlb_init(true, SWIOTLB_VERBOSE);
This makes me believe that this PCI BAR setup is merely an optimization.
However, if nobody has a clear answer, a fallback solution is to stay
on the safe side and add a flag to struct io_tlb_mem whether SWIOTLB
can grow dynamically. The helper function would then set this flag and
make sure that on this Octeon platform, the SWIOTLB stays restricted to
the default pool.
Hopefully, Thomas Bogendoerfer can shed some light on that code.
Petr T
Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote:
On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote:
+/**
+ * is_swiotlb_active() - check if the software IO TLB is initialized
+ * @dev: Device to check, or %NULL for the default IO TLB.
+ */
bool is_swiotlb_active(struct device *dev)
{
- struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
+ struct io_tlb_mem *mem = dev
+ ? dev->dma_io_tlb_mem
+ : _tlb_default_mem;
That's impossible to read and maintain over time, sorry.
Please use real "if () else" lines, so that it can be maintained over
time.
Moreover, it makes for a horrible interface anyway. If there's a need
for a non-specific "is SWIOTLB present at all?" check unrelated to any
particular device (which arguably still smells of poking into
implementation details...), please encapsulate it in its own distinct
helper like, say, is_swiotlb_present(void).
However, the more I think about it, the more I doubt that logic like
octeon_pci_setup() can continue to work properly at all if SWIOTLB
allocation becomes dynamic... :/
Thanks,
Robin.
Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to swiotlb.c
On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote:
> +/**
> + * is_swiotlb_active() - check if the software IO TLB is initialized
> + * @dev: Device to check, or %NULL for the default IO TLB.
> + */
> bool is_swiotlb_active(struct device *dev)
> {
> - struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> + struct io_tlb_mem *mem = dev
> + ? dev->dma_io_tlb_mem
> + : _tlb_default_mem;
That's impossible to read and maintain over time, sorry.
Please use real "if () else" lines, so that it can be maintained over
time.
thanks,
greg k-h
