x86/PV: don't half-open-code SIF_PM_MASK

Commit Message

Jan Beulich Feb. 19, 2025, 10:02 a.m. UTC

Avoid using the same literal number (8) in two distinct places.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Feb. 19, 2025, 10:29 a.m. UTC | #1

On 19/02/2025 10:02 am, Jan Beulich wrote:
> Avoid using the same literal number (8) in two distinct places.

You say two places but this is only one hunk.  I presume you mean
SIF_PM_MASK as the other place.

In which case I'd suggest that this would be clearer if phrased as "Use
MASK_INTR() to avoid opencoding the literal 8."

>
> Signed-off-by: Jan Beulich <[e-mail]>

Preferably with some kind of adjustment to the commit message, Acked-by:
Andrew Cooper <[e-mail]>

Jan Beulich Feb. 19, 2025, 10:34 a.m. UTC | #2

On 19.02.2025 11:29, Andrew Cooper wrote:
> On 19/02/2025 10:02 am, Jan Beulich wrote:
>> Avoid using the same literal number (8) in two distinct places.
> 
> You say two places but this is only one hunk.  I presume you mean
> SIF_PM_MASK as the other place.

Indeed. Somewhere there needs to be a literal number. Just that it should
be only one place rather than two. Obviously that other place isn't
touched, and hence isn't visible in the patch itself.

> In which case I'd suggest that this would be clearer if phrased as "Use
> MASK_INTR() to avoid opencoding the literal 8."

I've appended this to the sentence there was, i.e "..., using MASK_INTR()
...". To be honest, given the simplicity of the code change, I didn't
think it would be necessary to also say this verbally.

>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> Preferably with some kind of adjustment to the commit message, Acked-by:
> Andrew Cooper <[e-mail]>

Thanks.

Jan

Andrew Cooper Feb. 19, 2025, 10:48 a.m. UTC | #3

On 19/02/2025 10:34 am, Jan Beulich wrote:
> On 19.02.2025 11:29, Andrew Cooper wrote:
>> On 19/02/2025 10:02 am, Jan Beulich wrote:
>>> Avoid using the same literal number (8) in two distinct places.
>> You say two places but this is only one hunk.  I presume you mean
>> SIF_PM_MASK as the other place.
> Indeed. Somewhere there needs to be a literal number. Just that it should
> be only one place rather than two. Obviously that other place isn't
> touched, and hence isn't visible in the patch itself.
>
>> In which case I'd suggest that this would be clearer if phrased as "Use
>> MASK_INTR() to avoid opencoding the literal 8."
> I've appended this to the sentence there was, i.e "..., using MASK_INTR()
> ...". To be honest, given the simplicity of the code change, I didn't
> think it would be necessary to also say this verbally.

Honestly, you saying "two distinct places" for a while made me think
you'd forgotten a hunk.  It took longer than I care to admin to realise
that the change was in fact correct as-is.

~Andrew

Patch

--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -886,7 +886,7 @@  static int __init dom0_construct(struct
         si->flags    = SIF_PRIVILEGED | SIF_INITDOMAIN;
     if ( !vinitrd_start && initrd_len )
         si->flags   |= SIF_MOD_START_PFN;
-    si->flags       |= (xen_processor_pmbits << 8) & SIF_PM_MASK;
+    si->flags       |= MASK_INSR(xen_processor_pmbits, SIF_PM_MASK);
     si->pt_base      = vpt_start;
     si->nr_pt_frames = nr_pt_pages;
     si->mfn_list     = vphysmap_start;