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

[Jocelyn Falempe]

On 22/05/2025 23:05, Lucas De Marchi wrote:
> On Fri, May 23, 2025 at 06:32:43AM +1000, Dave Airlie wrote:
>> On Fri, 23 May 2025 at 01:10, Lucas De Marchi 
>> <lucas.demarchi at intel.com> wrote:
>>>
>>> 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?
>>
>> oh interesting, my thoughts was of course nobody would want to use
>> this for < 32-byte aligned ptrs :-)
>>
>> but I forgot about using the incr for stuff, I do wonder if the incr
>> could be modelled on a base + offset, as I do think for a lot of stuff
>> we'd want to retain the original vaddr for unmapping or other things,
> 
> for the parts of the code that want to update "inner blocks", the
> approach is to copy the struct and operate on that. Example from
> drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:guc_prep_golden_context:
> 
>      info_map = IOSYS_MAP_INIT_OFFSET(&guc->ads_map,
>                       offsetof(struct __guc_ads_blob, system_info));
> 
> then that function can manipulate its "local map" without affecting the
> one used for really mapping the memory.

Also drm_panic uses that to draw into the frame buffer:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_panic.c#L181

So in case of 16bits or 24bits pixel width, it might be unaligned.
As it is used only to save one argument to the "blit" function, I think 
saving 8 bytes in the iosys_map struct, and using an additional offset 
argument is doable.

It is also used the same way in drm_format_helper:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_format_helper.c#L290

Best regards,

-- 

Jocelyn

> 
>>
>> I'll play around a bit more next week with at least protecting against 
>> bad uses.
> 
> thanks
> Lucas De Marchi
> 
>>
>> Dave.
>