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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

> The other slight anomaly with this is that we're wasting 2 or 5 bytes of
> code space. Yet I guess that's an acceptable price to pay for keeping the
> actual translation code in C (rather than in assembly).

I wanted to avoid doing the translation in assembly, so I think it's a
fair price to pay.

Thanks, Roger.