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

[Ingo Molnar <mingo@kernel.org>]

* 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