Re: [RFC PATCH 7/9] x86/mm: Introduce Remote Action Request

Ingo Molnar
2025-05-06T16:31:38Z

* Rik van Riel <riel@surriel.com> wrote:

> +++ b/arch/x86/include/asm/rar.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_RAR_H
> +#define _ASM_X86_RAR_H
> +
> +/*
> + * RAR payload types
> + */
> +#define RAR_TYPE_INVPG 0
> +#define RAR_TYPE_INVPG_NO_CR3 1
> +#define RAR_TYPE_INVPCID 2
> +#define RAR_TYPE_INVEPT 3
> +#define RAR_TYPE_INVVPID 4
> +#define RAR_TYPE_WRMSR 5
> +
> +/*
> + * Subtypes for RAR_TYPE_INVLPG
> + */
> +#define RAR_INVPG_ADDR 0 /* address specific */
> +#define RAR_INVPG_ALL 2 /* all, include global */
> +#define RAR_INVPG_ALL_NO_GLOBAL 3 /* all, exclude global */
> +
> +/*
> + * Subtypes for RAR_TYPE_INVPCID
> + */
> +#define RAR_INVPCID_ADDR 0 /* address specific */
> +#define RAR_INVPCID_PCID 1 /* all of PCID */
> +#define RAR_INVPCID_ALL 2 /* all, include global */
> +#define RAR_INVPCID_ALL_NO_GLOBAL 3 /* all, exclude global */
> +
> +/*
> + * Page size for RAR_TYPE_INVLPG
> + */
> +#define RAR_INVLPG_PAGE_SIZE_4K 0
> +#define RAR_INVLPG_PAGE_SIZE_2M 1
> +#define RAR_INVLPG_PAGE_SIZE_1G 2
> +
> +/*
> + * Max number of pages per payload
> + */
> +#define RAR_INVLPG_MAX_PAGES 63
> +
> +typedef struct {
> + uint64_t for_sw : 8;
> + uint64_t type : 8;
> + uint64_t must_be_zero_1 : 16;
> + uint64_t subtype : 3;
> + uint64_t page_size: 2;
> + uint64_t num_pages : 6;
> + uint64_t must_be_zero_2 : 21;
> +
> + uint64_t must_be_zero_3;
> +
> + /*
> + * Starting address
> + */
> + uint64_t initiator_cr3;
> + uint64_t linear_address;
> +
> + /*
> + * Padding
> + */
> + uint64_t padding[4];
> +} rar_payload_t;

- Please don't use _t typedefs for complex types. 'struct rar_payload'
should be good enough.

- Please use u64/u32 for HW ABI definitions.

- Please align bitfield definitions vertically, for better readability:

u64 for_sw : 8;
u64 type : 8;
u64 must_be_zero_1 : 16;
u64 subtype : 3;
u64 page_size : 2;
u64 num_pages : 6;
u64 must_be_zero_2 : 21;

> +++ b/arch/x86/mm/rar.c
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * RAR Tlb shootdown

s/Tlb
/TLB

> +#include <linux/kgdb.h>

Is this really needed? There's nothing KGDB specific in here AFAICS.

> +static DEFINE_PER_CPU_ALIGNED(u64[(RAR_MAX_PAYLOADS + 8) / 8], rar_action);

> +static void set_action_entry(unsigned long idx, int target_cpu)
> +{
> + u8 *bitmap = (u8 *)per_cpu(rar_action, target_cpu);

> + u8 *bitmap = (u8 *)per_cpu(rar_action, target_cpu);

> + bitmap = (u8 *)per_cpu(rar_action, this_cpu);

So AFAICS all these ugly, forced type casts tp (u8 *) are needed only
because rar_action has the wrong type: if it were an u8[], then these
lines could be:

static DEFINE_PER_CPU_ALIGNED(u8[RAR_MAX_PAYLOADS], rar_action);

...

u8 *bitmap = per_cpu(rar_action, target_cpu);

right?

Thanks,

Ingo