| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <929647ee-e75d-2d2f-b448-7d7447448300@suse.de>
Date: Fri, 28 Jan 2022 10:39:49 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Lucas De Marchi <lucas.demarchi@...el.com>
Cc: airlied@...ux.ie, daniel.vetter@...ll.ch,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
srinivas.kandagatla@...aro.org, gregkh@...uxfoundation.org,
nouveau@...ts.freedesktop.org, christian.koenig@....com,
linux-media@...r.kernel.org
Subject: Re: [PATCH 01/14] iosys-map: Introduce renamed dma-buf-map
Hi
Am 28.01.22 um 10:24 schrieb Lucas De Marchi:
> On Fri, Jan 28, 2022 at 09:53:59AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 28.01.22 um 09:36 schrieb Lucas De Marchi:
>>> Add a new type, struct iosys_map, to eventually replace
>>> struct dma_buf_map and its helpers defiend in
>>> include/linux/dma-buf-map.h.
>>>
>>> This is mostly a copy of dma-buf-map with the renames in place and
>>> slightly different wording to avoid tying iosys_map to dma-buf: in fact
>>> it's just a shim layer to abstract system memory, that can be accessed
>>> via regular load and store, from IO memory that needs to be acessed via
>>> arch helpers. Over time the dma-buf-map proved useful outside of buffer
>>> sharing among drivers and started to be used in helper functions for
>>> allocation and generic use. See e.g.
>>>
>>> drivers/gpu/drm/drm_gem_shmem_helper.c
>>> drivers/gpu/drm/drm_gem_framebuffer_helper.c
>>> drivers/gpu/drm/drm_fb_helper.c
>>
>> Well, that was the original motivation: framebuffer memory can be
>> located in system or I/O memory, and even change their location. For
>> some architectures this makes difference. IIRC the framebuffer console
>> crashed on sparc64 because we didn't access I/O memory in the correct
>> way. Hence, we added dma_buf_map to return the type of memory from
>> dma_buf_vmap/dma_buf_vunmap. And because everything is tied together,
>> we had quite a bit of churn throughout the DRM/media code. There are
>
> Thanks for the history on this. It's helpful. My motivation for starting
> using dma_buf_map was very similar: i915 is full of direct load/store.
> It works on x86 for IO memory. While testing it on arm64 the story
> was not kind the same. Since we want to support archs other than x86 we
> need to start using some abstractions and dma_buf_map seemed a nice fit
> :)
>
>> still places in DRM where we access the raw pointers within
>> dma_buf_map. We need to clean this up at some point.
>
> at least those are easier to find than just the raw pointers without
> the abstraction
>
>>
>>>
>>> In the i915 driver it's also desired to share the implementation for
>>> integrated graphics, which uses mostly system memory, with discrete
>>> graphics, which may need to access IO memory.
>>>
>>> Once all the drivers using dma_buf_map are converted, the dma_buf_map
>>> can be retired and iosys_map extended to cover new use cases.
>>
>> Wrt the renaming: the old name isn't good and the new one isn't good
>> either. But I don't have strong feelings for either of them.
>
> :(. As the quote: "There are only two hard things in Computer Science:
> cache invalidation and naming things."
:D
>
> [ oh, and I'm also having to deal with cache invalidation to properly
> support other archs. ]
>
> At least I had 2 people saying the name was ok.
>
>>
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@...el.com>
>>> ---
>>> MAINTAINERS | 1 +
>>> include/linux/dma-buf-map.h | 3 +
>>> include/linux/iosys-map.h | 254 ++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 258 insertions(+)
>>> create mode 100644 include/linux/iosys-map.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index ea3e6c914384..27ebaded85f8 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -5734,6 +5734,7 @@ F: Documentation/driver-api/dma-buf.rst
>>> F: drivers/dma-buf/
>>> F: include/linux/*fence.h
>>> F: include/linux/dma-buf*
>>> +F: include/linux/iosys-map.h
>>
>> If anything, I'd complain tha twe now have something in dma-buf that
>> isn't obviously connected to dma-buf.
>
> Let's see if others come with a better name
Maybe we can move that file into some other module? Driver core or
memory management?
Best regards
Thomas
>
> thanks
> Lucas De Marchi
>
>>
>> Best regards
>> Thomas
>>
>>> F: include/linux/dma-resv.h
>>> K: \bdma_(?:buf|fence|resv)\b
>>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>>> index 19fa0b5ae5ec..4b4b2930660b 100644
>>> --- a/include/linux/dma-buf-map.h
>>> +++ b/include/linux/dma-buf-map.h
>>> @@ -263,4 +263,7 @@ static inline void dma_buf_map_incr(struct
>>> dma_buf_map *map, size_t incr)
>>> map->vaddr += incr;
>>> }
>>> +/* Temporary include for API migration */
>>> +#include <linux/iosys-map.h>
>>> +
>>> #endif /* __DMA_BUF_MAP_H__ */
>>> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>>> new file mode 100644
>>> index 000000000000..ad1f08f8f97f
>>> --- /dev/null
>>> +++ b/include/linux/iosys-map.h
>>> @@ -0,0 +1,254 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Pointer abstraction for IO/system memory
>>> + */
>>> +
>>> +#ifndef __IOSYS_MAP_H__
>>> +#define __IOSYS_MAP_H__
>>> +
>>> +#include <linux/io.h>
>>> +#include <linux/string.h>
>>> +
>>> +/* Temporary include while user of dma-buf-map are converted to
>>> iosys-map */
>>> +#include <linux/dma-buf-map.h>
>>> +
>>> +/**
>>> + * DOC: overview
>>> + *
>>> + * When accessing a memory region, depending on the its location,
>>> users may have
>>> + * to access it with I/O operations or memory load/store operations.
>>> For
>>> + * example, copying to system memory could be done with memcpy(),
>>> copying to I/O
>>> + * memory would be done with memcpy_toio().
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * void *vaddr = ...; // pointer to system memory
>>> + * memcpy(vaddr, src, len);
>>> + *
>>> + * void *vaddr_iomem = ...; // pointer to I/O memory
>>> + * memcpy_toio(vaddr, _iomem, src, len);
>>> + *
>>> + * The user of such pointer may not have information about the
>>> mapping of that
>>> + * region or may want to have a single code path to handle
>>> operations on that
>>> + * buffer, regardless if it's located in system or IO memory. The type
>>> + * :c:type:`struct iosys_map <iosys_map>` and its helpers abstract
>>> that so the
>>> + * buffer can be passed around to other drivers or have separate
>>> duties inside
>>> + * the same driver for allocation, read and write operations.
>>> + *
>>> + * Open-coding access to :c:type:`struct iosys_map <iosys_map>` is
>>> considered
>>> + * bad style. Rather then accessing its fields directly, use one of
>>> the provided
>>> + * helper functions, or implement your own. For example, instances of
>>> + * :c:type:`struct iosys_map <iosys_map>` can be initialized
>>> statically with
>>> + * IOSYS_MAP_INIT_VADDR(), or at runtime with iosys_map_set_vaddr().
>>> These
>>> + * helpers will set an address in system memory.
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * struct iosys_map map = IOSYS_MAP_INIT_VADDR(0xdeadbeaf);
>>> + *
>>> + * iosys_map_set_vaddr(&map, 0xdeadbeaf);
>>> + *
>>> + * To set an address in I/O memory, use iosys_map_set_vaddr_iomem().
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * iosys_map_set_vaddr_iomem(&map, 0xdeadbeaf);
>>> + *
>>> + * Instances of struct iosys_map do not have to be cleaned up, but
>>> + * can be cleared to NULL with iosys_map_clear(). Cleared mappings
>>> + * always refer to system memory.
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * iosys_map_clear(&map);
>>> + *
>>> + * Test if a mapping is valid with either iosys_map_is_set() or
>>> + * iosys_map_is_null().
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * if (iosys_map_is_set(&map) != iosys_map_is_null(&map))
>>> + * // always true
>>> + *
>>> + * Instances of :c:type:`struct iosys_map <iosys_map>` can be
>>> compared for
>>> + * equality with iosys_map_is_equal(). Mappings that point to
>>> different memory
>>> + * spaces, system or I/O, are never equal. That's even true if both
>>> spaces are
>>> + * located in the same address space, both mappings contain the same
>>> address
>>> + * value, or both mappings refer to NULL.
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * struct iosys_map sys_map; // refers to system memory
>>> + * struct iosys_map io_map; // refers to I/O memory
>>> + *
>>> + * if (iosys_map_is_equal(&sys_map, &io_map))
>>> + * // always false
>>> + *
>>> + * A set up instance of struct iosys_map can be used to access or
>>> manipulate the
>>> + * buffer memory. Depending on the location of the memory, the
>>> provided helpers
>>> + * will pick the correct operations. Data can be copied into the
>>> memory with
>>> + * iosys_map_memcpy_to(). The address can be manipulated with
>>> iosys_map_incr().
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * const void *src = ...; // source buffer
>>> + * size_t len = ...; // length of src
>>> + *
>>> + * iosys_map_memcpy_to(&map, src, len);
>>> + * iosys_map_incr(&map, len); // go to first byte after the memcpy
>>> + */
>>> +
>>> +/**
>>> + * struct iosys_map - Pointer to IO/system memory
>>> + * @vaddr_iomem: The buffer's address if in I/O memory
>>> + * @vaddr: The buffer's address if in system memory
>>> + * @is_iomem: True if the buffer is located in I/O memory, or
>>> false
>>> + * otherwise.
>>> + */
>>> +#define iosys_map dma_buf_map
>>> +
>>> +/**
>>> + * IOSYS_MAP_INIT_VADDR - Initializes struct iosys_map to an address
>>> in system memory
>>> + * @vaddr_: A system-memory address
>>> + */
>>> +#define IOSYS_MAP_INIT_VADDR(vaddr_) \
>>> + { \
>>> + .vaddr = (vaddr_), \
>>> + .is_iomem = false, \
>>> + }
>>> +
>>> +/**
>>> + * iosys_map_set_vaddr - Sets a iosys mapping structure to an
>>> address in system memory
>>> + * @map: The iosys_map structure
>>> + * @vaddr: A system-memory address
>>> + *
>>> + * Sets the address and clears the I/O-memory flag.
>>> + */
>>> +static inline void iosys_map_set_vaddr(struct iosys_map *map, void
>>> *vaddr)
>>> +{
>>> + map->vaddr = vaddr;
>>> + map->is_iomem = false;
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_set_vaddr_iomem - Sets a iosys mapping structure to an
>>> address in I/O memory
>>> + * @map: The iosys_map structure
>>> + * @vaddr_iomem: An I/O-memory address
>>> + *
>>> + * Sets the address and the I/O-memory flag.
>>> + */
>>> +static inline void iosys_map_set_vaddr_iomem(struct iosys_map *map,
>>> + void __iomem *vaddr_iomem)
>>> +{
>>> + map->vaddr_iomem = vaddr_iomem;
>>> + map->is_iomem = true;
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_is_equal - Compares two iosys mapping structures for
>>> equality
>>> + * @lhs: The iosys_map structure
>>> + * @rhs: A iosys_map structure to compare with
>>> + *
>>> + * Two iosys mapping structures are equal if they both refer to the
>>> same type of memory
>>> + * and to the same address within that memory.
>>> + *
>>> + * Returns:
>>> + * True is both structures are equal, or false otherwise.
>>> + */
>>> +static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
>>> + const struct iosys_map *rhs)
>>> +{
>>> + if (lhs->is_iomem != rhs->is_iomem)
>>> + return false;
>>> + else if (lhs->is_iomem)
>>> + return lhs->vaddr_iomem == rhs->vaddr_iomem;
>>> + else
>>> + return lhs->vaddr == rhs->vaddr;
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_is_null - Tests for a iosys mapping to be NULL
>>> + * @map: The iosys_map structure
>>> + *
>>> + * Depending on the state of struct iosys_map.is_iomem, tests if the
>>> + * mapping is NULL.
>>> + *
>>> + * Returns:
>>> + * True if the mapping is NULL, or false otherwise.
>>> + */
>>> +static inline bool iosys_map_is_null(const struct iosys_map *map)
>>> +{
>>> + if (map->is_iomem)
>>> + return !map->vaddr_iomem;
>>> + return !map->vaddr;
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_is_set - Tests if the iosys mapping has been set
>>> + * @map: The iosys_map structure
>>> + *
>>> + * Depending on the state of struct iosys_map.is_iomem, tests if the
>>> + * mapping has been set.
>>> + *
>>> + * Returns:
>>> + * True if the mapping is been set, or false otherwise.
>>> + */
>>> +static inline bool iosys_map_is_set(const struct iosys_map *map)
>>> +{
>>> + return !iosys_map_is_null(map);
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_clear - Clears a iosys mapping structure
>>> + * @map: The iosys_map structure
>>> + *
>>> + * Clears all fields to zero, including struct iosys_map.is_iomem, so
>>> + * mapping structures that were set to point to I/O memory are reset
>>> for
>>> + * system memory. Pointers are cleared to NULL. This is the default.
>>> + */
>>> +static inline void iosys_map_clear(struct iosys_map *map)
>>> +{
>>> + if (map->is_iomem) {
>>> + map->vaddr_iomem = NULL;
>>> + map->is_iomem = false;
>>> + } else {
>>> + map->vaddr = NULL;
>>> + }
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_memcpy_to - Memcpy into iosys mapping
>>> + * @dst: The iosys_map structure
>>> + * @src: The source buffer
>>> + * @len: The number of byte in src
>>> + *
>>> + * Copies data into a iosys mapping. The source buffer is in system
>>> + * memory. Depending on the buffer's location, the helper picks the
>>> correct
>>> + * method of accessing the memory.
>>> + */
>>> +static inline void iosys_map_memcpy_to(struct iosys_map *dst, const
>>> void *src,
>>> + size_t len)
>>> +{
>>> + if (dst->is_iomem)
>>> + memcpy_toio(dst->vaddr_iomem, src, len);
>>> + else
>>> + memcpy(dst->vaddr, src, len);
>>> +}
>>> +
>>> +/**
>>> + * iosys_map_incr - Increments the address stored in a iosys mapping
>>> + * @map: The iosys_map structure
>>> + * @incr: The number of bytes to increment
>>> + *
>>> + * Increments the address stored in a iosys mapping. Depending on the
>>> + * buffer's location, the correct value will be updated.
>>> + */
>>> +static inline void iosys_map_incr(struct iosys_map *map, size_t incr)
>>> +{
>>> + if (map->is_iomem)
>>> + map->vaddr_iomem += incr;
>>> + else
>>> + map->vaddr += incr;
>>> +}
>>> +
>>> +#endif /* __IOSYS_MAP_H__ */
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists