Re: [PATCH v2 5/8] pdx: allow per-arch optimization of PDX conversion helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2025 17:51, Roger Pau Monné wrote:
> On Tue, Jun 24, 2025 at 03:51:09PM +0200, Jan Beulich wrote:
>> On 20.06.2025 13:11, Roger Pau Monne wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/pdx.h
>>> @@ -0,0 +1,75 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#ifndef X86_PDX_H
>>> +#define X86_PDX_H
>>> +
>>> +#ifndef CONFIG_PDX_NONE
>>> +
>>> +#include <asm/alternative.h>
>>> +
>>> +/*
>>> + * Introduce a macro to avoid repeating the same asm goto block in each 
>>> helper.
>>> + * Note the macro is strictly tied to the code in the helpers.
>>> + */
>>> +#define PDX_ASM_GOTO_SKIP                           \
>>> +    asm_inline goto (                               \
>>> +        ALTERNATIVE(                                \
>>> +            "",                                     \
>>> +            "jmp %l[skip]",                         \
>>> +            ALT_NOT(X86_FEATURE_PDX_COMPRESSION))   \
>>> +        : : : : skip )
>>
>> Did you consider passing the label name as argument to the macro? That way 
>> ...
>>
>>> +static inline unsigned long pfn_to_pdx(unsigned long pfn)
>>> +{
>>> +    PDX_ASM_GOTO_SKIP;
>>> +
>>> +    return pfn_to_pdx_xlate(pfn);
>>> +
>>> + skip:
>>> +    return pfn;
>>> +}
>>
>> ... the labels here and below then wouldn't look unused.
> 
> Yes - that's why I've added the "Note the macro is strictly tied to
> the code in the helpers" comment ahead of the macro, and named it as
> "GOTO_SKIP" to explicitly reference the label name.  I could pass the
> label name however if that's preferred, ie: PDX_ASM_GOTO(skip).  IMO
> It seems a bit redundant since all callers will pass the same label
> name.

Well, that comment isn't necessarily "in sight" when looking at the
functions using the macro. Personally I'd favor passing the label as
an argument; indeed I think we would better try to do away with other
such macros where inputs are implicit. Yes, there may be cases where
that's hard or getting unwieldy. But the one here isn't one of them.

Jan