* [PATCH v2 2/5] HID: hid-oxp: Add Second Generation RGB Control
From: Derek J. Clark @ 2026-04-07 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Lambert Fan, Derek J . Clark,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20260407041354.2283201-1-derekjohn.clark@gmail.com>
Adds support for the second generation of RGB Control for OneXPlayer
devices. The interface mirrors the first generation, with some
differences to how messages are formatted.
Some devices have both a GEN1 MCU for RGB control and a GEN2 MCU for
button mapping. To avoid conflicts, quirk these devices to skip RGB
setup for the GEN2_USAGE_PAGE.
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
v2:
- Add DMI quirks table.
---
drivers/hid/Kconfig | 1 +
drivers/hid/hid-ids.h | 3 +
drivers/hid/hid-oxp.c | 151 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 155 insertions(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 2deaec9f467d..b779088b80b6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -924,6 +924,7 @@ config HID_OXP
depends on USB_HID
depends on LEDS_CLASS
depends on LEDS_CLASS_MULTICOLOR
+ depends on DMI
help
Say Y here if you would like to enable support for OneXPlayer handheld
devices that come with RGB LED rings around the joysticks and macro buttons.
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index dcc5a3a70eaf..0d1ff879e959 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1134,6 +1134,9 @@
#define USB_VENDOR_ID_CRSC 0x1a2c
#define USB_DEVICE_ID_ONEXPLAYER_GEN1 0xb001
+#define USB_VENDOR_ID_WCH 0x1a86
+#define USB_DEVICE_ID_ONEXPLAYER_GEN2 0xfe00
+
#define USB_VENDOR_ID_ONTRAK 0x0a07
#define USB_DEVICE_ID_ONTRAK_ADU100 0x0064
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
index c4219ecd8d71..25214356163e 100644
--- a/drivers/hid/hid-oxp.c
+++ b/drivers/hid/hid-oxp.c
@@ -10,6 +10,7 @@
#include <linux/delay.h>
#include <linux/dev_printk.h>
#include <linux/device.h>
+#include <linux/dmi.h>
#include <linux/hid.h>
#include <linux/jiffies.h>
#include <linux/kstrtox.h>
@@ -24,12 +25,15 @@
#define OXP_PACKET_SIZE 64
#define GEN1_MESSAGE_ID 0xff
+#define GEN2_MESSAGE_ID 0x3f
#define GEN1_USAGE_PAGE 0xff01
+#define GEN2_USAGE_PAGE 0xff00
enum oxp_function_index {
OXP_FID_GEN1_RGB_SET = 0x07,
OXP_FID_GEN1_RGB_REPLY = 0x0f,
+ OXP_FID_GEN2_STATUS_EVENT = 0xb8,
};
static struct oxp_hid_cfg {
@@ -121,6 +125,22 @@ struct oxp_gen_1_rgb_report {
u8 blue;
} __packed;
+struct oxp_gen_2_rgb_report {
+ u8 report_id;
+ u8 header_id;
+ u8 padding_2;
+ u8 message_id;
+ u8 padding_4[2];
+ u8 enabled;
+ u8 speed;
+ u8 brightness;
+ u8 red;
+ u8 green;
+ u8 blue;
+ u8 padding_12[3];
+ u8 effect;
+} __packed;
+
static u16 get_usage_page(struct hid_device *hdev)
{
return hdev->collection[0].usage >> 16;
@@ -161,6 +181,44 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
return 0;
}
+static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct led_classdev_mc *led_mc = drvdata.led_mc;
+ struct oxp_gen_2_rgb_report *rgb_rep;
+
+ if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
+ return 0;
+
+ if (data[3] != OXP_GET_PROPERTY)
+ return 0;
+
+ rgb_rep = (struct oxp_gen_2_rgb_report *)data;
+ /* Ensure we save monocolor as the list value */
+ drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ OXP_EFFECT_MONO_LIST :
+ rgb_rep->effect;
+ drvdata.rgb_speed = rgb_rep->speed;
+ drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ OXP_FEAT_ENABLED;
+ drvdata.rgb_brightness = rgb_rep->brightness;
+ led_mc->led_cdev.brightness = rgb_rep->brightness / 4 *
+ led_mc->led_cdev.max_brightness;
+ /* If monocolor had less than 100% brightness on the previous boot,
+ * there will be no reliable way to determine the real intensity.
+ * Since intensity scaling is used with a hardware brightness set at max,
+ * our brightness will always look like 100%. Use the last set value to
+ * prevent successive boots from lowering the brightness further.
+ * Brightness will be "wrong" but the effect will remain the same visually.
+ */
+ led_mc->subled_info[0].intensity = rgb_rep->red;
+ led_mc->subled_info[1].intensity = rgb_rep->green;
+ led_mc->subled_info[2].intensity = rgb_rep->blue;
+
+ return 0;
+}
+
static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
u8 *data, int size)
{
@@ -171,6 +229,8 @@ static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
switch (up) {
case GEN1_USAGE_PAGE:
return oxp_hid_raw_event_gen_1(hdev, report, data, size);
+ case GEN2_USAGE_PAGE:
+ return oxp_hid_raw_event_gen_2(hdev, report, data, size);
default:
break;
}
@@ -216,6 +276,18 @@ static int oxp_gen_1_property_out(enum oxp_function_index fid, u8 *data,
return mcu_property_out(header, header_size, data, data_size, NULL, 0);
}
+static int oxp_gen_2_property_out(enum oxp_function_index fid, u8 *data,
+ u8 data_size)
+{
+ u8 header[] = { fid, GEN2_MESSAGE_ID, 0x01 };
+ u8 footer[] = { GEN2_MESSAGE_ID, fid };
+ size_t header_size = ARRAY_SIZE(header);
+ size_t footer_size = ARRAY_SIZE(footer);
+
+ return mcu_property_out(header, header_size, data, data_size, footer,
+ footer_size);
+}
+
static int oxp_rgb_status_store(u8 enabled, u8 speed, u8 brightness)
{
u16 up = get_usage_page(drvdata.hdev);
@@ -230,6 +302,11 @@ static int oxp_rgb_status_store(u8 enabled, u8 speed, u8 brightness)
if (drvdata.rgb_effect == OXP_EFFECT_MONO_LIST)
data[3] = 0x04;
return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 4);
+ case GEN2_USAGE_PAGE:
+ data = (u8[6]) { OXP_SET_PROPERTY, 0x00, 0x02, enabled, speed, brightness };
+ if (drvdata.rgb_effect == OXP_EFFECT_MONO_LIST)
+ data[5] = 0x04;
+ return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 6);
default:
return -ENODEV;
}
@@ -244,6 +321,9 @@ static ssize_t oxp_rgb_status_show(void)
case GEN1_USAGE_PAGE:
data = (u8[1]) { OXP_GET_PROPERTY };
return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
+ case GEN2_USAGE_PAGE:
+ data = (u8[3]) { OXP_GET_PROPERTY, 0x00, 0x02 };
+ return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 3);
default:
return -ENODEV;
}
@@ -274,6 +354,16 @@ static int oxp_rgb_color_set(void)
data[3 * i + 3] = blue;
}
return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, size);
+ case GEN2_USAGE_PAGE:
+ size = 57;
+ data = (u8[57]) { OXP_EFFECT_MONO_TRUE, 0x00, 0x02 };
+
+ for (i = 1; i < size / 3; i++) {
+ data[3 * i] = red;
+ data[3 * i + 1] = green;
+ data[3 * i + 2] = blue;
+ }
+ return oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, size);
default:
return -ENODEV;
}
@@ -310,6 +400,10 @@ static int oxp_rgb_effect_set(u8 effect)
data = (u8[1]) { effect };
ret = oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
break;
+ case GEN2_USAGE_PAGE:
+ data = (u8[3]) { effect, 0x00, 0x02 };
+ ret = oxp_gen_2_property_out(OXP_FID_GEN2_STATUS_EVENT, data, 3);
+ break;
default:
ret = -ENODEV;
}
@@ -560,6 +654,56 @@ static struct led_classdev_mc oxp_cdev_rgb = {
.subled_info = oxp_rgb_subled_info,
};
+struct quirk_entry {
+ bool hybrid_mcu;
+};
+
+static struct quirk_entry quirk_hybrid_mcu = {
+ .hybrid_mcu = true,
+};
+
+static const struct dmi_system_id oxp_hybrid_mcu_list[] = {
+ {
+ .ident = "OneXPlayer Apex",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER APEX"),
+ },
+ .driver_data = &quirk_hybrid_mcu,
+ },
+ {
+ .ident = "OneXPlayer G1 AMD",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER G1 A"),
+ },
+ .driver_data = &quirk_hybrid_mcu,
+ },
+ {
+ .ident = "OneXPlayer G1 Intel",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ONE-NETBOOK"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "ONEXPLAYER G1 i"),
+ },
+ .driver_data = &quirk_hybrid_mcu,
+ },
+ {},
+};
+
+static bool oxp_hybrid_mcu_device(void)
+{
+ const struct dmi_system_id *dmi_id;
+ struct quirk_entry *quirks;
+
+ dmi_id = dmi_first_match(oxp_hybrid_mcu_list);
+ if (!dmi_id)
+ return false;
+
+ quirks = dmi_id->driver_data;
+
+ return quirks->hybrid_mcu;
+}
+
static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
{
int ret;
@@ -567,6 +711,10 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
hid_set_drvdata(hdev, &drvdata);
mutex_init(&drvdata.cfg_mutex);
drvdata.hdev = hdev;
+
+ if (up == GEN2_USAGE_PAGE && oxp_hybrid_mcu_device())
+ goto skip_rgb;
+
drvdata.led_mc = &oxp_cdev_rgb;
ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);
@@ -585,6 +733,7 @@ static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
dev_warn(drvdata.led_mc->led_cdev.dev,
"Failed to query RGB initial state: %i\n", ret);
+skip_rgb:
return 0;
}
@@ -613,6 +762,7 @@ static int oxp_hid_probe(struct hid_device *hdev,
switch (up) {
case GEN1_USAGE_PAGE:
+ case GEN2_USAGE_PAGE:
ret = oxp_cfg_probe(hdev, up);
if (ret) {
hid_hw_close(hdev);
@@ -633,6 +783,7 @@ static void oxp_hid_remove(struct hid_device *hdev)
static const struct hid_device_id oxp_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CRSC, USB_DEVICE_ID_ONEXPLAYER_GEN1) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_WCH, USB_DEVICE_ID_ONEXPLAYER_GEN2) },
{}
};
--
2.53.0
^ permalink raw reply related
* [PATCH v2 1/5] HID: hid-oxp: Add OneXPlayer configuration driver
From: Derek J. Clark @ 2026-04-07 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Lambert Fan, Derek J . Clark,
linux-input, linux-doc, linux-kernel
In-Reply-To: <20260407041354.2283201-1-derekjohn.clark@gmail.com>
Adds OneXPlayer HID configuration driver. In this initial driver patch,
add the RGB interface for the first generation of HID based RGB control.
This interface provides the following attributes:
- brightness: provided by the LED core, this works in a fairly unique
way on this device. The hardware accepts 5 brightness values (0-4),
which affects the brightness of the multicolor and animated effects
built into the MCU firmware. For monocolor settings, the device
expects the hardware brightness value to be pushed to maximum, then we
apply brightness adjustments mathematically based on % (0-100). This
leads to some odd conversion as we need the brightness slider to reach
the full range, but it has no affect when incrementing between the
division points for other effects.
- multi-intensity: provided by the LED core for red, green, and blue.
- effect: Allows the MCU to set 19 individual effects.
- effect_index: Lists the 19 valid effect names for the interface.
- enabled: Allows the MCU to toggle the RGB interface on/off.
- enabled_index: Lists the valid states for enabled.
- speed: Allows the MCU to set the animation rate for the various
effects.
- speed_range: Lists the valid range of speed (0-9).
The MCU also has a few odd quirks that make sending multiple synchronous
events challenging. It will essentially freeze if it receives another
message before it has finished processing the last command. It also will
not reply if you wait on it using a completion. To get around this, we
do a 200ms sleep inside a work queue thread and debounce all but the most
recent message using a 50ms mod_delayed_work. This will cache the last
write, queue the work, then return so userspace can release its write
thread. The work queue is only used for brightness/multi-intensity as
that is the path likely to receive rapid successive writes.
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 12 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 3 +
drivers/hid/hid-oxp.c | 651 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 673 insertions(+)
create mode 100644 drivers/hid/hid-oxp.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 6f6517bf4f97..dae814192fa4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19707,6 +19707,12 @@ S: Maintained
F: drivers/mtd/nand/onenand/
F: include/linux/mtd/onenand*.h
+ONEXPLAYER HID DRIVER
+M: Derek J. Clark <derekjohn.clark@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-oxp.c
+
ONEXPLAYER PLATFORM EC DRIVER
M: Antheas Kapenekakis <lkml@antheas.dev>
M: Derek John Clark <derekjohn.clark@gmail.com>
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3c034cd32fa8..2deaec9f467d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -919,6 +919,18 @@ config HID_ORTEK
- Ortek WKB-2000
- Skycable wireless presenter
+config HID_OXP
+ tristate "OneXPlayer handheld controller configuration support"
+ depends on USB_HID
+ depends on LEDS_CLASS
+ depends on LEDS_CLASS_MULTICOLOR
+ help
+ Say Y here if you would like to enable support for OneXPlayer handheld
+ devices that come with RGB LED rings around the joysticks and macro buttons.
+
+ To compile this driver as a module, choose M here: the module will
+ be called hid-oxp.
+
config HID_PANTHERLORD
tristate "Pantherlord/GreenAsia game controller"
help
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 03ef72ec4499..bda8a24c9257 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_HID_NTI) += hid-nti.o
obj-$(CONFIG_HID_NTRIG) += hid-ntrig.o
obj-$(CONFIG_HID_NVIDIA_SHIELD) += hid-nvidia-shield.o
obj-$(CONFIG_HID_ORTEK) += hid-ortek.o
+obj-$(CONFIG_HID_OXP) += hid-oxp.o
obj-$(CONFIG_HID_PRODIKEYS) += hid-prodikeys.o
obj-$(CONFIG_HID_PANTHERLORD) += hid-pl.o
obj-$(CONFIG_HID_PENMOUNT) += hid-penmount.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5bad81222c6e..dcc5a3a70eaf 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1131,6 +1131,9 @@
#define USB_VENDOR_ID_NVIDIA 0x0955
#define USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER 0x7214
+#define USB_VENDOR_ID_CRSC 0x1a2c
+#define USB_DEVICE_ID_ONEXPLAYER_GEN1 0xb001
+
#define USB_VENDOR_ID_ONTRAK 0x0a07
#define USB_DEVICE_ID_ONTRAK_ADU100 0x0064
diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
new file mode 100644
index 000000000000..c4219ecd8d71
--- /dev/null
+++ b/drivers/hid/hid-oxp.c
@@ -0,0 +1,651 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * HID driver for OneXPlayer gamepad configuration devices.
+ *
+ * Copyright (c) 2026 Valve Corporation
+ */
+
+#include <linux/array_size.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/jiffies.h>
+#include <linux/kstrtox.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "hid-ids.h"
+
+#define OXP_PACKET_SIZE 64
+
+#define GEN1_MESSAGE_ID 0xff
+
+#define GEN1_USAGE_PAGE 0xff01
+
+enum oxp_function_index {
+ OXP_FID_GEN1_RGB_SET = 0x07,
+ OXP_FID_GEN1_RGB_REPLY = 0x0f,
+};
+
+static struct oxp_hid_cfg {
+ struct led_classdev_mc *led_mc;
+ struct hid_device *hdev;
+ struct mutex cfg_mutex; /*ensure single synchronous output report*/
+ u8 rgb_brightness;
+ u8 rgb_effect;
+ u8 rgb_speed;
+ u8 rgb_en;
+} drvdata;
+
+enum oxp_feature_en_index {
+ OXP_FEAT_DISABLED,
+ OXP_FEAT_ENABLED,
+};
+
+static const char *const oxp_feature_en_text[] = {
+ [OXP_FEAT_DISABLED] = "false",
+ [OXP_FEAT_ENABLED] = "true",
+};
+
+enum oxp_rgb_effect_index {
+ OXP_UNKNOWN,
+ OXP_EFFECT_AURORA,
+ OXP_EFFECT_BIRTHDAY,
+ OXP_EFFECT_FLOWING,
+ OXP_EFFECT_CHROMA_1,
+ OXP_EFFECT_NEON,
+ OXP_EFFECT_CHROMA_2,
+ OXP_EFFECT_DREAMY,
+ OXP_EFFECT_WARM,
+ OXP_EFFECT_CYBERPUNK,
+ OXP_EFFECT_SEA,
+ OXP_EFFECT_SUNSET,
+ OXP_EFFECT_COLORFUL,
+ OXP_EFFECT_MONSTER,
+ OXP_EFFECT_GREEN,
+ OXP_EFFECT_BLUE,
+ OXP_EFFECT_YELLOW,
+ OXP_EFFECT_TEAL,
+ OXP_EFFECT_PURPLE,
+ OXP_EFFECT_FOGGY,
+ OXP_EFFECT_MONO_LIST, /* placeholder for effect_index_show */
+};
+
+/* These belong to rgb_effect_index, but we want to hide them from
+ * rgb_effect_text
+ */
+
+#define OXP_GET_PROPERTY 0xfc
+#define OXP_SET_PROPERTY 0xfd
+#define OXP_EFFECT_MONO_TRUE 0xfe /* actual index for monocolor */
+
+static const char *const oxp_rgb_effect_text[] = {
+ [OXP_UNKNOWN] = "unknown",
+ [OXP_EFFECT_AURORA] = "aurora",
+ [OXP_EFFECT_BIRTHDAY] = "birthday_cake",
+ [OXP_EFFECT_FLOWING] = "flowing_light",
+ [OXP_EFFECT_CHROMA_1] = "chroma_popping",
+ [OXP_EFFECT_NEON] = "neon",
+ [OXP_EFFECT_CHROMA_2] = "chroma_breathing",
+ [OXP_EFFECT_DREAMY] = "dreamy",
+ [OXP_EFFECT_WARM] = "warm_sun",
+ [OXP_EFFECT_CYBERPUNK] = "cyberpunk",
+ [OXP_EFFECT_SEA] = "sea_foam",
+ [OXP_EFFECT_SUNSET] = "sunset_afterglow",
+ [OXP_EFFECT_COLORFUL] = "colorful",
+ [OXP_EFFECT_MONSTER] = "monster_woke",
+ [OXP_EFFECT_GREEN] = "green_breathing",
+ [OXP_EFFECT_BLUE] = "blue_breathing",
+ [OXP_EFFECT_YELLOW] = "yellow_breathing",
+ [OXP_EFFECT_TEAL] = "teal_breathing",
+ [OXP_EFFECT_PURPLE] = "purple_breathing",
+ [OXP_EFFECT_FOGGY] = "foggy_haze",
+ [OXP_EFFECT_MONO_LIST] = "monocolor",
+};
+
+struct oxp_gen_1_rgb_report {
+ u8 report_id;
+ u8 message_id;
+ u8 padding_2[2];
+ u8 effect;
+ u8 enabled;
+ u8 speed;
+ u8 brightness;
+ u8 red;
+ u8 green;
+ u8 blue;
+} __packed;
+
+static u16 get_usage_page(struct hid_device *hdev)
+{
+ return hdev->collection[0].usage >> 16;
+}
+
+static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct led_classdev_mc *led_mc = drvdata.led_mc;
+ struct oxp_gen_1_rgb_report *rgb_rep;
+
+ if (data[1] != OXP_FID_GEN1_RGB_REPLY)
+ return 0;
+
+ rgb_rep = (struct oxp_gen_1_rgb_report *)data;
+ /* Ensure we save monocolor as the list value */
+ drvdata.rgb_effect = rgb_rep->effect == OXP_EFFECT_MONO_TRUE ?
+ OXP_EFFECT_MONO_LIST :
+ rgb_rep->effect;
+ drvdata.rgb_speed = rgb_rep->speed;
+ drvdata.rgb_en = rgb_rep->enabled == 0 ? OXP_FEAT_DISABLED :
+ OXP_FEAT_ENABLED;
+ drvdata.rgb_brightness = rgb_rep->brightness;
+ led_mc->led_cdev.brightness = rgb_rep->brightness / 4 *
+ led_mc->led_cdev.max_brightness;
+ /* If monocolor had less than 100% brightness on the previous boot,
+ * there will be no reliable way to determine the real intensity.
+ * Since intensity scaling is used with a hardware brightness set at max,
+ * our brightness will always look like 100%. Use the last set value to
+ * prevent successive boots from lowering the brightness further.
+ * Brightness will be "wrong" but the effect will remain the same visually.
+ */
+ led_mc->subled_info[0].intensity = rgb_rep->red;
+ led_mc->subled_info[1].intensity = rgb_rep->green;
+ led_mc->subled_info[2].intensity = rgb_rep->blue;
+
+ return 0;
+}
+
+static int oxp_hid_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ u16 up = get_usage_page(hdev);
+
+ dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", OXP_PACKET_SIZE, data);
+
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ return oxp_hid_raw_event_gen_1(hdev, report, data, size);
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static int mcu_property_out(u8 *header, size_t header_size, u8 *data,
+ size_t data_size, u8 *footer, size_t footer_size)
+{
+ unsigned char *dmabuf __free(kfree) = kzalloc(OXP_PACKET_SIZE, GFP_KERNEL);
+ int ret;
+
+ if (!dmabuf)
+ return -ENOMEM;
+
+ if (header_size + data_size + footer_size > OXP_PACKET_SIZE)
+ return -EINVAL;
+
+ guard(mutex)(&drvdata.cfg_mutex);
+ memcpy(dmabuf, header, header_size);
+ memcpy(dmabuf + header_size, data, data_size);
+ if (footer_size)
+ memcpy(dmabuf + OXP_PACKET_SIZE - footer_size, footer, footer_size);
+
+ dev_dbg(&drvdata.hdev->dev, "raw data: [%*ph]\n", OXP_PACKET_SIZE, dmabuf);
+
+ ret = hid_hw_output_report(drvdata.hdev, dmabuf, OXP_PACKET_SIZE);
+ if (ret < 0)
+ return ret;
+
+ /* MCU takes 200ms to be ready for another command. */
+ msleep(200);
+ return ret == OXP_PACKET_SIZE ? 0 : -EIO;
+}
+
+static int oxp_gen_1_property_out(enum oxp_function_index fid, u8 *data,
+ u8 data_size)
+{
+ u8 header[] = { fid, GEN1_MESSAGE_ID };
+ size_t header_size = ARRAY_SIZE(header);
+
+ return mcu_property_out(header, header_size, data, data_size, NULL, 0);
+}
+
+static int oxp_rgb_status_store(u8 enabled, u8 speed, u8 brightness)
+{
+ u16 up = get_usage_page(drvdata.hdev);
+ u8 *data;
+
+ /* Always default to max brightness and use intensity scaling when in
+ * monocolor mode.
+ */
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ data = (u8[4]) { OXP_SET_PROPERTY, enabled, speed, brightness };
+ if (drvdata.rgb_effect == OXP_EFFECT_MONO_LIST)
+ data[3] = 0x04;
+ return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 4);
+ default:
+ return -ENODEV;
+ }
+}
+
+static ssize_t oxp_rgb_status_show(void)
+{
+ u16 up = get_usage_page(drvdata.hdev);
+ u8 *data;
+
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ data = (u8[1]) { OXP_GET_PROPERTY };
+ return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
+ default:
+ return -ENODEV;
+ }
+}
+
+static int oxp_rgb_color_set(void)
+{
+ u8 max_br = drvdata.led_mc->led_cdev.max_brightness;
+ u8 br = drvdata.led_mc->led_cdev.brightness;
+ u16 up = get_usage_page(drvdata.hdev);
+ u8 green, red, blue;
+ size_t size;
+ u8 *data;
+ int i;
+
+ red = br * drvdata.led_mc->subled_info[0].intensity / max_br;
+ green = br * drvdata.led_mc->subled_info[1].intensity / max_br;
+ blue = br * drvdata.led_mc->subled_info[2].intensity / max_br;
+
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ size = 55;
+ data = (u8[55]) { OXP_EFFECT_MONO_TRUE };
+
+ for (i = 0; i < (size - 1) / 3; i++) {
+ data[3 * i + 1] = red;
+ data[3 * i + 2] = green;
+ data[3 * i + 3] = blue;
+ }
+ return oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, size);
+ default:
+ return -ENODEV;
+ }
+}
+
+static int oxp_rgb_effect_set(u8 effect)
+{
+ u16 up = get_usage_page(drvdata.hdev);
+ u8 *data;
+ int ret;
+
+ switch (effect) {
+ case OXP_EFFECT_AURORA:
+ case OXP_EFFECT_BIRTHDAY:
+ case OXP_EFFECT_FLOWING:
+ case OXP_EFFECT_CHROMA_1:
+ case OXP_EFFECT_NEON:
+ case OXP_EFFECT_CHROMA_2:
+ case OXP_EFFECT_DREAMY:
+ case OXP_EFFECT_WARM:
+ case OXP_EFFECT_CYBERPUNK:
+ case OXP_EFFECT_SEA:
+ case OXP_EFFECT_SUNSET:
+ case OXP_EFFECT_COLORFUL:
+ case OXP_EFFECT_MONSTER:
+ case OXP_EFFECT_GREEN:
+ case OXP_EFFECT_BLUE:
+ case OXP_EFFECT_YELLOW:
+ case OXP_EFFECT_TEAL:
+ case OXP_EFFECT_PURPLE:
+ case OXP_EFFECT_FOGGY:
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ data = (u8[1]) { effect };
+ ret = oxp_gen_1_property_out(OXP_FID_GEN1_RGB_SET, data, 1);
+ break;
+ default:
+ ret = -ENODEV;
+ }
+ break;
+ case OXP_EFFECT_MONO_LIST:
+ ret = oxp_rgb_color_set();
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (ret)
+ return ret;
+
+ drvdata.rgb_effect = effect;
+
+ return 0;
+}
+
+static ssize_t enabled_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ u8 val;
+
+ ret = sysfs_match_string(oxp_feature_en_text, buf);
+ if (ret < 0)
+ return ret;
+ val = ret;
+
+ ret = oxp_rgb_status_store(val, drvdata.rgb_speed,
+ drvdata.rgb_brightness);
+ if (ret)
+ return ret;
+
+ drvdata.rgb_en = val;
+ return count;
+}
+
+static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+
+ ret = oxp_rgb_status_show();
+ if (ret)
+ return ret;
+
+ if (drvdata.rgb_en >= ARRAY_SIZE(oxp_feature_en_text))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", oxp_feature_en_text[drvdata.rgb_en]);
+}
+static DEVICE_ATTR_RW(enabled);
+
+static ssize_t enabled_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ size_t count = 0;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(oxp_feature_en_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ", oxp_feature_en_text[i]);
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(enabled_index);
+
+static ssize_t effect_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ u8 val;
+
+ ret = sysfs_match_string(oxp_rgb_effect_text, buf);
+ if (ret < 0)
+ return ret;
+
+ val = ret;
+
+ ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed,
+ drvdata.rgb_brightness);
+ if (ret)
+ return ret;
+
+ ret = oxp_rgb_effect_set(val);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t effect_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+
+ ret = oxp_rgb_status_show();
+ if (ret)
+ return ret;
+
+ if (drvdata.rgb_effect >= ARRAY_SIZE(oxp_rgb_effect_text))
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", oxp_rgb_effect_text[drvdata.rgb_effect]);
+}
+
+static DEVICE_ATTR_RW(effect);
+
+static ssize_t effect_index_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ size_t count = 0;
+ unsigned int i;
+
+ for (i = 1; i < ARRAY_SIZE(oxp_rgb_effect_text); i++)
+ count += sysfs_emit_at(buf, count, "%s ", oxp_rgb_effect_text[i]);
+
+ if (count)
+ buf[count - 1] = '\n';
+
+ return count;
+}
+static DEVICE_ATTR_RO(effect_index);
+
+static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ u8 val;
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ if (val > 9)
+ return -EINVAL;
+
+ ret = oxp_rgb_status_store(drvdata.rgb_en, val, drvdata.rgb_brightness);
+ if (ret)
+ return ret;
+
+ drvdata.rgb_speed = val;
+ return count;
+}
+
+static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+
+ ret = oxp_rgb_status_show();
+ if (ret)
+ return ret;
+
+ if (drvdata.rgb_speed > 9)
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%hhu\n", drvdata.rgb_speed);
+}
+static DEVICE_ATTR_RW(speed);
+
+static ssize_t speed_range_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "0-9\n");
+}
+static DEVICE_ATTR_RO(speed_range);
+
+static void oxp_rgb_queue_fn(struct work_struct *work)
+{
+ unsigned int max_brightness = drvdata.led_mc->led_cdev.max_brightness;
+ unsigned int brightness = drvdata.led_mc->led_cdev.brightness;
+ u8 val = 4 * brightness / max_brightness;
+ int ret;
+
+ if (drvdata.rgb_brightness != val) {
+ ret = oxp_rgb_status_store(drvdata.rgb_en, drvdata.rgb_speed, val);
+ if (ret)
+ dev_err(drvdata.led_mc->led_cdev.dev,
+ "Error: Failed to write RGB Status: %i\n", ret);
+
+ drvdata.rgb_brightness = val;
+ }
+
+ if (drvdata.rgb_effect != OXP_EFFECT_MONO_LIST)
+ return;
+
+ ret = oxp_rgb_effect_set(drvdata.rgb_effect);
+ if (ret)
+ dev_err(drvdata.led_mc->led_cdev.dev, "Error: Failed to write RGB color: %i\n",
+ ret);
+}
+
+static DECLARE_DELAYED_WORK(oxp_rgb_queue, oxp_rgb_queue_fn);
+
+static void oxp_rgb_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ led_cdev->brightness = brightness;
+ mod_delayed_work(system_wq, &oxp_rgb_queue, msecs_to_jiffies(50));
+}
+
+static struct attribute *oxp_rgb_attrs[] = {
+ &dev_attr_effect.attr,
+ &dev_attr_effect_index.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_enabled_index.attr,
+ &dev_attr_speed.attr,
+ &dev_attr_speed_range.attr,
+ NULL,
+};
+
+static const struct attribute_group oxp_rgb_attr_group = {
+ .attrs = oxp_rgb_attrs,
+};
+
+static struct mc_subled oxp_rgb_subled_info[] = {
+ {
+ .color_index = LED_COLOR_ID_RED,
+ .intensity = 0x24,
+ .channel = 0x1,
+ },
+ {
+ .color_index = LED_COLOR_ID_GREEN,
+ .intensity = 0x22,
+ .channel = 0x2,
+ },
+ {
+ .color_index = LED_COLOR_ID_BLUE,
+ .intensity = 0x99,
+ .channel = 0x3,
+ },
+};
+
+static struct led_classdev_mc oxp_cdev_rgb = {
+ .led_cdev = {
+ .name = "oxp:rgb:joystick_rings",
+ .color = LED_COLOR_ID_RGB,
+ .brightness = 0x64,
+ .max_brightness = 0x64,
+ .brightness_set = oxp_rgb_brightness_set,
+ },
+ .num_colors = ARRAY_SIZE(oxp_rgb_subled_info),
+ .subled_info = oxp_rgb_subled_info,
+};
+
+static int oxp_cfg_probe(struct hid_device *hdev, u16 up)
+{
+ int ret;
+
+ hid_set_drvdata(hdev, &drvdata);
+ mutex_init(&drvdata.cfg_mutex);
+ drvdata.hdev = hdev;
+ drvdata.led_mc = &oxp_cdev_rgb;
+
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, &oxp_cdev_rgb);
+ if (ret)
+ return dev_err_probe(&hdev->dev, ret,
+ "Failed to create RGB device\n");
+
+ ret = devm_device_add_group(drvdata.led_mc->led_cdev.dev,
+ &oxp_rgb_attr_group);
+ if (ret)
+ return dev_err_probe(drvdata.led_mc->led_cdev.dev, ret,
+ "Failed to create RGB configuration attributes\n");
+
+ ret = oxp_rgb_status_show();
+ if (ret)
+ dev_warn(drvdata.led_mc->led_cdev.dev,
+ "Failed to query RGB initial state: %i\n", ret);
+
+ return 0;
+}
+
+static int oxp_hid_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ int ret;
+ u16 up;
+
+ ret = hid_parse(hdev);
+ if (ret)
+ return dev_err_probe(&hdev->dev, ret, "Failed to parse HID device\n");
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret)
+ return dev_err_probe(&hdev->dev, ret, "Failed to start HID device\n");
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_hw_stop(hdev);
+ return dev_err_probe(&hdev->dev, ret, "Failed to open HID device\n");
+ }
+
+ up = get_usage_page(hdev);
+ dev_dbg(&hdev->dev, "Got usage page %04x\n", up);
+
+ switch (up) {
+ case GEN1_USAGE_PAGE:
+ ret = oxp_cfg_probe(hdev, up);
+ if (ret) {
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+ }
+
+ return ret;
+ default:
+ return 0;
+ }
+}
+
+static void oxp_hid_remove(struct hid_device *hdev)
+{
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id oxp_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_CRSC, USB_DEVICE_ID_ONEXPLAYER_GEN1) },
+ {}
+};
+
+MODULE_DEVICE_TABLE(hid, oxp_devices);
+static struct hid_driver hid_oxp = {
+ .name = "hid-oxp",
+ .id_table = oxp_devices,
+ .probe = oxp_hid_probe,
+ .remove = oxp_hid_remove,
+ .raw_event = oxp_hid_raw_event,
+};
+module_hid_driver(hid_oxp);
+
+MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
+MODULE_DESCRIPTION("Driver for OneXPlayer HID Interfaces");
+MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related
* [PATCH v2 0/5] Add OneXPlayer Configuration HID Driver
From: Derek J. Clark @ 2026-04-07 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Pierre-Loup A . Griffais, Lambert Fan, Derek J . Clark,
linux-input, linux-doc, linux-kernel
Adds an HID driver for OneXPlayer HID configuration devices. There are
currently 2 generations of OneXPlayer HID protocol. The first (OneXPlayer
F1 series) only provides an RGB control interface over HID. The Second
(X1 mini series, G1 series, AOKZOE A1X) also includes a hardware level
button mapping interface, vibration intensity settings, and the ability
to switch output between xinput and a debug mode that can be used to debug
the button mapping. Some devices (G1 Series, APEX) use a hybrid of Gen1
RGB control and Gen 2 controller settings. To ensure there is no conflicts
when the driver is loaded, we skip creating the RGB interface for Gen 2
devices if there is a DMI match.
I'll also add a note that Gen 1 devices also have an interface for
setting the key map and debug mode, but that is done entirely over a
serial TTY device so it is not able to be added to this driver. There
are also some "Gen 0" devices (OneXPlayer 2 Series) also use it, but
the TTY interface also handles the RGB control so no support is
provided by this driver for those devices.
Signed-off-by: Derel J. Clark <derekjohn.clark@gmail.com>
---
v2:
- Add DMI quirks for certain devices that ship with both GEN1 and GEN2
MCU to avoid clashing when initializing the RGB interface.
- Add left & right vibration intensity attributes.
- Add additional mappings for keyboard inputs.
- Add a delayed work trigger to re-apply settings after the MCU
completes initializing after a suspend/resume cycle.
v1: https://lore.kernel.org/linux-input/20260322031615.1524307-1-derekjohn.clark@gmail.com/
Derek J. Clark (5):
HID: hid-oxp: Add OneXPlayer configuration driver
HID: hid-oxp: Add Second Generation RGB Control
HID: hid-oxp: Add Second Generation Gamepad Mode Switch
HID: hid-oxp: Add Button Mapping Interface
HID: hid-oxp: Add Virbation Intenstity Attributes
MAINTAINERS | 6 +
drivers/hid/Kconfig | 13 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 6 +
drivers/hid/hid-oxp.c | 1595 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 1621 insertions(+)
create mode 100644 drivers/hid/hid-oxp.c
--
2.53.0
^ permalink raw reply
* Re: [PATCH v7 9/9] KVM: selftests: nSVM: Add svm_nested_pat test
From: Jim Mattson @ 2026-04-07 3:58 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <adRK0dyF9QAsZyVz@google.com>
On Mon, Apr 6, 2026 at 5:07 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Mar 27, 2026, Jim Mattson wrote:
> > When KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT is disabled, verify that KVM
> > correctly virtualizes the host PAT MSR and the guest PAT register for
> > nested SVM guests.
> >
> > With nested NPT disabled:
> > * L1 and L2 share the same PAT
> > * The vmcb12.g_pat is ignored
> >
> > With nested NPT enabled:
> > * An invalid g_pat in vmcb12 causes VMEXIT_INVALID
> > * RDMSR(IA32_PAT) from L2 returns the value of the guest PAT register
> > * WRMSR(IA32_PAT) from L2 is reflected in vmcb12's g_pat on VMEXIT
> > * RDMSR(IA32_PAT) from L1 returns the value of the host PAT MSR
> > * Save/restore with the vCPU in guest mode preserves both hPAT and gPAT
> >
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> > tools/arch/x86/include/uapi/asm/kvm.h | 2 +
>
> Don't update uAPI headers in tools/, they're not used by KVM selftests (perf
> folks will sync them as needed).
>
> > +#define PAT_DEFAULT 0x0007040600070406ULL
> > +#define L1_PAT_VALUE 0x0007040600070404ULL /* Change PA0 to WT */
> > +#define L2_VMCB12_PAT 0x0606060606060606ULL /* All WB */
> > +#define L2_PAT_MODIFIED 0x0606060606060604ULL /* Change PA0 to WT */
> > +#define INVALID_PAT_VALUE 0x0808080808080808ULL /* 8 is reserved */
> > +
> > +/*
> > + * Shared state between L1 and L2 for verification.
> > + */
> > +struct pat_test_data {
> > + uint64_t l2_pat_read;
> > + uint64_t l2_pat_after_write;
> > + uint64_t l1_pat_after_vmexit;
> > + uint64_t vmcb12_gpat_after_exit;
> > + bool l2_done;
> > +};
> > +
> > +static struct pat_test_data *pat_data;
>
> This is ridiculous. Whatever AI you're using is reinventing sync_global_to_guest()
> in a very obfuscated way. Drop the indirection along with the params and the
> full page allocation, and just sync the damn struct.
>
> Actually, this is even dumber than that. The "data" is only ever accessed from
> within the guest; it's used to pass info between L1 and L2. Drop the struct
> entirely and just write global variables.
>
> In general, please clean this test up before submitting v8. All of the L2 code
> is basically copy+paste of itself. This is the second vibe coded selftest (AFAIK)
> you've posted, and it has many of the same flaws as the first one[*]. While I'm
> not opposed to using fancy tools, and the bar is generally lower for selftests,
> the code still needs to be readable and maintainable. This ain't.
>
> [*] https://lore.kernel.org/all/aXJal3srw2-3J5Dm@google.com
I contend that the kvm selftests infrastructure already fails to meet
that bar, but I will toss this in the bit bucket and write a selftest
by hand for the next version.
^ permalink raw reply
* Re: [PATCH v7 8/9] KVM: x86: nSVM: Save/restore gPAT with KVM_{GET,SET}_NESTED_STATE
From: Jim Mattson @ 2026-04-07 3:08 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <adRGLPrmBpX-3DdX@google.com>
On Mon, Apr 6, 2026 at 4:47 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Mar 27, 2026, Jim Mattson wrote:
> > @@ -1918,6 +1921,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > struct vmcb_save_area_cached save_cached;
> > struct vmcb_ctrl_area_cached ctl_cached;
> > unsigned long cr0;
> > + bool use_separate_l2_pat;
>
> Land this above "cr0" to preserve the inverted fir tree.
>
> > int ret;
> >
> > BUILD_BUG_ON(sizeof(struct vmcb_control_area) + sizeof(struct vmcb_save_area) >
> > @@ -1993,6 +1997,18 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > !nested_vmcb_check_save(vcpu, &save_cached, false))
> > goto out_free;
> >
> > + /*
> > + * Validate gPAT when the shared PAT quirk is disabled (i.e. L2
> > + * has its own gPAT). This is done separately from the
> > + * vmcb_save_area_cached validation above, because gPAT is L2
> > + * state, but the vmcb_save_area_cached is populated with L1 state.
> > + */
> > + use_separate_l2_pat =
> > + (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
> > + !kvm_check_has_quirk(vcpu->kvm,
> > + KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
>
> I vote for either:
>
> use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
> !kvm_check_has_quirk(vcpu->kvm,
> KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
>
LOL! Aren't you the one who keeps complaining that my indentation
doesn't line up? Are you schizophrenic?
> or
>
> use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP);
> if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT))
> use_separate_l2_pat = false;
Wow. I really have no idea how to predict what you're going to want
the code to look like. How is this better than the original?!?
>
> > + if (use_separate_l2_pat && !kvm_pat_valid(kvm_state->hdr.svm.gpat))
> > + goto out_free;
> >
> > /*
> > * All checks done, we can enter guest mode. Userspace provides
> > @@ -2017,6 +2033,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > nested_copy_vmcb_control_to_cache(svm, ctl);
> >
> > svm_switch_vmcb(svm, &svm->nested.vmcb02);
> > +
> > + if (use_separate_l2_pat)
> > + vmcb_set_gpat(svm->vmcb, kvm_state->hdr.svm.gpat);
> > +
> > nested_vmcb02_prepare_control(svm);
> >
> > /*
> > --
> > 2.53.0.1018.g2bb0e51243-goog
> >
^ permalink raw reply
* Re: [PATCH v2 2/3] mm/memory-failure: add panic_on_unrecoverable_memory_failure sysctl
From: Miaohe Lin @ 2026-04-07 2:57 UTC (permalink / raw)
To: Breno Leitao
Cc: linux-mm, linux-kernel, linux-doc, kernel-team, Naoya Horiguchi,
Andrew Morton, Jonathan Corbet, Shuah Khan
In-Reply-To: <20260331-ecc_panic-v2-2-9e40d0f64f7a@debian.org>
On 2026/3/31 19:00, Breno Leitao wrote:
> Add a sysctl that allows the system to panic when an unrecoverable
> memory failure is detected. This covers kernel pages, high-order
> kernel pages, and unknown page types that cannot be recovered.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> mm/memory-failure.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 6ff80e01b91a..d0d911c54ff1 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -74,6 +74,8 @@ static int sysctl_memory_failure_recovery __read_mostly = 1;
>
> static int sysctl_enable_soft_offline __read_mostly = 1;
>
> +static int sysctl_panic_on_unrecoverable_mf __read_mostly;
> +
> atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0);
>
> static bool hw_memory_failure __read_mostly = false;
> @@ -155,6 +157,15 @@ static const struct ctl_table memory_failure_table[] = {
> .proc_handler = proc_dointvec_minmax,
> .extra1 = SYSCTL_ZERO,
> .extra2 = SYSCTL_ONE,
> + },
> + {
> + .procname = "panic_on_unrecoverable_memory_failure",
> + .data = &sysctl_panic_on_unrecoverable_mf,
> + .maxlen = sizeof(sysctl_panic_on_unrecoverable_mf),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE,
> }
> };
>
> @@ -1298,6 +1309,11 @@ static int action_result(unsigned long pfn, enum mf_action_page_type type,
> pr_err("%#lx: recovery action for %s: %s\n",
> pfn, action_page_types[type], action_name[result]);
>
> + if (sysctl_panic_on_unrecoverable_mf && result == MF_IGNORED &&
> + (type == MF_MSG_KERNEL || type == MF_MSG_KERNEL_HIGH_ORDER ||
> + type == MF_MSG_UNKNOWN))
> + panic("Memory failure: %#lx: unrecoverable page", pfn);
Will it be better to add a helper here?
Thanks.
.
^ permalink raw reply
* Re: [PATCH v2 1/3] mm/memory-failure: report MF_MSG_KERNEL for reserved pages
From: Miaohe Lin @ 2026-04-07 2:56 UTC (permalink / raw)
To: Breno Leitao
Cc: linux-mm, linux-kernel, linux-doc, kernel-team, Naoya Horiguchi,
Andrew Morton, Jonathan Corbet, Shuah Khan
In-Reply-To: <20260331-ecc_panic-v2-1-9e40d0f64f7a@debian.org>
On 2026/3/31 19:00, Breno Leitao wrote:
> When get_hwpoison_page() returns a negative value, distinguish
> reserved pages from other failure cases by reporting MF_MSG_KERNEL
> instead of MF_MSG_GET_HWPOISON. Reserved pages belong to the kernel
> and should be classified accordingly for proper handling by the
> panic_on_unrecoverable_memory_failure mechanism.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> mm/memory-failure.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ee42d4361309..6ff80e01b91a 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2432,7 +2432,11 @@ int memory_failure(unsigned long pfn, int flags)
> }
> goto unlock_mutex;
> } else if (res < 0) {
> - res = action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
> + if (PageReserved(p))
> + res = action_result(pfn, MF_MSG_KERNEL, MF_IGNORED);
Is it safe or common to check page flags without holding extra refcnt?
Thanks.
.
^ permalink raw reply
* Re: [PATCH v2 00/33] rust: bump minimum Rust and `bindgen` versions
From: Miguel Ojeda @ 2026-04-07 1:15 UTC (permalink / raw)
To: John Hubbard
Cc: Miguel Ojeda, Nathan Chancellor, Nicolas Schier, Danilo Krummrich,
Andreas Hindborg, Catalin Marinas, Will Deacon, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Courbot, Simona Vetter,
Brendan Higgins, David Gow, Greg Kroah-Hartman,
Arve Hjønnevåg, Todd Kjos, Christian Brauner,
Carlos Llamas, Alice Ryhl, Jonathan Corbet, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Trevor Gross, rust-for-linux,
linux-kbuild, Lorenzo Stoakes, Vlastimil Babka, Liam R . Howlett,
Uladzislau Rezki, linux-block, moderated for non-subscribers,
Alexandre Ghiti, linux-riscv, nouveau, dri-devel, Rae Moar,
linux-kselftest, kunit-dev, Nick Desaulniers, Bill Wendling,
Justin Stitt, llvm, linux-kernel, Shuah Khan, linux-doc
In-Reply-To: <efe61810-2b28-4acd-b69f-d577042c0b62@nvidia.com>
On Mon, Apr 6, 2026 at 9:07 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> That's what I thought I recalled, too. Weird that it is not in rust-next
> already, though.
It is normal -- in the kernel back merges are generally to be avoided.
Cheers,
Miguel
^ permalink raw reply
* [RFC PATCH v3 05/10] Docs/admin-guide/mm/damon/usage: document fail_charge_{num,denom} files
From: SeongJae Park @ 2026-04-07 1:05 UTC (permalink / raw)
Cc: SeongJae Park, Liam R. Howlett, Andrew Morton, David Hildenbrand,
Jonathan Corbet, Lorenzo Stoakes, Michal Hocko, Mike Rapoport,
Shuah Khan, Suren Baghdasaryan, Vlastimil Babka, damon, linux-doc,
linux-kernel, linux-mm
In-Reply-To: <20260407010536.83603-1-sj@kernel.org>
Update DAMON usage document for the DAMOS action failed regions quota
charge ratio control sysfs files.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
Documentation/admin-guide/mm/damon/usage.rst | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/mm/damon/usage.rst b/Documentation/admin-guide/mm/damon/usage.rst
index bfdb717441f05..d5548e460857c 100644
--- a/Documentation/admin-guide/mm/damon/usage.rst
+++ b/Documentation/admin-guide/mm/damon/usage.rst
@@ -84,7 +84,9 @@ comma (",").
│ │ │ │ │ │ │ │ sz/min,max
│ │ │ │ │ │ │ │ nr_accesses/min,max
│ │ │ │ │ │ │ │ age/min,max
- │ │ │ │ │ │ │ :ref:`quotas <sysfs_quotas>`/ms,bytes,reset_interval_ms,effective_bytes,goal_tuner
+ │ │ │ │ │ │ │ :ref:`quotas <sysfs_quotas>`/ms,bytes,reset_interval_ms,
+ │ │ │ │ │ │ │ effective_bytes,goal_tuner,
+ │ │ │ │ │ │ │ fail_charge_num,fail_charge_denom
│ │ │ │ │ │ │ │ weights/sz_permil,nr_accesses_permil,age_permil
│ │ │ │ │ │ │ │ :ref:`goals <sysfs_schemes_quota_goals>`/nr_goals
│ │ │ │ │ │ │ │ │ 0/target_metric,target_value,current_value,nid,path
@@ -381,9 +383,10 @@ schemes/<N>/quotas/
The directory for the :ref:`quotas <damon_design_damos_quotas>` of the given
DAMON-based operation scheme.
-Under ``quotas`` directory, five files (``ms``, ``bytes``,
-``reset_interval_ms``, ``effective_bytes`` and ``goal_tuner``) and two
-directories (``weights`` and ``goals``) exist.
+Under ``quotas`` directory, seven files (``ms``, ``bytes``,
+``reset_interval_ms``, ``effective_bytes``, ``goal_tuner``, ``fail_charge_num``
+and ``fail_charge_denom``) and two directories (``weights`` and ``goals``)
+exist.
You can set the ``time quota`` in milliseconds, ``size quota`` in bytes, and
``reset interval`` in milliseconds by writing the values to the three files,
@@ -402,6 +405,13 @@ the background design of the feature and the name of the selectable algorithms.
Refer to :ref:`goals directory <sysfs_schemes_quota_goals>` for the goals
setup.
+You can set the action-failed memory quota charging ratio by writing the
+numerator and the denominator for the ratio to ``fail_charge_num`` and
+``fail_charge_denom`` files, respectively. Reading those files will return the
+current set values. Refer to :ref:`design
+<damon_design_damos_quotas_failed_memory_charging_ratio>` for more details of
+the ratio feature.
+
The time quota is internally transformed to a size quota. Between the
transformed size quota and user-specified size quota, smaller one is applied.
Based on the user-specified :ref:`goal <sysfs_schemes_quota_goals>`, the
--
2.47.3
^ permalink raw reply related
* [RFC PATCH v3 04/10] Docs/mm/damon/design: document fail_charge_{num,denom}
From: SeongJae Park @ 2026-04-07 1:05 UTC (permalink / raw)
Cc: SeongJae Park, Liam R. Howlett, Andrew Morton, David Hildenbrand,
Jonathan Corbet, Lorenzo Stoakes, Michal Hocko, Mike Rapoport,
Shuah Khan, Suren Baghdasaryan, Vlastimil Babka, damon, linux-doc,
linux-kernel, linux-mm
In-Reply-To: <20260407010536.83603-1-sj@kernel.org>
Update DAMON design document for the DAMOS action failed region quota
charge ratio.
Signed-off-by: SeongJae Park <sj@kernel.org>
---
Documentation/mm/damon/design.rst | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/mm/damon/design.rst b/Documentation/mm/damon/design.rst
index 510ec6375178d..94e898b671d15 100644
--- a/Documentation/mm/damon/design.rst
+++ b/Documentation/mm/damon/design.rst
@@ -572,6 +572,28 @@ interface <sysfs_interface>`, refer to :ref:`weights <sysfs_quotas>` part of
the documentation.
+.. _damon_design_damos_quotas_failed_memory_charging_ratio:
+
+Action-failed Memory Charging Ratio
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+DAMOS action to a given region can fail for some subsets of the memory of the
+region. For example, if the action is ``pageout`` and the region has some
+unreclaimable pages, applying the action to the pages will fail. The amount of
+system resource that is taken for such failed action applications is usually
+different from that for successful action applications. For such cases, users
+can set different charging ratio for such failed memory. The ratio can be
+specified using ``fail_charge_num`` and ``fail_charge_denom`` parameters. The
+two parameters represent the numerator and denominator of the ratio. The
+feature is enabled only if ``fail_charge_denom`` is not zero.
+
+For example, let's suppose a DAMOS action is applied to a region of 1,000 MiB
+size. The action is successfully applied to only 700 MiB of the region.
+``fail_charge_num`` and ``fail_charge_denom`` are set to ``1`` and ``1024``,
+respectively. Then only 700 MiB and 300 KiB of size (``700 MiB + 300 MiB * 1 /
+1024``) will be charged.
+
+
.. _damon_design_damos_quotas_auto_tuning:
Aim-oriented Feedback-driven Auto-tuning
--
2.47.3
^ permalink raw reply related
* [RFC PATCH v3 00/10] mm/damon: introduce DAMOS failed region quota charge ratio
From: SeongJae Park @ 2026-04-07 1:05 UTC (permalink / raw)
Cc: SeongJae Park, Liam R. Howlett, Andrew Morton, Brendan Higgins,
David Gow, David Hildenbrand, Jonathan Corbet, Lorenzo Stoakes,
Michal Hocko, Mike Rapoport, Shuah Khan, Shuah Khan,
Suren Baghdasaryan, Vlastimil Babka, damon, kunit-dev, linux-doc,
linux-kernel, linux-kselftest, linux-mm
TL; DR: Let users set different DAMOS quota charge ratios for DAMOS
action failed regions, for deterministic and consistent DAMOS action
progress.
Common Reports: Unexpectedly Slow DAMOS
=======================================
One common issue report that we get from DAMON users is that DAMOS
action applying progress speed is sometimes much slower than expected.
And one common root cause is that the DAMOS quota is exceeded by the
action applying failed memory regions.
For example, a group of users tried to run DAMOS-based proactive memory
reclamation (DAMON_RECLAIM) with 100 MiB per second DAMOS quota. They
ran it on a system having no active workload which means all memory of
the system is cold. The expectation was that the system will show 100
MiB per second reclamation until (nearly) all memory is reclaimed. But
what they found is that the speed is quite inconsistent and sometimes it
becomes very slower than the expectation, sometimes even no reclamation
at all for about tens of seconds. The upper limit of the speed (100 MiB
per second) was being kept as expected, though.
By monitoring the qt_exceeds (number of DAMOS quota exceed events) DAMOS
stat, we found DAMOS quota is always exceeded when the speed is slow. By
monitoring sz_tried and sz_applied (the total amount of DAMOS action
tried memory and succeeded memory) DAMOS stats together, we found the
reclamation attempts nearly always failed when the speed is slow.
DAMOS quota charges DAMOS action tried regions regardless of the
successfulness of the try. Hence in the example reported case, there
was unreclaimable memory spread around the system memory. Sometimes
nearly 100 MiB of memory that DAMOS tried to reclaim in the given quota
interval was reclaimable, and therefore showed nearly 100 MiB per second
speed. Sometimes nearly 99 MiB of memory that DAMOS was trying to
reclaim in the given quota interval was unreclaimable, and therefore
showing only about 1 MiB per second reclaim speed.
We explained it is an expected behavior of the feature rather than a
bug, as DAMOS quota is there for only the upper-limit of the speed. The
users agreed and later reported a huge win from the adoption of
DAMON_RECLAIM on their products.
It is Not a Bug but a Feature; But...
=====================================
So nothing is broken. DAMOS quota is working as intended, as the upper
limit of the speed. It also provides its behavior observability via
DAMOS stat. In the real world production environment that runs long
term active workloads and matters stability, the speed sometimes being
slow is not a real problem.
But, the non-deterministic behavior is sometimes annoying, especially in
lab environments. Even in a realistic production environment, when
there is a huge amount of DAMOS action unapplicable memory, the speed
could be problematically slow. Let's suppose a virtual machines
provider that setup 99% of the host memory as hugetlb pages that cannot
be reclaimed, to give it to virtual machines. Also, when aim-oriented
DAMOS auto-tuning is applied, this could also make the internal feedback
loop confused.
The intention of the current behavior was that trying DAMOS action to
regions would anyway impose some overhead, and therefore somehow be
charged. But in the real world, the overhead for failed action is much
lighter than successful action. Charging those at the same ratio may be
unfair, or at least suboptimum in some environments.
DAMOS Action Failed Region Quota Charge Ratio
=============================================
Let users set the charge ratio for the action-failed memory, for more
optimal and deterministic use of DAMOS. It allows users to specify the
numerator and the denominator of the ratio for flexible setup. For
example, let's suppose the numerator and the denominator are set to 1
and 4,096, respectively. The ratio is 1 / 4,096. A DAMOS scheme action
is applied to 5 GiB memory. For 1 GiB of the memory, the action is
succeeded. For the rest (4 GiB), the action is failed. Then, only 1
GiB and 1 MiB quota is charged.
The optimal charge ratio will depend on the use case and
system/workload. I'd recommend starting from setting the nominator as 1
and the denominator as PAGE_SIZE and tune based on the results, because
many DAMOS actions are applied at page level.
Tests
=====
I tested this feature in the steps below.
1. Allocate 50% of system memory and mlock() it using a test program.
2. Fill up the page cache to exhaust nearly all free memory.
3. Start DAMON-based proactive reclamation with 100 MiB/second DAMOS
hard-quota. Auto-tune the DAMOS soft-quota under the hard-quota for
achieving 40% free memory of the system with 'temporal' tuner.
For step 1, I run a simple C program that is written by Gemini. It is
quite straightforward, so I'm not sharing the code here.
For step 2, I use dd command like below:
dd if=/dev/zero of=foo bs=1M count=$50_percent_of_system_memory
For step 3, I use the latest version of DAMON user-space tool (damo)
like below.
sudo damo start --damos_action pageout \
` # Do the pageout only up to 100 MiB per second ` \
--damos_quota_space 100M --damos_quota_interval 1s \
` # Auto-tune the quota below the hard quota aiming` \
` # 40% free memory of the node 0 ` \
` # (entire node of the test system)` \
--damos_quota_goal node_mem_free_bp 40% 0 \
` # use temporal tuner, which is easy to understnd ` \
--damos_quota_goal_tuner temporal
As expected, the progress of the reclamation is not consistent, because
the quota is exceeded for the failed reclamation of the unreclaimable
memory.
I do this again, but with the failed region charge ratio feature. For
this, the above 'damo' command is used, after appending command line
option for setup of the charge ratio like below. Note that the option
was added to 'damo' after v3.1.9.
sudo ./damo start --damos_action pageout \
[...]
` # quota-charge only 1/4096 for pageout-failed regions ` \
--damos_quota_fail_charge_ratio 1 4096
The progress of the reclamation was nearly 100 MiB per second until the
goal was achieved, meeting the expectation.
Patches Sequence
================
Patch 1 updates fully charged quota check to handle <min_region_sz
remaining quota, which will be able to exist after this series is
applied. Patch 2 implements the feature and exposes it via DAMON core
API. Patch 3 implements DAMON sysfs ABI for the feature. Three
following patches (4-6) document the feature and ABI on design, usage,
and ABI documents, respectively. Four patches for testing of the new
feature follow. Patch 7 implements a kunit test for the feature.
Patches 8 and 9 extend DAMON selftest helpers for DAMON sysfs control
and internal state dumping for adding a new selftest for the feature.
Patch 10 extends existing DAMON sysfs interface selftest to test the new
feature using the extended helper scripts.
Changelog
=========
Changes from RFC v2
(https://lore.kernel.org/20260405151232.102690-1-sj@kernel.org)
- Handle <min_region_sz remaining quota.
- Document zero denum behavior.
- Fix typos: s/selftets/selftests/
Changes from RFC v1
(https://lore.kernel.org/20260404163943.89278-1-sj@kernel.org)
- Avoid overflows in charge amount calculation.
- Fix/wordsmith documentation for grammar, typo, and wrong examples.
- Improve unit test for more consistent comparison source use.
SeongJae Park (10):
mm/damon/core: handle <min_region_sz remaining quota as empty
mm/damon/core: introduce failed region quota charge ratio
mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files
Docs/mm/damon/design: document fail_charge_{num,denom}
Docs/admin-guide/mm/damon/usage: document fail_charge_{num,denom}
files
Docs/ABI/damon: document fail_charge_{num,denom}
mm/damon/tests/core-kunit: test fail_charge_{num,denom} committing
selftests/damon/_damon_sysfs: support failed region quota charge ratio
selftests/damon/drgn_dump_damon_status: support failed region quota
charge ratio
selftests/damon/sysfs.py: test failed region quota charge ratio
.../ABI/testing/sysfs-kernel-mm-damon | 12 +++++
Documentation/admin-guide/mm/damon/usage.rst | 18 +++++--
Documentation/mm/damon/design.rst | 22 ++++++++
include/linux/damon.h | 9 ++++
mm/damon/core.c | 38 ++++++++++---
mm/damon/sysfs-schemes.c | 54 +++++++++++++++++++
mm/damon/tests/core-kunit.h | 6 +++
tools/testing/selftests/damon/_damon_sysfs.py | 21 +++++++-
.../selftests/damon/drgn_dump_damon_status.py | 2 +
tools/testing/selftests/damon/sysfs.py | 6 +++
10 files changed, 175 insertions(+), 13 deletions(-)
base-commit: b1ca86c92674eaf92a32ce3a2d89a0349e406df1
--
2.47.3
^ permalink raw reply
* Re: (sashiko review) [PATCH v6 1/1] mm/damon: add node_eligible_mem_bp and node_ineligible_mem_bp goal metrics
From: SeongJae Park @ 2026-04-07 0:13 UTC (permalink / raw)
To: Ravi Jonnalagadda
Cc: SeongJae Park, damon, linux-mm, linux-kernel, linux-doc, akpm,
corbet, bijan311, ajayjoshi, honggyu.kim, yunjeong.mun
In-Reply-To: <CALa+Y14oWqu5+DbkENy7GgBjc=dCbFTaoOCr1i4=9CN-ZNRgEA@mail.gmail.com>
On Mon, 6 Apr 2026 12:47:56 -0700 Ravi Jonnalagadda <ravis.opensrc@gmail.com> wrote:
> On Sun, Apr 5, 2026 at 3:45 PM SeongJae Park <sj@kernel.org> wrote:
> >
> >
> > Ravi, thank you for reposting this patch after the rebase. This time sashiko
> > was able to review this, and found good points including things that deserve
> > another revision of this patch.
> >
> > Forwarding full sashiko review in a reply format with my inline comments below,
> > for sharing details of my view and doing followup discussions via mails. Ravi,
> > could you please reply?
> >
>
> Thanks SJ, providing your comments on top of sashiko's review is very helpful.
I'm glad to hear that it is working for you :)
[...]
> > > +static unsigned long damos_calc_eligible_bytes(struct damon_ctx *c,
> > > > + struct damos *s, int nid, unsigned long *total)
> > > > +{
[...]
> > > > + struct folio *folio;
> > > > + unsigned long folio_sz, counted;
> > > > +
> > > > + folio = damon_get_folio(PHYS_PFN(addr));
> > >
> > > What happens if this metric is assigned to a DAMON context configured for
> > > virtual address space monitoring? If the context uses DAMON_OPS_VADDR,
> > > passing a user-space virtual address to PHYS_PFN() might cause invalid
> > > memory accesses or out-of-bounds page struct reads. Should this code
> > > explicitly verify the operations type first?
> >
> > Good finding. We intend to support only paddr ops. But there is no guard for
> > using this on vaddr ops configuration. Ravi, could we add underlying ops
> > check? I think damon_commit_ctx() is a good place to add that. The check
> > could be something like below?
> >
>
> I plan to add the ops type check directly in the metric functions
> (damos_get_node_eligible_mem_bp and its counterpart) rather than in
> damon_commit_ctx(). The functions will return 0 early
> if c->ops.id != DAMON_OPS_PADDR.
>
> That said, if you prefer the damon_commit_ctx() validation approach to
> reject the configuration outright, I can implement it that way instead.
> Please let me know your preference.
I'd prefer damon_commit_ctx() validation approach since it would give users
more clear message of the failure.
>
> > '''
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1515,10 +1515,23 @@ static int damon_commit_sample_control(
> > int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
> > {
> > int err;
> > + struct damos *scheme;
> > + struct damos_quota_goal *goal;
> >
> > dst->maybe_corrupted = true;
> > if (!is_power_of_2(src->min_region_sz))
> > return -EINVAL;
> > + if (src->ops.id != DAMON_OPS_PADDR) {
> > + damon_for_each_scheme(scheme, src) {
> > + damos_for_each_quota_goal(goal, &scheme->quota) {
> > + switch (goal->metric) {
> > + case DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP:
> > + case DAMOS_QUOTA_NODE_INELIGIBLE_MEMPBP:
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > + }
> >
> > err = damon_commit_schemes(dst, src);
> > if (err)
> > '''
[...]
> > > > + /* Compute ineligible ratio directly: 10000 - eligible_bp */
> > > > + return 10000 - mult_frac(node_eligible, 10000, total_eligible);
> > > > +}
> > >
> > > Does this return value match the documented metric? The formula computes the
> > > percentage of the system's eligible memory located on other NUMA nodes,
> > > rather than the amount of actual ineligible (filtered out) memory residing
> > > on the target node. Could this semantic mismatch cause confusion when
> > > configuring quota policies?
> >
> > Nice catch. The name and the documentation are confusing. We actually
> > confused a few times in previous revisions, and I'm again confused now. IIUC,
> > the current implementation is the intended and right one for the given use
> > case, though. If my understanding is correct, how about renaming
> > DAMOS_QUOTA_NODE_INELIGIBLE_MEM_BP to
> > DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP_COMPLEMENT, and updating the documentation
> > together? Ravi, what do you think?
> >
>
> Agreed, the current name is confusing. How about
> DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP_OFFNODE?
>
> The rationale is that this metric measures "eligible memory that is off
> this node" (i.e., on other nodes).
>
> I think "offnode" conveys the physical meaning more directly than "complement".
> That said, I'm happy to go with "complement" if you prefer.
> both are clearer than "ineligible".
Thank you for the nice suggestion. I like "offnode" term. But I think having
"node" twice on the name is not really efficient for people who print code on
papers. What about DAMOS_QUOTA_OFFNODE_ELIGIBLE_MEM_BP?
But... Maybe more importantly... Now I realize this means that
offnode_eligible_mem_bp with target nid 0 is just same to node_eligible_mem_bp
with target nid 1, on your test setup. Maybe we don't really need
offnode_eligible_mem_bp? That is, your test setup could be like below.
'''
For maintaining hot memory on DRAM (node 0) and CXL (node 1) in a 7:3
ratio:
PUSH scheme: migrate_hot from node 0 -> node 1
goal: node_eligible_mem_bp, nid=1, target=3000
"Move hot pages from DRAM to CXL if less thatn 30% of hot data is
in CXL"
PULL scheme: migrate_hot from node 1 -> node 0
goal: node_eligible_mem_bp, nid=0, target=7000
"Move hot pages from CXL to DRAM if less than 70% of hot data is
in DRAM"
'''
And the schemes are more easy to read and understand for me. This seems even
straightforward to scale for >2 nodes. For example, if we want hot memory
distribution of 5:3:2 to nodes 0:1:2,
Two schemes for migrating hot pages out of node 0
- migrate_hot from node 0 -> node 1
- goal: node_eligible_mem_bp, nid=1, target=3000
- migrate_hot from node 0 -> node 2
- goal: node_eligible_mem_bp, nid=2, target=2000
Two schemes for migrating hot pages out of node 1
- migrate_hot from node 1 -> node 0
- goal: node_eligible_mem_bp, nid=0, target=5000
- migrate_hot from node 1 -> node 2
- goal: node_eligible_mem_bp, nid=2, target=2000
Two schemes for migrating hot pages out of node 2
- migrate_hot from node 2 -> node 0
- goal: node_eligible_mem_bp, nid=0, target=5000
- migrate_hot from node 2 -> node 1
- goal: node_eligible_mem_bp, nid=1, target=3000
Do you think this makes sense? If it makes sense and works for your use case,
what about dropping the offnode goal type?
Thanks,
SJ
[...]
^ permalink raw reply
* Re: [PATCH v7 9/9] KVM: selftests: nSVM: Add svm_nested_pat test
From: Sean Christopherson @ 2026-04-07 0:07 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <20260327234023.2659476-10-jmattson@google.com>
On Fri, Mar 27, 2026, Jim Mattson wrote:
> When KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT is disabled, verify that KVM
> correctly virtualizes the host PAT MSR and the guest PAT register for
> nested SVM guests.
>
> With nested NPT disabled:
> * L1 and L2 share the same PAT
> * The vmcb12.g_pat is ignored
>
> With nested NPT enabled:
> * An invalid g_pat in vmcb12 causes VMEXIT_INVALID
> * RDMSR(IA32_PAT) from L2 returns the value of the guest PAT register
> * WRMSR(IA32_PAT) from L2 is reflected in vmcb12's g_pat on VMEXIT
> * RDMSR(IA32_PAT) from L1 returns the value of the host PAT MSR
> * Save/restore with the vCPU in guest mode preserves both hPAT and gPAT
>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
> tools/arch/x86/include/uapi/asm/kvm.h | 2 +
Don't update uAPI headers in tools/, they're not used by KVM selftests (perf
folks will sync them as needed).
> +#define PAT_DEFAULT 0x0007040600070406ULL
> +#define L1_PAT_VALUE 0x0007040600070404ULL /* Change PA0 to WT */
> +#define L2_VMCB12_PAT 0x0606060606060606ULL /* All WB */
> +#define L2_PAT_MODIFIED 0x0606060606060604ULL /* Change PA0 to WT */
> +#define INVALID_PAT_VALUE 0x0808080808080808ULL /* 8 is reserved */
> +
> +/*
> + * Shared state between L1 and L2 for verification.
> + */
> +struct pat_test_data {
> + uint64_t l2_pat_read;
> + uint64_t l2_pat_after_write;
> + uint64_t l1_pat_after_vmexit;
> + uint64_t vmcb12_gpat_after_exit;
> + bool l2_done;
> +};
> +
> +static struct pat_test_data *pat_data;
This is ridiculous. Whatever AI you're using is reinventing sync_global_to_guest()
in a very obfuscated way. Drop the indirection along with the params and the
full page allocation, and just sync the damn struct.
Actually, this is even dumber than that. The "data" is only ever accessed from
within the guest; it's used to pass info between L1 and L2. Drop the struct
entirely and just write global variables.
In general, please clean this test up before submitting v8. All of the L2 code
is basically copy+paste of itself. This is the second vibe coded selftest (AFAIK)
you've posted, and it has many of the same flaws as the first one[*]. While I'm
not opposed to using fancy tools, and the bar is generally lower for selftests,
the code still needs to be readable and maintainable. This ain't.
[*] https://lore.kernel.org/all/aXJal3srw2-3J5Dm@google.com
> +static void l2_guest_code(void)
> +{
> + pat_data->l2_pat_read = rdmsr(MSR_IA32_CR_PAT);
> + wrmsr(MSR_IA32_CR_PAT, L2_PAT_MODIFIED);
> + pat_data->l2_pat_after_write = rdmsr(MSR_IA32_CR_PAT);
> + pat_data->l2_done = true;
> + vmmcall();
> +}
...
> +static void run_test(void *l1_code, const char *test_name, bool npt_enabled,
> + bool do_save_restore)
> +{
> + struct pat_test_data *data_hva;
> + vm_vaddr_t svm_gva, data_gva;
> + struct kvm_x86_state *state;
> + struct kvm_vcpu *vcpu;
> + struct kvm_vm *vm;
> + struct ucall uc;
> +
> + pr_info("Testing: %s\n", test_name);
> +
> + vm = vm_create_with_one_vcpu(&vcpu, l1_code);
> + vm_enable_cap(vm, KVM_CAP_DISABLE_QUIRKS2,
> + KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> + if (npt_enabled)
> + vm_enable_npt(vm);
> +
> + vcpu_alloc_svm(vm, &svm_gva);
> +
> + data_gva = vm_vaddr_alloc_page(vm);
> + data_hva = addr_gva2hva(vm, data_gva);
> + memset(data_hva, 0, sizeof(*data_hva));
Ugh.
> +
> + if (npt_enabled)
> + tdp_identity_map_default_memslots(vm);
> +
> + vcpu_args_set(vcpu, 2, svm_gva, data_gva);
^ permalink raw reply
* Re: [PATCH v7 8/9] KVM: x86: nSVM: Save/restore gPAT with KVM_{GET,SET}_NESTED_STATE
From: Sean Christopherson @ 2026-04-06 23:47 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <20260327234023.2659476-9-jmattson@google.com>
On Fri, Mar 27, 2026, Jim Mattson wrote:
> @@ -1918,6 +1921,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> struct vmcb_save_area_cached save_cached;
> struct vmcb_ctrl_area_cached ctl_cached;
> unsigned long cr0;
> + bool use_separate_l2_pat;
Land this above "cr0" to preserve the inverted fir tree.
> int ret;
>
> BUILD_BUG_ON(sizeof(struct vmcb_control_area) + sizeof(struct vmcb_save_area) >
> @@ -1993,6 +1997,18 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> !nested_vmcb_check_save(vcpu, &save_cached, false))
> goto out_free;
>
> + /*
> + * Validate gPAT when the shared PAT quirk is disabled (i.e. L2
> + * has its own gPAT). This is done separately from the
> + * vmcb_save_area_cached validation above, because gPAT is L2
> + * state, but the vmcb_save_area_cached is populated with L1 state.
> + */
> + use_separate_l2_pat =
> + (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
> + !kvm_check_has_quirk(vcpu->kvm,
> + KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
I vote for either:
use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
!kvm_check_has_quirk(vcpu->kvm,
KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
or
use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP);
if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT))
use_separate_l2_pat = false;
> + if (use_separate_l2_pat && !kvm_pat_valid(kvm_state->hdr.svm.gpat))
> + goto out_free;
>
> /*
> * All checks done, we can enter guest mode. Userspace provides
> @@ -2017,6 +2033,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> nested_copy_vmcb_control_to_cache(svm, ctl);
>
> svm_switch_vmcb(svm, &svm->nested.vmcb02);
> +
> + if (use_separate_l2_pat)
> + vmcb_set_gpat(svm->vmcb, kvm_state->hdr.svm.gpat);
> +
> nested_vmcb02_prepare_control(svm);
>
> /*
> --
> 2.53.0.1018.g2bb0e51243-goog
>
^ permalink raw reply
* Re: [PATCH v7 5/9] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
From: Sean Christopherson @ 2026-04-06 23:45 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <20260327234023.2659476-6-jmattson@google.com>
On Fri, Mar 27, 2026, Jim Mattson wrote:
> When KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT is disabled and the vCPU is in
> guest mode with nested NPT enabled, guest accesses to IA32_PAT are
> redirected to the gPAT register, which is stored in VMCB02's g_pat field.
>
> Non-guest accesses (e.g. from userspace) to IA32_PAT are always redirected
> to hPAT, which is stored in vcpu->arch.pat.
>
> Directing host-initiated accesses to hPAT ensures that KVM_GET/SET_MSRS and
> KVM_GET/SET_NESTED_STATE are independent of each other and can be ordered
> arbitrarily during save and restore. gPAT is saved and restored separately
> via KVM_GET/SET_NESTED_STATE.
>
> Use WARN_ON_ONCE to flag any host-initiated accesses originating from KVM
> itself rather than userspace.
>
> Use pr_warn_once to flag any use of the common MSR-handling code (now
> shared by VMX and TDX) for IA32_PAT by a vCPU that is SVM-capable.
>
> Fixes: 15038e147247 ("KVM: SVM: obey guest PAT")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/svm/nested.c | 9 -------
> arch/x86/kvm/svm/svm.c | 53 ++++++++++++++++++++++++++++++++++-----
> arch/x86/kvm/svm/svm.h | 1 -
> arch/x86/kvm/x86.c | 6 +++++
> 4 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 8170042d5fb3..ccc556eb4d2f 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -703,15 +703,6 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
> return 0;
> }
>
> -void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm)
> -{
> - if (!svm->nested.vmcb02.ptr)
> - return;
> -
> - /* FIXME: merge g_pat from vmcb01 and vmcb12. */
> - vmcb_set_gpat(svm->nested.vmcb02.ptr, svm->vmcb01.ptr->save.g_pat);
> -}
> -
> static bool nested_vmcb12_has_lbrv(struct kvm_vcpu *vcpu)
> {
> return guest_cpu_cap_has(vcpu, X86_FEATURE_LBRV) &&
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index af808e83173e..34fd07d6ad4d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2776,6 +2776,47 @@ static bool sev_es_prevent_msr_access(struct kvm_vcpu *vcpu,
> !msr_write_intercepted(vcpu, msr_info->index);
> }
>
> +static bool svm_pat_accesses_gpat(struct kvm_vcpu *vcpu, bool from_host)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> +
> + /*
> + * When nested NPT is enabled, L2 has a separate PAT from L1. Guest
Hmm, do we also want to add "and KVM's quirk is disabled"?
/*
* When nested NPT is enabled and KVM's quirk is disabled, L2 has a
* separate PAT from L1. Guest accesses to IA32_PAT while running L2
* target L2's gPAT; host-initiated accesses always target L1's hPAT so
* that KVM_GET/SET_MSRS and KVM_GET/SET_NESTED_STATE are independent
* of each other and can be ordered arbitrarily during save and restore.
*/
> + * accesses to IA32_PAT while running L2 target L2's gPAT;
> + * host-initiated accesses always target L1's hPAT so that
> + * KVM_GET/SET_MSRS and KVM_GET/SET_NESTED_STATE are independent of
> + * each other and can be ordered arbitrarily during save and restore.
> + */
> + WARN_ON_ONCE(from_host && vcpu->wants_to_run);
> + return !from_host && is_guest_mode(vcpu) && l2_has_separate_pat(svm);
> +}
> +
> +static u64 svm_get_pat(struct kvm_vcpu *vcpu, bool from_host)
> +{
> + if (svm_pat_accesses_gpat(vcpu, from_host))
> + return to_svm(vcpu)->vmcb->save.g_pat;
> + else
> + return vcpu->arch.pat;
> +}
> +
> +static void svm_set_pat(struct kvm_vcpu *vcpu, bool from_host, u64 data)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> +
> + if (svm_pat_accesses_gpat(vcpu, from_host)) {
> + vmcb_set_gpat(svm->vmcb, data);
> + return;
> + }
> +
> + svm->vcpu.arch.pat = data;
> +
> + if (npt_enabled) {
> + vmcb_set_gpat(svm->vmcb01.ptr, data);
> + if (is_guest_mode(&svm->vcpu) && !l2_has_separate_pat(svm))
> + vmcb_set_gpat(svm->vmcb, data);
> + }
> +}
> +
> static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> @@ -2892,6 +2933,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_AMD64_DE_CFG:
> msr_info->data = svm->msr_decfg;
> break;
> + case MSR_IA32_CR_PAT:
> + msr_info->data = svm_get_pat(vcpu, msr_info->host_initiated);
> + break;
> default:
> return kvm_get_msr_common(vcpu, msr_info);
> }
> @@ -2975,13 +3019,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>
> break;
> case MSR_IA32_CR_PAT:
> - ret = kvm_set_msr_common(vcpu, msr);
> - if (ret)
> - break;
> + if (!kvm_pat_valid(data))
> + return 1;
>
> - vmcb_set_gpat(svm->vmcb01.ptr, data);
> - if (is_guest_mode(vcpu))
> - nested_vmcb02_compute_g_pat(svm);
> + svm_set_pat(vcpu, msr->host_initiated, data);
> break;
> case MSR_IA32_SPEC_CTRL:
> if (!msr->host_initiated &&
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index b43e37b0448c..0b0279734486 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -868,7 +868,6 @@ void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
> void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> struct vmcb_save_area *save);
> void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
> -void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
> void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
>
> extern struct kvm_x86_nested_ops svm_nested_ops;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0b5d48e75b65..cfb2517f692a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4025,6 +4025,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> }
> break;
> case MSR_IA32_CR_PAT:
> + if (!(efer_reserved_bits & EFER_SVME))
> + pr_warn_once("%s: MSR_IA32_CR_PAT should be handled by AMD vendor-specific code\n", __func__);
If we want to add a sanity check, simply do
WARN_ON_ONCE(efer_reserved_bits & EFER_SVME)?
so that bugs fail loudly.
Hrm. Though I'm leaning towards saying we should keep the "common" behavior in
x86.c. If VMX and TDX didn't need to add get() APIs, I would say move the common
code to vmx/common.h, but that ends up being pretty ugly.
If we do that, the total diff for svm.c is quite nice. Duplicating the
kvm_pat_valid() sucks, but it seems like the lesser of all evils :-/
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e7fdd7a9c280..6281599d5311 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2767,6 +2767,21 @@ static bool sev_es_prevent_msr_access(struct kvm_vcpu *vcpu,
!msr_write_intercepted(vcpu, msr_info->index);
}
+static bool svm_pat_accesses_gpat(struct kvm_vcpu *vcpu, bool from_host)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ /*
+ * When nested NPT is enabled, L2 has a separate PAT from L1. Guest
+ * accesses to IA32_PAT while running L2 target L2's gPAT;
+ * host-initiated accesses always target L1's hPAT so that
+ * KVM_GET/SET_MSRS and KVM_GET/SET_NESTED_STATE are independent of
+ * each other and can be ordered arbitrarily during save and restore.
+ */
+ WARN_ON_ONCE(from_host && vcpu->wants_to_run);
+ return !from_host && is_guest_mode(vcpu) && l2_has_separate_pat(svm);
+}
+
static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -2883,6 +2898,12 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_AMD64_DE_CFG:
msr_info->data = svm->msr_decfg;
break;
+ case MSR_IA32_CR_PAT:
+ if (svm_pat_accesses_gpat(vcpu, msr_info->host_initiated)) {
+ msr_info->data = to_svm(vcpu)->vmcb->save.g_pat;
+ break;
+ }
+ return kvm_get_msr_common(vcpu, msr_info);
default:
return kvm_get_msr_common(vcpu, msr_info);
}
@@ -2966,14 +2987,23 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
break;
case MSR_IA32_CR_PAT:
+ if (svm_pat_accesses_gpat(vcpu, msr->host_initiated)) {
+ if (kvm_pat_valid(data))
+ return 1;
+
+ vmcb_set_gpat(svm->vmcb, data);
+ break;
+ }
+
ret = kvm_set_msr_common(vcpu, msr);
if (ret)
break;
- svm->vmcb01.ptr->save.g_pat = data;
- if (is_guest_mode(vcpu))
- nested_vmcb02_compute_g_pat(svm);
- vmcb_mark_dirty(svm->vmcb, VMCB_NPT);
+ if (npt_enabled) {
+ vmcb_set_gpat(svm->vmcb01.ptr, data);
+ if (is_guest_mode(&svm->vcpu) && !l2_has_separate_pat(svm))
+ vmcb_set_gpat(svm->vmcb, data);
+ }
break;
case MSR_IA32_SPEC_CTRL:
if (!msr->host_initiated &&
> +
> if (!kvm_pat_valid(data))
> return 1;
>
> @@ -4436,6 +4439,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> break;
> }
> case MSR_IA32_CR_PAT:
> + if (!(efer_reserved_bits & EFER_SVME))
> + pr_warn_once("%s: MSR_IA32_CR_PAT should be handled by AMD vendor-specific code\n", __func__);
And obviously the same here..
> +
> msr_info->data = vcpu->arch.pat;
> break;
> case MSR_MTRRcap:
> --
> 2.53.0.1018.g2bb0e51243-goog
>
^ permalink raw reply related
* Re: [PATCH v7 1/9] KVM: x86: Define KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT
From: Sean Christopherson @ 2026-04-06 23:27 UTC (permalink / raw)
To: Jim Mattson
Cc: Paolo Bonzini, Jonathan Corbet, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
kvm, linux-doc, linux-kernel, linux-kselftest, Yosry Ahmed
In-Reply-To: <20260327234023.2659476-2-jmattson@google.com>
On Fri, Mar 27, 2026, Jim Mattson wrote:
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index ff1e4b4dc998..74014110b550 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -616,6 +616,17 @@ static inline bool nested_npt_enabled(struct vcpu_svm *svm)
> return svm->nested.ctl.misc_ctl & SVM_MISC_ENABLE_NP;
> }
>
> +static inline bool l2_has_separate_pat(struct vcpu_svm *svm)
Take @vcpu instead of @svm. All of the callers have a "vcpu", but not all have
a local "svm". That will shorten the quirk check far enough to let it poke out.
> +{
> + /*
> + * If KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT is disabled while a vCPU
> + * is running, the L2 IA32_PAT semantics for that vCPU are undefined.
> + */
> + return nested_npt_enabled(svm) &&
> + !kvm_check_has_quirk(svm->vcpu.kvm,
> + KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
Align indentation. With the @svm => @vcpu change, this becomes:
return nested_npt_enabled(to_svm(vcpu)) &&
!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> +}
> +
> static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
> {
> return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_VNMI) &&
> --
> 2.53.0.1018.g2bb0e51243-goog
>
^ permalink raw reply
* [PATCH] riscv: Docs: fix unmatched quote warning
From: Randy Dunlap @ 2026-04-06 23:23 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Deepak Gupta, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, linux-riscv, Jonathan Corbet,
Shuah Khan, linux-doc
'make htmldocs' complains about ``prctrl` -- so add a second '`' to
avoid the warning.
Documentation/arch/riscv/zicfilp.rst:79: WARNING: Inline literal start-string without end-string. [docutils]
Fixes: 08ee1559052b ("prctl: cfi: change the branch landing pad prctl()s to be more descriptive")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
---
Cc: Deepak Gupta <debug@rivosinc.com>
Cc: Paul Walmsley <pjw@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Alexandre Ghiti <alex@ghiti.fr>
Cc: linux-riscv@lists.infradead.org
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: linux-doc@vger.kernel.org
Documentation/arch/riscv/zicfilp.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-next-20260406.orig/Documentation/arch/riscv/zicfilp.rst
+++ linux-next-20260406/Documentation/arch/riscv/zicfilp.rst
@@ -78,7 +78,7 @@ the program.
Per-task indirect branch tracking state can be monitored and
controlled via the :c:macro:`PR_GET_CFI` and :c:macro:`PR_SET_CFI`
-``prctl()` arguments (respectively), by supplying
+``prctl()`` arguments (respectively), by supplying
:c:macro:`PR_CFI_BRANCH_LANDING_PADS` as the second argument. These
are architecture-agnostic, and will return -EINVAL if the underlying
functionality is not supported.
^ permalink raw reply
* Re: [PATCH v2 00/16] fs,x86/resctrl: Add kernel-mode (e.g., PLZA) support to the resctrl subsystem
From: Babu Moger @ 2026-04-06 22:45 UTC (permalink / raw)
To: Reinette Chatre, corbet, tony.luck, Dave.Martin, james.morse,
tglx, mingo, bp, dave.hansen
Cc: skhan, x86, hpa, peterz, juri.lelli, vincent.guittot,
dietmar.eggemann, rostedt, bsegall, mgorman, vschneid, kas,
rick.p.edgecombe, akpm, pmladek, rdunlap, dapeng1.mi, kees, elver,
paulmck, lirongqing, safinaskar, fvdl, seanjc, pawan.kumar.gupta,
xin, tiala, Neeraj.Upadhyay, chang.seok.bae, thomas.lendacky,
elena.reshetova, linux-doc, linux-kernel, linux-coco, kvm,
eranian, peternewman
In-Reply-To: <83ae0c18-5c5e-4b52-901d-4126fe7c141b@intel.com>
Hi Reinette,
Sorry for the late response. I was trying to get confirmation about the
use case.
On 3/31/26 17:24, Reinette Chatre wrote:
> Hi Babu,
>
> On 3/30/26 11:46 AM, Babu Moger wrote:
>> On 3/27/26 17:11, Reinette Chatre wrote:
>>> On 3/26/26 10:12 AM, Babu Moger wrote:
>>>> On 3/24/26 17:51, Reinette Chatre wrote:
>>>>> On 3/12/26 1:36 PM, Babu Moger wrote:
>
>>>>>> Tony suggested using global variables to store the kernel mode
>>>>>> CLOSID and RMID. However, the kernel mode CLOSID and RMID are
>>>>>> coming from rdtgroup structure with the new interface. Accessing
>>>>>> them requires holding the associated lock, which would make the
>>>>>> context switch path unnecessarily expensive. So, dropped the idea.
>>>>>> https://lore.kernel.org/lkml/aXuxVSbk1GR2ttzF@agluck-desk3/
>>>>>> Let me know if there are other ways to optimize this.
>>>>> I do not see why the context switch path needs to be touched at all with this
>>>>> implementation. Since PLZA only supports global assignment does it not mean that resctrl
>>>>> only needs to update PQR_PLZA_ASSOC when user writes to info/kernel_mode and
>>>>> info/kernel_mode_assignment?
>>>> Each thread has an MSR to configure whether to associate privilege level zero execution with a separate COS and/or RMID, and the value of the COS and/or RMID. PLZA may be enabled or disabled on a per-thread basis. However, the COS and RMID association and configuration must be the same for all threads in the QOS Domain.
>>> Based on previous comment in https://lore.kernel.org/lkml/abb049fa-3a3d-4601-9ae3-61eeb7fd8fcf@amd.com/
>>> and this implementation all fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_en must be the
>>> same for all CPUs on the system, not just per QoS domain. Could you please confirm?
>>
>> Sorry for the confusion. It is "per QoS domain".
>>
>> All the fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_enmust be set to the same value for all HW threads in the QOS domain for consistent operation (Per-QosDomain).
>
> Thank you for clarifying. To build on this, what would be best way for resctrl to interpret this?
> As I see it all values in PQR_PLZA_ASSOC apply to *all* resources yet (theoretically?) every resource
Yes. That is correct. PLZA applies to all the resources.
> can have domains that span different CPUs. There thus seem to be a built in assumption of what a "domain"
> means for PQR_PLZA_ASSOC so it sounds to me as though, instead of saying that "PQR_PLZA_ASSOC needs
> to be the same in QoS domain" it may be more accurate to, for example, say that "PQR_PLZA_ASSOC has L3 scope"?
Yes.
>
> This seems to be what this implementation does since it hardcodes PQR_PLZA_ASSOC scope to the L3
> resource but that creates dependency to the L3 resource that would make PLZA unusable if, for example,
> the user boots with "rdt=!l3cat" while wanting to use PLZA to manage MBA allocations when in kernel?
Yes. that is correct. It should not be attached to one resource. We need
to change it to global scope.
>
> ...
>
>> Yes, I agree with your concerns. The goal here is to make the interface less disruptive while still addressing the different use cases.
>
> I consider changing resctrl behavior when values are written to existing resctrl files
> to be disruptive. This is something we explicitly discussed during v1 as something to
> be avoided so this implementation that overloads the tasks file again is unexpected.
Yes. Agree. If required we need to introduce new files (kmode_cpus,
kmode_cpu_list or kmode_tasks) to handle these cases.
>
>> Background: Customers have identified an issue with the QoS
>> Bandwidth Control feature: when a CLOS is aggressively throttled
>> and execution transitions into kernel mode, kernel operations are
>> also subject to the same aggressive throttling.
>>
>>> Privilege-Level Zero Association (PLZA) allows a user to specify a
>> COS and/or RMID to be used during execution at Privilege Level Zero.
>> When PLZA is enabled on a hardware thread, any execution that enters
>> Privilege Level Zero will have its transactions associated with the
>> PLZA COS and/or RMID. Otherwise, the thread continues to use the COS
>> and RMID specified by |PQR_ASSOC|. In other words, the hardware
>> provides a dedicated COS and/or RMID specifically for kernel-mode
>> execution.
> ack.
>
>>
>> There are multiple ways this feature can be applied. For simplicity, the discussion below focuses only on CLOSID.
>>
>>
>> 1. Global PLZA enablement
>>
>> PLZA can be configured as a global feature by setting |PQR_PLZA_ASSOC.closid = CLOSID| and |PQR_PLZA_ASSOC.plza_en = 1| on all threads in the system. A dedicated CLOSID is reserved for this purpose,
>
> Also discussed during v1 is that there is no need to dedicate a CLOSID for this purpose.
> There could be an "unthrottled" CLOSID to which all high priority user space tasks as
> well as all kernel work of all tasks are assigned.
> If user space chooses to dedicate a CLOSID for kernel work then that should supported and
> interface can allow that, but there is no need for resctrl to enforce this.
>
>> and all CPU threads use its allocations whenever they enter Privilege Level Zero. This CLOSID does not need to be associated with any resctrl group.
I misspoke here.
>
> The CLOSID has to be associated with a resource group to be able to manage its
> resource allocations, no?
Yes. We need to have resource group schemata to enforce the limits.
>
>> The user can explicitly enable or disable this feature.
> ack.
>
>> There is no context switch overhead but there is no flexibility with this approach.
>
> Flexibility is subjective. As I understand this supports the only use case we learned about so far:
> https://lore.kernel.org/lkml/CABPqkBSq=cgn-am4qorA_VN0vsbpbfDePSi7gubicpROB1=djw@mail.gmail.com/
>
>> 2. Group based PLZA allocation : PLZA is managed via dedicated
>> restctrl group. A separate resctrl group can be created
>> specifically for PLZA, with a dedicated CLOSID used exclusively
>> for kernel mode execution. This approach can be further divided
>> into two association models:
>
> So far this sounds like global allocation since both need a dedicated resource group.
> Whether this group is dedicated to kernel work or shared between kernel and user space work
> is up to the user. There is no motivation why CLOSID should ever be enforced to be
> exclusive for kernel mode execution.
Yes. That is fine.
>
>>
>> i) CPU based association
>> CPUs are assigned to the PLZA group, and PLZA is enabled only on
>> those CPUs. This effectively creates a dedicated PLZA group. MSRs (|
>> PQR_PLZA_ASSOC)| are programmed only when the user changes CPU
>> assignments. This approach requires no changes to the context switch
>> code and introduces no additional context switch overhead.
>>
>> ii) Task based association
>> Tasks are explicitly assigned by the user to the PLZA group. Tasks
>> need to be updated when user adds a new task. Also, this requires
>> updates during task scheduling so that the MSRs (|PQR_PLZA_ASSOC)|
>> are programmed on each context switch, which introduces additional
>> context switch overhead.
>
> As discussed during v1 any changes needed to support per task assignment would
> need to be done with new files dedicated to this purpose. Do not overload the
> existing resctrl tasks/cpus/cpus_list files.
Yes. Sure.
>
>> I tried to fit these requirements into the interface files in /sys/
>> fs/resctrl/info/. I may have missed few things while trying to
>> achieve it. As usual, I am open for the discussion and
>> recommendations.
>
> Many of these items were already discussed as part of v1 so I think we may be
> talking past each other here. I tried to highlight the relevant points raised
> during v1 discussion that I thought there already was agreement on.
>
> The one new aspect is that I assumed this implementation will only be for
> global configuration and assignment. It looks like you want to support both
> global configuration and per-task assignment. In the original I did not consider
> configuration and assignment to occur at different scope so we may need to come up
> with new modes to distinguish. Consider the addition of two modes as below:
>
> # cat info/kernel_mode
> [inherit_ctrl_and_mon]
> global_assign_ctrl_inherit_mon_set_all
> global_assign_ctrl_assign_mon_set_all
> global_assign_ctrl_inherit_mon_set_individual
> global_assign_ctrl_assign_mon_set_individual
>
> Above introduces a "set_all" and "set_individual" suffix to the original two
> modes.
>
> global_assign_ctrl_inherit_mon_set_all
> global_assign_ctrl_assign_mon_set_all:
>
> Above are the original two modes but makes it clear that when this mode is
> activated _all_ tasks run with the assignment.
>
> global_assign_ctrl_inherit_mon_set_individual
> global_assign_ctrl_assign_mon_set_individual:
>
> Above are two new modes. In this mode user space also assigns a resource
> group globally but then needs to follow that up by activating every task
> separately to run with this assignment.
> One way in which this can be accomplished could be to have "kernel_mode_tasks",
> "kernel_mode_cpus", and "kernel_mode_cpus_list" files become visible (or be
> created) in the resource group found in info/kernel_mode_assignment. User
> space interacts with the new files to set which tasks and/or CPUs run with
> PLZA enabled.
>
> Even so, as I understand global_assign_ctrl_inherit_mon_set_all and
> global_assign_ctrl_assign_mon_set_all addresses the only known use case. Do you know
> if there are use cases for global_assign_ctrl_inherit_mon_set_individual and
> global_assign_ctrl_assign_mon_set_individual? The latter two adds significant
> complexity to resctrl while I have not heard about any use case for it.
>
Yes. I agree. The changes in context switch code is a concern.
You covered some of the cases I was thinking(xx_set_individual).
How about this idea?
I suggest splitting the PLZA into two distinct aspects:
1. How PLZA is applied within a resource group
2. How PLZA is monitored
Introduce a new file, "info/kmode_type", to describe how kmode applies
in the system.
# cat info/kmode_type
[global] <- Kernel mode applies to the entire system (all CPUs/tasks)
cpus <- Kernel mode applies only to the CPUs in the group
tasks <- Kernel mode applies only to the tasks in the group
The "global" option is the default right now and it is current common
use-case.
The "info/kmode_type -> cpus" option introduces new files "kmode_cpus"
and "kmode_cpus_list" for users to apply kmode to specific set of CPUs.
This lets users change the CPU set for PLZA. The PLZA MSR is updated
when user changes the association to the file. No context switch code
changes are needed. This will be dedicated group. The current resctrl
group files, "cpus, cpus_list and tasks" will not be accessible in this
mode. This option give some flexibility for the user without the context
switch overhead.
The "info/kmode_type -> tasks" option introduces a new file,
"kmode_tasks", for users to apply kmode to specific set of tasks. This
requires context switch changes. This will be dedicated group. The
current resctrl group files, "cpus, cpus_list and tasks" will not be
accessible in this mode. We currently have no use case for this, so it
will not be supported now.
Add a file, "info/kmode_monitor", to describe how kmode is monitored.
# cat info/kmode_monitor
[inherit_ctrl_and_mon] <- Kernel uses the same CLOSID/RMID as user.
Default option for the "global"
assign_ctrl_inherit_mon <- One CLOSID for all kernel work; RMID
inherited from user.
assign_ctrl_assign_mon <- One resource group (CLOSID+RMID) for all
kernel work. Default option for "cpu" type.
Rename “kernel_mode_assignment” to “kmode_group” to assign the specific
group to kmode. This file usage is same as before.
#cat info/kmode_groups (Renamed "kernel_mode_assignment")
//
Thoughts?
thanks
Babu
^ permalink raw reply
* Re: [PATCH v10 07/21] gpu: nova-core: mm: Add TLB flush support
From: Joel Fernandes @ 2026-04-06 22:10 UTC (permalink / raw)
To: Matthew Brost
Cc: linux-kernel, Miguel Ojeda, Boqun Feng, Gary Guo, Bjorn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dave Airlie, Daniel Almeida, Koen Koning,
dri-devel, rust-for-linux, Nikola Djukic, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Jonathan Corbet, Alex Deucher, Christian Koenig, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Huang Rui,
Matthew Auld, Lucas De Marchi, Thomas Hellstrom, Helge Deller,
Alex Gaynor, Boqun Feng, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Zhi Wang, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, alexeyi, Eliot Courtney, joel, linux-doc, amd-gfx,
intel-gfx, intel-xe, linux-fbdev
In-Reply-To: <0f5605c1-32e8-4a62-b852-b1db01e42817@nvidia.com>
On 4/6/2026 5:24 PM, Joel Fernandes wrote:
>
>
> On 4/2/2026 1:59 AM, Matthew Brost wrote:
>> On Tue, Mar 31, 2026 at 05:20:34PM -0400, Joel Fernandes wrote:
>>> Add TLB (Translation Lookaside Buffer) flush support for GPU MMU.
>>>
>>> After modifying page table entries, the GPU's TLB must be invalidated
>>> to ensure the new mappings take effect. The Tlb struct provides flush
>>> functionality through BAR0 registers.
>>>
>>> The flush operation writes the page directory base address and triggers
>>> an invalidation, polling for completion with a 2 second timeout matching
>>> the Nouveau driver.
>>>
>>> Cc: Nikola Djukic <ndjukic@nvidia.com>
>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> ---
>>> drivers/gpu/nova-core/mm.rs | 1 +
>>> drivers/gpu/nova-core/mm/tlb.rs | 95 +++++++++++++++++++++++++++++++++
>>> drivers/gpu/nova-core/regs.rs | 42 +++++++++++++++
>>> 3 files changed, 138 insertions(+)
>>> create mode 100644 drivers/gpu/nova-core/mm/tlb.rs
>>>
>>> diff --git a/drivers/gpu/nova-core/mm.rs b/drivers/gpu/nova-core/mm.rs
>>> index 8f3089a5fa88..cfe9cbe11d57 100644
>>> --- a/drivers/gpu/nova-core/mm.rs
>>> +++ b/drivers/gpu/nova-core/mm.rs
>>> @@ -5,6 +5,7 @@
>>> #![expect(dead_code)]
>>>
>>> pub(crate) mod pramin;
>>> +pub(crate) mod tlb;
>>>
>>> use kernel::sizes::SZ_4K;
>>>
>>> diff --git a/drivers/gpu/nova-core/mm/tlb.rs b/drivers/gpu/nova-core/mm/tlb.rs
>>> new file mode 100644
>>> index 000000000000..cd3cbcf4c739
>>> --- /dev/null
>>> +++ b/drivers/gpu/nova-core/mm/tlb.rs
>>> @@ -0,0 +1,95 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! TLB (Translation Lookaside Buffer) flush support for GPU MMU.
>>> +//!
>>> +//! After modifying page table entries, the GPU's TLB must be flushed to
>>> +//! ensure the new mappings take effect. This module provides TLB flush
>>> +//! functionality for virtual memory managers.
>>> +//!
>>> +//! # Example
>>> +//!
>>> +//! ```ignore
>>> +//! use crate::mm::tlb::Tlb;
>>> +//!
>>> +//! fn page_table_update(tlb: &Tlb, pdb_addr: VramAddress) -> Result<()> {
>>> +//! // ... modify page tables ...
>>> +//!
>>> +//! // Flush TLB to make changes visible (polls for completion).
>>> +//! tlb.flush(pdb_addr)?;
>>> +//!
>>> +//! Ok(())
>>> +//! }
>>> +//! ```
>>> +
>>> +use kernel::{
>>> + devres::Devres,
>>> + io::poll::read_poll_timeout,
>>> + io::Io,
>>> + new_mutex,
>>> + prelude::*,
>>> + sync::{
>>> + Arc,
>>> + Mutex, //
>>> + },
>>> + time::Delta, //
>>> +};
>>> +
>>> +use crate::{
>>> + driver::Bar0,
>>> + mm::VramAddress,
>>> + regs, //
>>> +};
>>> +
>>> +/// TLB manager for GPU translation buffer operations.
>>> +#[pin_data]
>>> +pub(crate) struct Tlb {
>>> + bar: Arc<Devres<Bar0>>,
>>> + /// TLB flush serialization lock: This lock is acquired during the
>>> + /// DMA fence signalling critical path. It must NEVER be held across any
>>> + /// reclaimable CPU memory allocations because the memory reclaim path can
>>> + /// call `dma_fence_wait()`, which would deadlock with this lock held.
>>> + #[pin]
>>> + lock: Mutex<()>,
>>> +}
>>> +
>>> +impl Tlb {
>>> + /// Create a new TLB manager.
>>> + pub(super) fn new(bar: Arc<Devres<Bar0>>) -> impl PinInit<Self> {
>>> + pin_init!(Self {
>>> + bar,
>>> + lock <- new_mutex!((), "tlb_flush"),
>>> + })
>>> + }
>>> +
>>> + /// Flush the GPU TLB for a specific page directory base.
>>> + ///
>>> + /// This invalidates all TLB entries associated with the given PDB address.
>>> + /// Must be called after modifying page table entries to ensure the GPU sees
>>> + /// the updated mappings.
>>> + pub(crate) fn flush(&self, pdb_addr: VramAddress) -> Result {
>>
>> This landed on my list randomly, so I took a look.
>>
>> Wouldn’t you want to virtualize the invalidation based on your device?
>> For example, what if you need to register interface changes on future hardware?
>
> Good point, for future hardware it indeed makes sense. I will do that.
Actually, at least in the future as far as I can see, the register definitions
are the same for TLB invalidation are the same, so we are good and I will not be
making any change in this regard.
But, thanks for raising the point and forcing me to double check!
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH v10 12/21] gpu: nova-core: mm: Add unified page table entry wrapper enums
From: Joel Fernandes @ 2026-04-06 21:55 UTC (permalink / raw)
To: Eliot Courtney, linux-kernel
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Bjorn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Dave Airlie, Daniel Almeida, Koen Koning, dri-devel,
rust-for-linux, Nikola Djukic, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Jonathan Corbet,
Alex Deucher, Christian Koenig, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, Huang Rui, Matthew Auld,
Matthew Brost, Lucas De Marchi, Thomas Hellstrom, Helge Deller,
Alex Gaynor, Boqun Feng, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Zhi Wang, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, alexeyi, joel, linux-doc, amd-gfx, intel-gfx,
intel-xe, linux-fbdev
In-Reply-To: <DHIFF98P1YQ3.1IXUT02E3TF20@nvidia.com>
On 4/2/2026 1:40 AM, Eliot Courtney wrote:
> On Wed Apr 1, 2026 at 6:20 AM JST, Joel Fernandes wrote:
>> Add unified Pte, Pde, and DualPde wrapper enums that abstract over
>> MMU v2 and v3 page table entry formats. These enums allow the page
>> table walker and VMM to work with both MMU versions.
>>
>> Each unified type:
>> - Takes MmuVersion parameter in constructors
>> - Wraps both ver2 and ver3 variants
>> - Delegates method calls to the appropriate variant
>>
>> This enables version-agnostic page table operations while keeping
>> version-specific implementation details encapsulated in the ver2
>> and ver3 modules.
>>
>> Cc: Nikola Djukic <ndjukic@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/mm/pagetable.rs | 330 ++++++++++++++++++++++++++
>> 1 file changed, 330 insertions(+)
>>
>> diff --git a/drivers/gpu/nova-core/mm/pagetable.rs b/drivers/gpu/nova-core/mm/pagetable.rs
>> index 6e01a1af5222..909df37c3ee8 100644
>> --- a/drivers/gpu/nova-core/mm/pagetable.rs
>> +++ b/drivers/gpu/nova-core/mm/pagetable.rs
>> @@ -12,6 +12,13 @@
>> pub(crate) mod ver3;
>>
>> use crate::gpu::Architecture;
>> +use crate::mm::{
>> + pramin,
>> + Pfn,
>> + VirtualAddress,
>> + VramAddress, //
>> +};
>> +use kernel::prelude::*;
>>
>> /// Extracts the page table index at a given level from a virtual address.
>> pub(crate) trait VaLevelIndex {
>> @@ -84,6 +91,96 @@ pub(crate) const fn as_index(&self) -> u64 {
>> }
>> }
>>
>> +impl MmuVersion {
>> + /// Get the `PDE` levels (excluding PTE level) for page table walking.
>> + pub(crate) fn pde_levels(&self) -> &'static [PageTableLevel] {
>> + match self {
>> + Self::V2 => ver2::PDE_LEVELS,
>> + Self::V3 => ver3::PDE_LEVELS,
>> + }
>> + }
>> +
>> + /// Get the PTE level for this MMU version.
>> + pub(crate) fn pte_level(&self) -> PageTableLevel {
>> + match self {
>> + Self::V2 => ver2::PTE_LEVEL,
>> + Self::V3 => ver3::PTE_LEVEL,
>> + }
>> + }
>> +
>> + /// Get the dual PDE level (128-bit entries) for this MMU version.
>> + pub(crate) fn dual_pde_level(&self) -> PageTableLevel {
>> + match self {
>> + Self::V2 => ver2::DUAL_PDE_LEVEL,
>> + Self::V3 => ver3::DUAL_PDE_LEVEL,
>> + }
>> + }
>> +
>> + /// Get the number of PDE levels for this MMU version.
>> + pub(crate) fn pde_level_count(&self) -> usize {
>> + self.pde_levels().len()
>> + }
>> +
>> + /// Get the entry size in bytes for a given level.
>> + pub(crate) fn entry_size(&self, level: PageTableLevel) -> usize {
>> + if level == self.dual_pde_level() {
>> + 16 // 128-bit dual PDE
>> + } else {
>> + 8 // 64-bit PDE/PTE
>> + }
>> + }
>> +
>> + /// Get the number of entries per page table page for a given level.
>> + pub(crate) fn entries_per_page(&self, level: PageTableLevel) -> usize {
>> + match self {
>> + Self::V2 => match level {
>> + // TODO: Calculate these values from the bitfield dynamically
>> + // instead of hardcoding them.
>> + PageTableLevel::Pdb => 4, // PD3 root: bits [48:47] = 2 bits
>> + PageTableLevel::L3 => 256, // PD0 dual: bits [28:21] = 8 bits
>> + _ => 512, // PD2, PD1, PT: 9 bits each
>> + },
>> + Self::V3 => match level {
>> + PageTableLevel::Pdb => 2, // PDE4 root: bit [56] = 1 bit, 2 entries
>> + PageTableLevel::L4 => 256, // PDE0 dual: bits [28:21] = 8 bits
>> + _ => 512, // PDE3, PDE2, PDE1, PT: 9 bits each
>> + },
>> + }
>> + }
>> +
>> + /// Extract the page table index at `level` from `va` for this MMU version.
>> + pub(crate) fn level_index(&self, va: VirtualAddress, level: u64) -> u64 {
>> + match self {
>> + Self::V2 => ver2::VirtualAddressV2::new(va).level_index(level),
>> + Self::V3 => ver3::VirtualAddressV3::new(va).level_index(level),
>> + }
>> + }
>> +
>> + /// Compute upper bound on page table pages needed for `num_virt_pages`.
>> + ///
>> + /// Walks from PTE level up through PDE levels, accumulating the tree.
>> + pub(crate) fn pt_pages_upper_bound(&self, num_virt_pages: usize) -> usize {
>> + let mut total = 0;
>> +
>> + // PTE pages at the leaf level.
>> + let pte_epp = self.entries_per_page(self.pte_level());
>> + let mut pages_at_level = num_virt_pages.div_ceil(pte_epp);
>> + total += pages_at_level;
>> +
>> + // Walk PDE levels bottom-up (reverse of pde_levels()).
>> + for &level in self.pde_levels().iter().rev() {
>> + let epp = self.entries_per_page(level);
>> +
>> + // How many pages at this level do we need to point to
>> + // the previous pages_at_level?
>> + pages_at_level = pages_at_level.div_ceil(epp);
>> + total += pages_at_level;
>> + }
>> +
>> + total
>> + }
>> +}
>> +
>
> We have a lot of matches on the MMU version here (and below in Pte, Pde,
> DualPde). What about making MmuVersion into a trait (e.g. Mmu) with
> associated types for Pte, Pde, DualPde which can implement traits
> defining their common operations too?
I coded this up and it did not look pretty, there's not much LOC savings and the
code becomes harder to read because of parametrization of several functions. Also:
> Then you can parameterise Vmm/PtWalk on this type.
The match still to be done somewhere, so you end up matching on chipset to call
the correct parametrized functions versus just passing in the parameter or
chipset down, in some cases.
For now I am inclined to leave it as is. Also there's a Rust pitfall we all
learnt during the turing and other patch reviews, sometimes doing a bunch of
matches is good especially if the number of variants are expected to be fixed
(in the mm case, version 2 and version 3). Traits have some disadvantages too,
example dyn traits have to heap-allocated, parametrizing can increase code size
(due to monomorphization) etc.
thanks,
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH v10 07/21] gpu: nova-core: mm: Add TLB flush support
From: Joel Fernandes @ 2026-04-06 21:24 UTC (permalink / raw)
To: Matthew Brost
Cc: linux-kernel, Miguel Ojeda, Boqun Feng, Gary Guo, Bjorn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dave Airlie, Daniel Almeida, Koen Koning,
dri-devel, rust-for-linux, Nikola Djukic, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Jonathan Corbet, Alex Deucher, Christian Koenig, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Huang Rui,
Matthew Auld, Lucas De Marchi, Thomas Hellstrom, Helge Deller,
Alex Gaynor, Boqun Feng, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Zhi Wang, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, alexeyi, Eliot Courtney, joel, linux-doc, amd-gfx,
intel-gfx, intel-xe, linux-fbdev
In-Reply-To: <ac4FpcD29XnbbsdD@gsse-cloud1.jf.intel.com>
On 4/2/2026 1:59 AM, Matthew Brost wrote:
> On Tue, Mar 31, 2026 at 05:20:34PM -0400, Joel Fernandes wrote:
>> Add TLB (Translation Lookaside Buffer) flush support for GPU MMU.
>>
>> After modifying page table entries, the GPU's TLB must be invalidated
>> to ensure the new mappings take effect. The Tlb struct provides flush
>> functionality through BAR0 registers.
>>
>> The flush operation writes the page directory base address and triggers
>> an invalidation, polling for completion with a 2 second timeout matching
>> the Nouveau driver.
>>
>> Cc: Nikola Djukic <ndjukic@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/mm.rs | 1 +
>> drivers/gpu/nova-core/mm/tlb.rs | 95 +++++++++++++++++++++++++++++++++
>> drivers/gpu/nova-core/regs.rs | 42 +++++++++++++++
>> 3 files changed, 138 insertions(+)
>> create mode 100644 drivers/gpu/nova-core/mm/tlb.rs
>>
>> diff --git a/drivers/gpu/nova-core/mm.rs b/drivers/gpu/nova-core/mm.rs
>> index 8f3089a5fa88..cfe9cbe11d57 100644
>> --- a/drivers/gpu/nova-core/mm.rs
>> +++ b/drivers/gpu/nova-core/mm.rs
>> @@ -5,6 +5,7 @@
>> #![expect(dead_code)]
>>
>> pub(crate) mod pramin;
>> +pub(crate) mod tlb;
>>
>> use kernel::sizes::SZ_4K;
>>
>> diff --git a/drivers/gpu/nova-core/mm/tlb.rs b/drivers/gpu/nova-core/mm/tlb.rs
>> new file mode 100644
>> index 000000000000..cd3cbcf4c739
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/mm/tlb.rs
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! TLB (Translation Lookaside Buffer) flush support for GPU MMU.
>> +//!
>> +//! After modifying page table entries, the GPU's TLB must be flushed to
>> +//! ensure the new mappings take effect. This module provides TLB flush
>> +//! functionality for virtual memory managers.
>> +//!
>> +//! # Example
>> +//!
>> +//! ```ignore
>> +//! use crate::mm::tlb::Tlb;
>> +//!
>> +//! fn page_table_update(tlb: &Tlb, pdb_addr: VramAddress) -> Result<()> {
>> +//! // ... modify page tables ...
>> +//!
>> +//! // Flush TLB to make changes visible (polls for completion).
>> +//! tlb.flush(pdb_addr)?;
>> +//!
>> +//! Ok(())
>> +//! }
>> +//! ```
>> +
>> +use kernel::{
>> + devres::Devres,
>> + io::poll::read_poll_timeout,
>> + io::Io,
>> + new_mutex,
>> + prelude::*,
>> + sync::{
>> + Arc,
>> + Mutex, //
>> + },
>> + time::Delta, //
>> +};
>> +
>> +use crate::{
>> + driver::Bar0,
>> + mm::VramAddress,
>> + regs, //
>> +};
>> +
>> +/// TLB manager for GPU translation buffer operations.
>> +#[pin_data]
>> +pub(crate) struct Tlb {
>> + bar: Arc<Devres<Bar0>>,
>> + /// TLB flush serialization lock: This lock is acquired during the
>> + /// DMA fence signalling critical path. It must NEVER be held across any
>> + /// reclaimable CPU memory allocations because the memory reclaim path can
>> + /// call `dma_fence_wait()`, which would deadlock with this lock held.
>> + #[pin]
>> + lock: Mutex<()>,
>> +}
>> +
>> +impl Tlb {
>> + /// Create a new TLB manager.
>> + pub(super) fn new(bar: Arc<Devres<Bar0>>) -> impl PinInit<Self> {
>> + pin_init!(Self {
>> + bar,
>> + lock <- new_mutex!((), "tlb_flush"),
>> + })
>> + }
>> +
>> + /// Flush the GPU TLB for a specific page directory base.
>> + ///
>> + /// This invalidates all TLB entries associated with the given PDB address.
>> + /// Must be called after modifying page table entries to ensure the GPU sees
>> + /// the updated mappings.
>> + pub(crate) fn flush(&self, pdb_addr: VramAddress) -> Result {
>
> This landed on my list randomly, so I took a look.
>
> Wouldn’t you want to virtualize the invalidation based on your device?
> For example, what if you need to register interface changes on future hardware?
Good point, for future hardware it indeed makes sense. I will do that.
> Or, if you’re a VF, can you even do MMIO?
For VFs, TLB flush is typically done directly via VF's BAR0 MMIO. If there is
VF-specific handling for any reason, that should probably be a different
interface than a per-chip interface (more specifically, checking if Nova is
booted on a VF).
thanks,
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH v10 10/21] gpu: nova-core: mm: Add MMU v2 page table types
From: Joel Fernandes @ 2026-04-06 21:14 UTC (permalink / raw)
To: Eliot Courtney, linux-kernel
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Bjorn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Dave Airlie, Daniel Almeida, Koen Koning, dri-devel,
rust-for-linux, Nikola Djukic, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Jonathan Corbet,
Alex Deucher, Christian Koenig, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, Huang Rui, Matthew Auld,
Matthew Brost, Lucas De Marchi, Thomas Hellstrom, Helge Deller,
Alex Gaynor, Boqun Feng, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Zhi Wang, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, alexeyi, joel, linux-doc, amd-gfx, intel-gfx,
intel-xe, linux-fbdev
In-Reply-To: <DHIFGC4E879U.AXIIQKGRZQAF@nvidia.com>
On 4/2/2026 1:41 AM, Eliot Courtney wrote:
> On Wed Apr 1, 2026 at 6:20 AM JST, Joel Fernandes wrote:
>> Add page table entry and directory structures for MMU version 2
>> used by Turing/Ampere/Ada GPUs.
>>
>> Cc: Nikola Djukic <ndjukic@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/mm/pagetable.rs | 2 +
>> drivers/gpu/nova-core/mm/pagetable/ver2.rs | 232 +++++++++++++++++++++
>> 2 files changed, 234 insertions(+)
>> create mode 100644 drivers/gpu/nova-core/mm/pagetable/ver2.rs
>>
>> diff --git a/drivers/gpu/nova-core/mm/pagetable.rs b/drivers/gpu/nova-core/mm/pagetable.rs
>> index 50b76d5e5aaf..38d88f8f09a9 100644
>> --- a/drivers/gpu/nova-core/mm/pagetable.rs
>> +++ b/drivers/gpu/nova-core/mm/pagetable.rs
>> @@ -8,6 +8,8 @@
>>
>> #![expect(dead_code)]
>>
>> +pub(crate) mod ver2;
>> +
>
> This looks like it has more visibility than necessary. And it seems
> incorrect for anyone in the crate to care about MMU version details.
> This can probably be just 'mod ver2'. There are a lot of other types /
> functions in this series that could have tighter visibility. Could you
> go through and see if you can reduce a bunch to private or pub(super)?
>
Yes, indeed. I am tightening it now, there are several. Thanks!
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH net-next] docs: netdev: document AI-assisted review tooling
From: Fernando Fernandez Mancera @ 2026-04-06 21:14 UTC (permalink / raw)
To: Nicolai Buchwitz
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jonathan Corbet, Shuah Khan, netdev, workflows,
linux-doc, linux-kernel, mbloch
In-Reply-To: <56c5bdfe2e37738e47b3b4d22e21697c@tipi-net.de>
On 4/6/26 10:24 PM, Nicolai Buchwitz wrote:
> On 6.4.2026 21:58, Fernando Fernandez Mancera wrote:
>> [...]
>
>>
>> Hi Nicolai,
>> maybe I am missing something but [2] isn't from sashiko.dev but from
>> netdev AI CI instead. See: https://netdev-ai.bots.linux.dev/ai-
>> review.html?id=0b114a22-9aab-4265-8bfc-ea1b5bca5514
>
> You're right, I mixed up the two systems - the example I linked was
> from the netdev AI bot, not Sashiko. My mistake on the link.
>
> I stumbled over Sashiko when I noticed the name appearing more often
> in other reviews and then found Jonathan's LWN article about it [1].
>
> Both tools are actively reviewing patches on the list today. I think
> it makes sense to document both rather than just one:
>
> The netdev AI bot at netdev-ai.bots.linux.dev
> Sashiko at sashiko.dev, which posts reviews publicly on its website
> Both use the same review prompts by Chris Mason [2], so there is
> common ground - though results will vary between them due to the
> different AI models (Claude Opus for netdev-ai, Gemini for Sashiko)
> on top of the usual AI uncertainty.
>
> I think it would be useful to document that AI reviews are happening
> but mixing AI bots might confuse people.
>
> Agreed, I'll rework the patch to distinguish the two systems once
> the discussion has been settled.
>
>>
>> The documentation mentioned for running the AI locally is correctly
>> related to netdev AI bot.
>>
>> I think it would be useful to document that AI reviews are happening
>> but mixing AI bots might confuse people.
>>
>>> Check for findings on your submissions and address
>>> +valid ones before a maintainer has to relay the same questions.
>>> +
>>
>> I wonder what would be the consequences for this. If less experienced
>> submitters are expected to address issues pointed out by AI bots they
>> might work on something that isn't valid. AFAIU, the AI output is only
>> forwarded to the submitter after a maintainer reviewed it and believes
>> it makes sense.
>
> Fair point. The wording should make clear that the local tooling is
> an optional aid, not an obligation. I'll soften the language around
> addressing findings.
>
Thank you! Regarding this topic it seems people have been already
discussing this around other subsystems [1]. It might be useful to check
out similar discussions and outcomes.
[1] https://lwn.net/Articles/1064830/
> Would appreciate input on how much detail is appropriate here -
> should the doc just acknowledge that AI review exists and point to
> the tooling, or go into more detail about the workflow?
>
To be honest that is hard for me to tell, I am not a maintainer and not
the one doing the forwarding currently. I think there isn't an official
workflow regarding this. Maybe a good starter would be to just mention
that they exist. Or maybe this is a good opportunity to define an
official workflow!
Thanks,
Fernando.
^ permalink raw reply
* Re: [PATCH v10 03/21] gpu: nova-core: gsp: Expose total physical VRAM end from FB region info
From: Joel Fernandes @ 2026-04-06 21:08 UTC (permalink / raw)
To: Eliot Courtney, linux-kernel
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Bjorn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Dave Airlie, Daniel Almeida, Koen Koning, dri-devel,
rust-for-linux, Nikola Djukic, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Jonathan Corbet,
Alex Deucher, Christian Koenig, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, Huang Rui, Matthew Auld,
Matthew Brost, Lucas De Marchi, Thomas Hellstrom, Helge Deller,
Alex Gaynor, Boqun Feng, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Zhi Wang, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, alexeyi, joel, linux-doc, amd-gfx, intel-gfx,
intel-xe, linux-fbdev
In-Reply-To: <DHIFD6N7QSU1.1RGEN0APPDHD8@nvidia.com>
On 4/2/2026 1:37 AM, Eliot Courtney wrote:
> On Wed Apr 1, 2026 at 6:20 AM JST, Joel Fernandes wrote:
>> Add `total_fb_end()` to `GspStaticConfigInfo` that computes the
>> exclusive end address of the highest valid FB region covering both
>> usable and GSP-reserved areas.
>>
>> This allows callers to know the full physical VRAM extent, not just
>> the allocatable portion.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/gsp/commands.rs | 6 ++++++
>> drivers/gpu/nova-core/gsp/fw/commands.rs | 7 +++++++
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
>> index 41742c1633c8..5e0649024637 100644
>> --- a/drivers/gpu/nova-core/gsp/commands.rs
>> +++ b/drivers/gpu/nova-core/gsp/commands.rs
>> @@ -196,6 +196,9 @@ pub(crate) struct GetGspStaticInfoReply {
>> /// Usable FB (VRAM) region for driver memory allocation.
>> #[expect(dead_code)]
>> pub(crate) usable_fb_region: Range<u64>,
>> + /// End of VRAM.
>> + #[expect(dead_code)]
>> + pub(crate) total_fb_end: u64,
>> }
>>
>> impl MessageFromGsp for GetGspStaticInfoReply {
>> @@ -209,9 +212,12 @@ fn read(
>> ) -> Result<Self, Self::InitError> {
>> let (base, size) = msg.first_usable_fb_region().ok_or(ENODEV)?;
>>
>> + let total_fb_end = msg.total_fb_end().ok_or(ENODEV)?;
>> +
>> Ok(GetGspStaticInfoReply {
>> gpu_name: msg.gpu_name_str(),
>> usable_fb_region: base..base.saturating_add(size),
>> + total_fb_end,
>> })
>> }
>> }
>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
>> index 9fffa74d03f9..46932d5c8c1d 100644
>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>> @@ -163,6 +163,13 @@ pub(crate) fn first_usable_fb_region(&self) -> Option<(u64, u64)> {
>> }
>> })
>> }
>> +
>> + /// Compute the end of physical VRAM from all FB regions.
>> + pub(crate) fn total_fb_end(&self) -> Option<u64> {
>> + self.fb_regions()
>> + .map(|reg| reg.limit.saturating_add(1))
>
> I think it would be better to used checked_add here.
>
Done, thanks.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH net-next] docs: netdev: document AI-assisted review tooling
From: Laurent Pinchart @ 2026-04-06 21:06 UTC (permalink / raw)
To: Nicolai Buchwitz
Cc: Fernando Fernandez Mancera, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Shuah Khan, netdev, workflows, linux-doc, linux-kernel, mbloch
In-Reply-To: <56c5bdfe2e37738e47b3b4d22e21697c@tipi-net.de>
On Mon, Apr 06, 2026 at 10:24:14PM +0200, Nicolai Buchwitz wrote:
> On 6.4.2026 21:58, Fernando Fernandez Mancera wrote:
> > [...]
>
> >
> > Hi Nicolai,
> > maybe I am missing something but [2] isn't from sashiko.dev but from
> > netdev AI CI instead. See:
> > https://netdev-ai.bots.linux.dev/ai-review.html?id=0b114a22-9aab-4265-8bfc-ea1b5bca5514
>
> You're right, I mixed up the two systems - the example I linked was
> from the netdev AI bot, not Sashiko. My mistake on the link.
>
> I stumbled over Sashiko when I noticed the name appearing more often
> in other reviews and then found Jonathan's LWN article about it [1].
>
> Both tools are actively reviewing patches on the list today. I think
> it makes sense to document both rather than just one:
>
> The netdev AI bot at netdev-ai.bots.linux.dev
> Sashiko at sashiko.dev, which posts reviews publicly on its website
> Both use the same review prompts by Chris Mason [2], so there is
> common ground - though results will vary between them due to the
> different AI models (Claude Opus for netdev-ai, Gemini for Sashiko)
> on top of the usual AI uncertainty.
>
> I think it would be useful to document that AI reviews are happening
> but mixing AI bots might confuse people.
>
> Agreed, I'll rework the patch to distinguish the two systems once
> the discussion has been settled.
>
> > The documentation mentioned for running the AI locally is correctly
> > related to netdev AI bot.
> >
> > I think it would be useful to document that AI reviews are happening
> > but mixing AI bots might confuse people.
> >
> >> Check for findings on your submissions and address
> >> +valid ones before a maintainer has to relay the same questions.
> >> +
> >
> > I wonder what would be the consequences for this. If less experienced
> > submitters are expected to address issues pointed out by AI bots they
> > might work on something that isn't valid. AFAIU, the AI output is only
> > forwarded to the submitter after a maintainer reviewed it and believes
> > it makes sense.
>
> Fair point. The wording should make clear that the local tooling is
> an optional aid, not an obligation. I'll soften the language around
> addressing findings.
>
> Would appreciate input on how much detail is appropriate here -
> should the doc just acknowledge that AI review exists and point to
> the tooling, or go into more detail about the workflow?
In general, if a workflow is expected by a subsystem, it should be
documented. I don't see much to be gained from not telling submitters
what they're expecting to do.
More precisely in this case, as a submitter, I would take it pretty
badly if I was told to act on the output of a tool that is prone to
hallucinations without a maintainer first triaging the comments.
> [1] https://lwn.net/Articles/1063292/
> [2] https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/networking.md
>
> >> +You can also run AI reviews locally before submitting. Instructions
> >> +and tooling are available at:
> >> +
> >> + https://netdev-ai.bots.linux.dev/ai-local.html
> >> +
> >> Testimonials / feedback
> >> -----------------------
> >>
>
> Thanks for your input
>
> Nicolai
--
Regards,
Laurent Pinchart
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox