Re: [PATCH v4] drm/xe/pf: Allow to lockdown the PF using custom guard

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 11/6/2025 11:30 AM, Manszewski, Christoph wrote:
> Hi Michal,
> 
> On 5.11.2025 14:00, Michal Wajdeczko wrote:
>> Some driver components, like eudebug or ccs-mode, can't be used
>> when VFs are enabled.  Add functions to allow those components
>> to block the PF from enabling VFs for the requested duration.
>>
>> Introduce trivial counter to allow lockdown or exclusive access
>> that can be used in the scenarios where we can't follow the strict
>> owner semantics as required by the rw_semaphore implementation.
>>
>> Before enabling VFs, the PF will try to arm the "vfs_enabling"
>> guard for the exclusive access.  This will fail if there are
>> some lockdown requests already initiated by the other components.
>>
>> For testing purposes, add debugfs file which will call these new
>> functions from the file's open/close hooks.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Christoph Manszewski <christoph.manszewski@intel.com>
>> ---
>> v1: https://patchwork.freedesktop.org/series/156687/#rev1
>> v2: https://patchwork.freedesktop.org/series/156795/#rev1
>>      use less confusing names (Matthew/Christoph)
>>      allow many components to lockdown the PF (Christoph)
>> v3: no atomic_t, just use spinlock (Matthew)
>> v4: add missing local change (Michal)
>> ---
>> Cc: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>
>> Cc: Maciej Patelczyk <maciej.patelczyk@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guard.h            | 119 +++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_pci_sriov.c        |  21 ++++
>>   drivers/gpu/drm/xe/xe_sriov_pf.c         |  98 +++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_sriov_pf.h         |   4 +
>>   drivers/gpu/drm/xe/xe_sriov_pf_debugfs.c |  30 ++++++
>>   drivers/gpu/drm/xe/xe_sriov_pf_helpers.h |   5 +
>>   drivers/gpu/drm/xe/xe_sriov_pf_types.h   |   4 +
>>   7 files changed, 281 insertions(+)
>>   create mode 100644 drivers/gpu/drm/xe/xe_guard.h
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guard.h b/drivers/gpu/drm/xe/xe_guard.h
>> new file mode 100644
>> index 000000000000..333f8e13b5a1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_guard.h
>> @@ -0,0 +1,119 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GUARD_H_
>> +#define _XE_GUARD_H_
>> +
>> +#include <linux/spinlock.h>
>> +
>> +/**
>> + * struct xe_guard - Simple logic to protect a feature.
>> + *
>> + * Implements simple semaphore-like logic that can be used to lockdown the
>> + * feature unless it is already in use.  Allows enabling of the otherwise
> 
> Nit: double space before 'Allows'

it was on purpose, like usually done (in many places that use plain text) to emphasize start of the new sentence
(but I don't care)

> 
> 
>> + * incompatible features, where we can't follow the strict owner semantics
>> + * required by the &rw_semaphore.
>> + *
>> + * NOTE! It shouldn't be used to protect a data, use &rw_semaphore instead.
>> + */
>> +struct xe_guard {
>> +    /**
>> +     * @counter: implements simple exclusive/lockdown logic:
>> +     *           if == 0 then guard/feature is idle/not in use,
>> +     *           if < 0 then feature is active and can't be locked-down,
>> +     *           if > 0 then feature is lockded-down and can't be activated.
>> +     */
>> +    int counter;
>> +
>> +    /** @name: the name of the guard (useful for debug) */
>> +    const char *name;
>> +
>> +    /** @owner: the info about the last owner of the guard (for debug) */
>> +    void *owner;
>> +
>> +    /** @lock: protects guard's data */
>> +    spinlock_t lock;
>> +};
>> +
>> +/**
>> + * xe_guard_init() - Initialize the guard.
>> + * @guard: the &xe_guard to init
>> + * @name: name of the guard
>> + */
>> +static inline void xe_guard_init(struct xe_guard *guard, const char *name)
>> +{
>> +    spin_lock_init(&guard->lock);
>> +    guard->counter = 0;
>> +    guard->name = name;
>> +}
>> +
>> +/**
>> + * xe_guard_arm() - Arm the guard for the exclusive/lockdown mode.
>> + * @guard: the &xe_guard to arm
>> + * @lockdown: arm for lockdown(true) or exclusive(false) mode
>> + * @who: optional owner info (for debug only)
>> + *
>> + * Multiple lockdown requests are allowed.
>> + * Only single exclusive access can be granted.
>> + * Will fail if the guard is already in exclusive mode.
>> + * On success, must call the xe_guard_disarm() to release.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
> 
> I am not sold on the arm/disarm and exclusive/lockdown naming. Something like 'get/put' and 'exclusive/shared' sounds more familiar, but that's just a personal opinion.

actually I was considering "lockdown_get|put" and "exclusive_get|put" but still was afraid of being accused of using misleading tags used elsewhere

also note that "shared" is actually a bad term here, as in our case we are not going to "share" the feature with anyone, there just might be collective requests to "block" the feature

and this is how I ended with the "lockdown" naming

> 
> None the less the solution looks clean and does what it should so:
> 
> Reviewed-by: Christoph Manszewski <christoph.manszewski@intel.com>

thanks, but you have to wait for another rev anyway (see below)

> 
> Regards,
> Christoph
> 

...

>> diff --git a/drivers/gpu/drm/xe/xe_sriov_pf.h b/drivers/gpu/drm/xe/xe_sriov_pf.h
>> index cba3fde9581f..b5bee3a549c1 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_pf.h
>> +++ b/drivers/gpu/drm/xe/xe_sriov_pf.h
>> @@ -17,11 +17,15 @@ bool xe_sriov_pf_readiness(struct xe_device *xe);
>>   int xe_sriov_pf_init_early(struct xe_device *xe);
>>   int xe_sriov_pf_init_late(struct xe_device *xe);
>>   int xe_sriov_pf_wait_ready(struct xe_device *xe);
>> +int xe_sriov_pf_lockdown(struct xe_device *xe);
>> +void xe_sriov_pf_end_lockdown(struct xe_device *xe);
>>   void xe_sriov_pf_print_vfs_summary(struct xe_device *xe, struct drm_printer *p);
>>   #else
>>   static inline bool xe_sriov_pf_readiness(struct xe_device *xe) { return false; }
>>   static inline int xe_sriov_pf_init_early(struct xe_device *xe) { return 0; }
>>   static inline int xe_sriov_pf_init_late(struct xe_device *xe) { return 0; }
>> +int xe_sriov_pf_lockdown(struct xe_device *xe) { return 0; }
>> +void xe_sriov_pf_end_lockdown(struct xe_device *xe) { }

those stubs shall be "static inline"