Messages in this thread | | | Date | Thu, 16 Nov 2023 00:50:46 -0500 | From | "H. Peter Anvin" <> | Subject | Re: [PATCH v2] x86: Ensure input to pfn to kaddr() is treated as a 64-bit type |
| |
On November 15, 2023 5:42:31 PM EST, Michael Roth <michael.roth@amd.com> wrote: >On Wed, Nov 15, 2023 at 12:48:58PM -0800, Dave Hansen wrote: >> On 11/15/23 12:14, Michael Roth wrote: >> > While it might be argued that the issue is on the caller side, other >> > archs/macros have taken similar approaches to deal with instances like >> > this, such as commit e48866647b48 ("ARM: 8396/1: use phys_addr_t in >> > pfn_to_kaddr()"). >> >> Gah, I really hope nobody is arguing that for real, or is even thinking >> about this as a valid argument. > >Not that I'm aware, but I did have my own doubts initially, which is >why I thought it warranted a note in the commit just in case it came up >from someone else. > >> >> The helper should, well, help the caller. It makes zero sense to me >> that every single call site would need to know if the argument's type >> was big enough to hold the _return_ value. This nonsense can only even >> happen with macros. Type promotion would just do the right thing for >> any sanely declared actual helper function. > >My thought was that it is easier to expect developers to know the pitfalls >of bit-field types, since it is universally applicable to all C code, >whereas expecting developers to anticipate such issues when writing similar >macros is potentially harder to enforce/audit and could lead to similar >issues popping up as things are refactored over time and new macros get >added that don't take such usages into account. > >But neither argument seems to hold up in reality. Experienced developers >obviously do fall victim to the subtleties of of bit-field types, and >kernel devs obviously do tend to address these instances in more robust >ways based on the various pfn-related macros I looked through. > >-Mike
Now, if you are doing a cast, you are making the macro unusable for assembly anyway; any reason not to make it an inline function at that point?
| |