[PATCH 1/2] RFC drm/xe: Add configfs to enable survivability mode
Rodrigo Vivi
rodrigo.vivi at intel.com
Mon Mar 10 17:14:02 UTC 2025
On Mon, Mar 10, 2025 at 12:01:55PM -0500, Lucas De Marchi wrote:
> On Mon, Mar 10, 2025 at 12:40:39PM -0400, Rodrigo Vivi wrote:
> > On Mon, Mar 10, 2025 at 07:53:27PM +0530, Riana Tauro wrote:
> > > Hi Lucas
> > >
> > > On 3/10/2025 7:01 PM, Lucas De Marchi wrote:
> > > > On Mon, Mar 10, 2025 at 11:01:50AM +0530, Riana Tauro wrote:
> > > > > Hi Lucas
> > > > >
> > > > > On 3/7/2025 8:46 PM, Lucas De Marchi wrote:
> > > > > > On Fri, Mar 07, 2025 at 07:54:44PM +0530, Riana Tauro wrote:
> > > > > > > Registers a configfs subsystem called 'xe' to userspace. The user can
> > > > > > > use this to modify exposed attributes.
> > > > > > >
> > > > > > > Add survivability mode attribute (config/xe/survivability_mode) to the
> > > > > > > subsystem to allow the user to specify the card that should enter
> > > > > > > survivability mode.
> > > > > > >
> > > > > > > Signed-off-by: Riana Tauro <riana.tauro at intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/xe/Makefile | 1 +
> > > > > > > drivers/gpu/drm/xe/xe_configfs.c | 95 ++++++++++++++++++++++++++++++++
> > > > > > > drivers/gpu/drm/xe/xe_configfs.h | 12 ++++
> > > > > > > drivers/gpu/drm/xe/xe_module.c | 5 ++
> > > > > > > drivers/gpu/drm/xe/xe_module.h | 1 +
> > > > > > > 5 files changed, 114 insertions(+)
> > > > > > > create mode 100644 drivers/gpu/drm/xe/xe_configfs.c
> > > > > > > create mode 100644 drivers/gpu/drm/xe/xe_configfs.h
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > > > > > > index 9699b08585f7..3f8c87292cee 100644
> > > > > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > > > > @@ -28,6 +28,7 @@ $(obj)/generated/%_wa_oob.c
> > > > > > > $(obj)/generated/ %_wa_oob.h: $(obj)/xe_gen_wa_oob \
> > > > > > > xe-y += xe_bb.o \
> > > > > > > xe_bo.o \
> > > > > > > xe_bo_evict.o \
> > > > > > > + xe_configfs.o \
> > > > > > > xe_devcoredump.o \
> > > > > > > xe_device.o \
> > > > > > > xe_device_sysfs.o \
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > > > > > > b/drivers/gpu/drm/xe/ xe_configfs.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..8c5f248e466d
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > > > > > @@ -0,0 +1,95 @@
> > > > > > > +// SPDX-License-Identifier: MIT
> > > > > > > +/*
> > > > > > > + * Copyright © 2025 Intel Corporation
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/configfs.h>
> > > > > > > +#include <linux/init.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +
> > > > > > > +#include "xe_configfs.h"
> > > > > > > +#include "xe_module.h"
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * DOC: Xe Configfs
> > > > > > > + *
> > > > > > > + * XE KMD registers a configfs subsystem called 'xe'to
> > > > > > > userspace that allows users to modify
> > > > > > > + * the exposed attributes.
> > > > > > > + *
> > > > > > > + * Attributes:
> > > > > > > + *
> > > > > > > + * config/xe/survivability_mode : Write only attribute that
> > > > > > > allows user to specify the PCI address
> > > > > > > + * of the card that has to enter survivability mode
> > > > > >
> > > > > > I think the desired interface is actually that the user mkdir the bdf in
> > > > > > <configfs>/xe/. This populates the available config entries that
> > > > > > the user
> > > > > > writes to.
> > > > >
> > > > > Initial thought was mkdir bdf, but since it was a single attribute
> > > > > and after a offline discussion with Rodrigo, did a simpler version
> > > > > to get comments on the RFC patch and if configfs is okay to use
> > > > > for survivability mode
> > > >
> > > > I disagree on the conclusion as then you are moving away from how
> > > > configfs is commonly used and also making it harder to add new
> > > > attributes in a uniform way.
> >
> > My bad, I'm sorry.
> > I might have created the confusion here.
> >
> > > >
> > > > >
> > > > > For survivability mode, below procedure needs to be followed
> > > > >
> > > > > echo "0000:03:00.0" > /sys/bus/pci/drivers/xe/unbind
> > > > > add bdf for survivability mode
> > > > > echo "0000:03:00.0" > /sys/bus/pci/drivers/xe/bind
> > > > >
> > > > > After the device is removed directory has to be created, the bdf has
> > > > > to be checked if it belongs to xe driver and then attrs populated.
> > > > > Even i think mkdir is better in case other attrs have to be added in
> > > > > future, but for unbind case the check of the driver might be tricky
> > > > > .
> > > >
> > > > I think you are talking about the mkdir in the wrong place. It isn't
> > > > related to unbind at all. The sequence you mentioned is just because we
> > > > lazy and let pci auto probe the driver. Under the hood you are doing:
> > > >
> > > > 1) load the module
> > > > 2) bind the driver
> > > > 3) unbind the driver
> > > > 4) bind the driver in survivability mode
> > > >
> > > > The sequence we should actually have is:
> > > >
> > > > 1) load the module
> > > > 2) bind the driver in survivability mode
> >
> > No, this doesn't work for the use case we want it.
> >
> > The goal is to be able to only move one single device to the survivability
> > mode at runtime without disrupting the work that is going on on other devices.
> > So we cannot unload the module and remove the autoprobe.
>
> where are you seeing the need to remove the module in the above
> sequence?
>
> See that step 1 is about loading the module. *If* the module
> was already loaded, then unbinding it would make sense, but it's not a
> required step.
Ah okay. Yeap, in the use case that we have the module is already loaded
and we don't want to disrupt other running GPUs. So let's considered
that module is already loaded at boot and we need to unbind. So we are
in the same page and avoid some confusion here.
so,
1. unbind specific device
2. configfs (create dirs/files needed with the content of the desired pci bus address.)
3. bind specific device (during the bind we check the config against the device it is
getting probed and move to survivability mode. ignore otherwise and proceed with the probe)
>
> Lucas De Marchi
>
> >
> > >
> > > This is what i meant.
> > >
> > > 1) load the module
> > > 2) bind the driver
> > > 3) unbind the driver
> > >
> > > 4) mkdir /sys/kernel/config/xe/0000:03:00.0
> > > echo .... > /sys/kernel/kernel/config/xe/0000:03:00.0/...
> > > 5) bind the driver in survivability mode
> > >
> > >
> > > At step 4, when creating directory. The bdf needs to be validated, ie (it
> > > should be pci_dev that xe will probe). i checked the struct pci_dev but
> > > didn't find anything.
> > > Will have to match the device id against supported pci device ids.
> >
> > I don't believe we need to run any validation. If admin creates a file/directory
> > with invalid address it will be just ignored, no?!
> >
> > On the probe we only get the directory, file for the pci device we are probing.
> >
> > >
> > > Otherwise, there might be wrong non functional bdf directories present based
> > > on what user provides.
> > >
> > > Thanks
> > > Riana
> > > >
> > > > You shouldn't create any directory when unbinding. It's the user who
> > > > should create it. When **binding** the driver we should check what are
> > > > the extra configuration available and adapt the probe according to that.
> > > >
> > > > For that we have to disable pci driver autoprobe and tweak the
> > > > configfs settings:
> > > >
> > > > 1) echo 0 > /sys/bus/pci/drivers_autoprobe
> > > > modprobe xe
> > > >
> > > > 2) mkdir /sys/kernel/config/xe/0000:03:00.0
> > > > echo .... > /sys/kernel/kernel/config/xe/0000:03:00.0/...
> > > > echo "0000:03:00.0" > /sys/bus/pci/drivers/xe/bind
> > > >
> > > >
> > > > Lucas De Marchi
> > > >
> > > > >
> > > > > The attr is WO and any value can be written, only if the correct bdf
> > > > > is added in the attr then it'll be used on probe to enter
> > > > > survivability mode.The current patch only checks the format and does
> > > > > not check if bdf belongs to xe.
> > > > > >
> > > > > > > + */
> > > > > > > +
> > > > > > > +void xe_configfs_clear_survivability_mode(void)
> > > > > > > +{
> > > > > > > + kfree(xe_modparam.survivability_mode);
> > > > > > > + xe_modparam.survivability_mode = NULL;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static ssize_t survivability_mode_store(struct config_item
> > > > > > > *item, const char *page, size_t len)
> > > > > >
> > > > > > once you handle the mkdir mentioned above, this should probably be
> > > > > > a boolean attr like this:
> > > > > >
> > > > > > drivers/nvme/target/configfs.c:CONFIGFS_ATTR(nvmet_,
> > > > > > param_pi_enable);
> > > > > >
> > > > > > > +{
> > > > > > > + char *survivability_mode;
> > > > > > > + int ret;
> > > > > > > + unsigned int domain, bus, slot, function;
> > > > > > > +
> > > > > > > + ret = sscanf(page, "%04x:%02x:%02x.%x", &domain, &bus,
> > > > > > > &slot, &function);
> > > > > > > + if (ret != 4)
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > + survivability_mode = kstrdup(page, GFP_KERNEL);
> > > > > > > + if (!survivability_mode)
> > > > > > > + return -ENOMEM;
> > > > > > > +
> > > > > > > + xe_configfs_clear_survivability_mode();
> > > > > > > + xe_modparam.survivability_mode = survivability_mode;
> > > > > > > +
> > > > > > > + return len;
> > > > > > > +}
> > > > > > > +
> > > > > > > +CONFIGFS_ATTR_WO(, survivability_mode);
> > > > > > > +
> > > > > > > +static struct configfs_attribute *xe_configfs_attrs[] = {
> > > > > > > + &attr_survivability_mode,
> > > > > > > + NULL,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct config_item_type xe_config_type = {
> > > > > > > + .ct_attrs = xe_configfs_attrs,
> > > > > > > + .ct_owner = THIS_MODULE,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct configfs_subsystem xe_config_subsys = {
> > > > > > > + .su_group = {
> > > > > > > + .cg_item = {
> > > > > > > + .ci_namebuf = "xe",
> > > > > > > + .ci_type = &xe_config_type,
> > > > > > > + },
> > > > > > > + },
> > > > > > > +};
> > > > > > >
> > > > > >
> > > > > > so... it seems you are missing a configfs_group_operations.make_group.
> > > > > >
> > > > > > > +int __init xe_configfs_init(void)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + config_group_init(&xe_config_subsys.su_group);
> > > > > > > + mutex_init(&xe_config_subsys.su_mutex);
> > > > > >
> > > > > > this mutex_init seems odd, but inline with other drivers
> > > > > yeah it is not used anywhere but the sample and other drivers also
> > > > > initialize it
> > > > > >
> > > > > > > + ret = configfs_register_subsystem(&xe_config_subsys);
> > > > > > > + if (ret) {
> > > > > > > + pr_err("Error %d while registering subsystem %s\n",
> > > > > > > + ret, xe_config_subsys.su_group.cg_item.ci_namebuf);
> > > > > > > + mutex_destroy(&xe_config_subsys.su_mutex);
> > > > > > > + return ret;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +void __exit xe_configfs_exit(void)
> > > > > > > +{
> > > > > > > + xe_configfs_clear_survivability_mode();
> > > > > > > + configfs_unregister_subsystem(&xe_config_subsys);
> > > > > > > + mutex_destroy(&xe_config_subsys.su_mutex);
> > > > > > > +}
> > > > > > > +
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_configfs.h
> > > > > > > b/drivers/gpu/drm/xe/ xe_configfs.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..491629a2ca53
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_configfs.h
> > > > > > > @@ -0,0 +1,12 @@
> > > > > > > +/* SPDX-License-Identifier: MIT */
> > > > > > > +/*
> > > > > > > + * Copyright © 2025 Intel Corporation
> > > > > > > + */
> > > > > > > +#ifndef _XE_CONFIGFS_H_
> > > > > > > +#define _XE_CONFIGFS_H_
> > > > > > > +
> > > > > > > +int xe_configfs_init(void);
> > > > > > > +void xe_configfs_exit(void);
> > > > > > > +void xe_configfs_clear_survivability_mode(void);
> > > > > > > +
> > > > > > > +#endif
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > > > > > > b/drivers/gpu/drm/xe/ xe_module.c
> > > > > > > index 475acdba2b55..15b3cf22193c 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_module.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_module.c
> > > > > > > @@ -11,6 +11,7 @@
> > > > > > > #include <drm/drm_module.h>
> > > > > > >
> > > > > > > #include "xe_drv.h"
> > > > > > > +#include "xe_configfs.h"
> > > > > > > #include "xe_hw_fence.h"
> > > > > > > #include "xe_pci.h"
> > > > > > > #include "xe_pm.h"
> > > > > > > @@ -91,6 +92,10 @@ static const struct init_funcs init_funcs[] = {
> > > > > > > {
> > > > > > > .init = xe_check_nomodeset,
> > > > > > > },
> > > > > > > + {
> > > > > > > + .init = xe_configfs_init,
> > > > > > > + .exit = xe_configfs_exit,
> > > > > > > + },
> > > > > > > {
> > > > > > > .init = xe_hw_fence_module_init,
> > > > > > > .exit = xe_hw_fence_module_exit,
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_module.h
> > > > > > > b/drivers/gpu/drm/xe/ xe_module.h
> > > > > > > index 84339e509c80..c238dbee6bc7 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_module.h
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_module.h
> > > > > > > @@ -24,6 +24,7 @@ struct xe_modparam {
> > > > > > > #endif
> > > > > > > int wedged_mode;
> > > > > > > u32 svm_notifier_size;
> > > > > > > + char *survivability_mode;
> > > > > >
> > > > > > drop this.. We want a config struct in the xe_device. It's actually
> > > > > > allocated by the mkdir in the configs and when we are probing, we should
> > > > > > find the config instace based on pdev and set the pointer xe->configfs
> > > > > > or something like that.
> > > > >
> > > > > Will add a new struct
> > > > >
> > > > > Thanks
> > > > > Riana
> > > > > >
> > > > > > thanks
> > > > > > Lucas De Marchi
> > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > extern struct xe_modparam xe_modparam;
> > > > > > > --
> > > > > > > 2.47.1
> > > > > > >
> > > > >
> > >
More information about the Intel-xe
mailing list