[PATCH 9/9] iosys_map: embed the is_iomem bit into the pointer.

[Thomas Zimmermann]

Hi

Am 22.05.25 um 17:09 schrieb Lucas De Marchi:
> On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This reduces this struct from 16 to 8 bytes, and it gets embedded
>> into a lot of things.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>
> Replied too early on cover. Chatting with Michal Wajdeczko today, this
> may break things as we then can't byte-address anymore. It seems
> particularly dangerous when using the iosys_map_wr/iosys_map_rd as
> there's nothing preventing an unaligned address and we increment the map
> with the sizeof() of a struct that could be __packed. Example: in
> xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
> In this particular case it doesn't give unaligned address, but we should
> probably then protect iosys_map from doing the wrong thing.
>
> So, if we are keeping this patch, we should probably protect
> initially-unaligned addresses and the iosys_map_incr() call?

That sounds like a blocker to me. And there's another thing to keep in 
mind. We have use cases where we need to know the caching of the memory 
area. I never got to fully implement this, but it would be stored in the 
iosys-map struct as well. We'd need 2 additional bits to represent UC, 
WC and WT caching. If we don't have at least 3-bit alignment, shrinking 
iosys-map might not be feasible anyway.

Best regards
Thomas

>
> thanks
> Lucas De Marchi
>
>> ---
>> include/linux/iosys-map.h | 30 ++++++++----------------------
>> 1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
>> index a6c2cc9ca756..44479966ce24 100644
>> --- a/include/linux/iosys-map.h
>> +++ b/include/linux/iosys-map.h
>> @@ -99,29 +99,27 @@
>>  *    iosys_map_incr(&map, len); // go to first byte after the memcpy
>>  */
>>
>> +#define _IOSYS_MAP_IS_IOMEM 1
>> /**
>>  * 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.
>>  */
>> struct iosys_map {
>>     union {
>>         void __iomem *_vaddr_iomem;
>>         void *_vaddr;
>>     };
>> -    bool _is_iomem;
>> };
>>
>> static inline bool iosys_map_is_iomem(const struct iosys_map *map)
>> {
>> -    return map->_is_iomem;
>> +    return ((unsigned long)map->_vaddr) & _IOSYS_MAP_IS_IOMEM;
>> }
>>
>> static inline void __iomem *iosys_map_ioptr(const struct iosys_map *map)
>> {
>> -    return map->_vaddr_iomem;
>> +    return (void __iomem *)((unsigned long)map->_vaddr_iomem & 
>> ~_IOSYS_MAP_IS_IOMEM);
>> }
>>
>> static inline void *iosys_map_ptr(const struct iosys_map *map)
>> @@ -136,7 +134,6 @@ static inline void *iosys_map_ptr(const struct 
>> iosys_map *map)
>> #define IOSYS_MAP_INIT_VADDR(vaddr_)    \
>>     {                \
>>         ._vaddr = (vaddr_),    \
>> -        ._is_iomem = false,    \
>>     }
>>
>> /**
>> @@ -145,8 +142,7 @@ static inline void *iosys_map_ptr(const struct 
>> iosys_map *map)
>>  */
>> #define IOSYS_MAP_INIT_VADDR_IOMEM(vaddr_iomem_)    \
>>     {                        \
>> -        ._vaddr_iomem = (vaddr_iomem_),        \
>> -        ._is_iomem = true,            \
>> +        ._vaddr_iomem = (void __iomem *)(((unsigned 
>> long)(vaddr_iomem_) | _IOSYS_MAP_IS_IOMEM)), \
>>     }
>>
>> /**
>> @@ -198,7 +194,6 @@ static inline void *iosys_map_ptr(const struct 
>> iosys_map *map)
>> static inline void iosys_map_set_vaddr(struct iosys_map *map, void 
>> *vaddr)
>> {
>>     map->_vaddr = vaddr;
>> -    map->_is_iomem = false;
>> }
>>
>> /**
>> @@ -211,8 +206,7 @@ static inline void iosys_map_set_vaddr(struct 
>> iosys_map *map, void *vaddr)
>> 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;
>> +    map->_vaddr_iomem = (void __iomem *)((unsigned long)vaddr_iomem 
>> | _IOSYS_MAP_IS_IOMEM);
>> }
>>
>> /**
>> @@ -229,12 +223,9 @@ static inline void 
>> iosys_map_set_vaddr_iomem(struct iosys_map *map,
>> static inline bool iosys_map_is_equal(const struct iosys_map *lhs,
>>                       const struct iosys_map *rhs)
>> {
>> -    if (lhs->_is_iomem != rhs->_is_iomem)
>> +    if (lhs->_vaddr != rhs->_vaddr)
>>         return false;
>> -    else if (lhs->_is_iomem)
>> -        return lhs->_vaddr_iomem == rhs->_vaddr_iomem;
>> -    else
>> -        return lhs->_vaddr == rhs->_vaddr;
>> +    return true;
>> }
>>
>> /**
>> @@ -279,12 +270,7 @@ static inline bool iosys_map_is_set(const struct 
>> iosys_map *map)
>>  */
>> 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;
>> -    }
>> +    map->_vaddr = NULL;
>> }
>>
>> /**
>> -- 
>> 2.49.0
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)