Re: FW: [RFC PATCH 2/5] drm/xe/xe_ras: Handle page offline requests for device memory ecc errors

["Tauro, Riana" <riana.tauro@intel.com>]

On 07-09-2026 17:53, Michal Wajdeczko wrote:
>
> On 9/7/2026 8:31 AM, Tauro, Riana wrote:
>>> On 8/25/2026 8:36 AM, Riana Tauro wrote:
>>>> This will be integrated with the related address-fault handling flow
>>>> once this patch is merged.
>>>> https://lore.kernel.org/intel-xe/20260818104055.3833974-14-tejas.upadhyay@intel.com/
>>>> Sending for initial comments.
>>>>
>>>> Add basic support for sending page offline/decline requests to system
>>>> controller and use it for device memory ECC error handling.
>>>> Pages that belong to critical BOs cannot be handled by offlining and
>>>> require a SBR (Secondary Bus Reset).
>>>> Pages that are configured for log-only handling are not marked as bad by
>>>> firmware.
>>>>
>>>> For all other valid page addresses, the first occurrence of error
>>>> indicates a poison error and the page is offlined only by software.
>>>> Firmware avoids permanently marking the page as bad. The second occurrence
>>>> of an error indicates a Double-bit ECC error and the firmware
>>>> permanently marks the page as bad.
>>>>
>>>> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com>
>>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_ras.c                   | 121 +++++++++++++++++-
>>>>    drivers/gpu/drm/xe/xe_ras_types.h             |  35 +++++
>>>>    drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h |   2 +
>>>>    3 files changed, 153 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
>>>> index d25d25f77531..c643c7137a42 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ras.c
>>>> +++ b/drivers/gpu/drm/xe/xe_ras.c
>>>> @@ -3,6 +3,7 @@
>>>>     * Copyright © 2026 Intel Corporation
>>>>     */
>>>>    +#include "xe_bo.h"
>>>>    #include "xe_debugfs.h"
>>>>    #include "xe_device.h"
>>>>    #include "xe_drm_ras.h"
>>>> @@ -200,6 +201,115 @@ static inline const char *comp_to_str(u8 component)
>>>>        return xe_ras_components[component];
>>>>    }
>>>>    +static int send_page_offline_cmd(struct xe_device *xe, u64 page_address,
>>>> +                 enum xe_ras_page_action action)
>>>> +{
>>>> +    struct xe_sysctrl_mailbox_command command = {0};
>>>> +    struct xe_ras_page_offline_request request = {0};
>>>> +    struct xe_ras_page_offline_response response = {0};
>>>> +    size_t rlen;
>>>> +    int ret;
>>>> +
>>>> +    if (!xe->info.has_sysctrl)
>>>> +        return 0;
>>>> +
>>>> +    if (action >= XE_RAS_PAGE_ACTION_MAX) {
>>>> +        xe_log_err(xe, DEVICE_MEMORY, -EINVAL, "Invalid page offline action %d\n", action);
>>>> +        return -EINVAL;
>>> this looks like our programming mistake, shouldn't we use xe_assert() instead?
>> Sure will change it to assert instead of sigid
>>
>>>> +    }
>>>> +
>>>> +    request.page_address = page_address;
>>>> +    request.action = action;
>>>> +
>>>> +    xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, XE_SYSCTRL_CMD_PAGE_OFFLINE,
>>>> +                  &request, sizeof(request), &response, sizeof(response));
>>>> +
>>>> +    ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
>>>> +    if (ret) {
>>>> +        xe_log_err_fatal(xe, SYSCTRL, ret, "failed to send page offline command\n");
>>> what about moving xe_log to the xe_sysctrl_send_command() and use:
>>>
>>>      "Failed to send command %u.%u (%s %s)\n"
>>>          group_id, cmd_id,
>>>          group_id_str(group_id), cmd_id_str(cmd_id)
>> This can be taken as a separate patch if required.  This is currently consistent with rest of the file
>>
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    if (rlen != sizeof(response)) {
>>>> +        xe_log_err(xe, SYSCTRL, -EINVAL,
>>> -EPROTO ?
>> This is consistent with rest of the file.
> but there are already other series in flight where we are trying
> to fix returned errors, so why not doing that here right from the
> beginning?

Can you please point me to the series. Since response is invalid, imo 
the return code
seems right.

>
>>>> +               "unexpected page offline response length %zu (expected %zu)\n",
>>>> +               rlen, sizeof(response));
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    ret = ras_status_to_errno(response.status);
>>>> +    if (ret) {
>>>> +        xe_log_err(xe, SYSCTRL, ret, "page offline command failed with status %u\n",
>>>> +               response.status);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int handle_page_offline(struct xe_device *xe, u64 page_address, bool send_cmd)
>>>> +{
>>>> +    enum xe_ras_page_action action;
>>>> +    int ret = 0;
>>>> +
>>>> +    if (!IS_ALIGNED(page_address, XE_PAGE_SIZE)) {
>>> hmm, can FW really send us such a broken address?
>> We cannot guarantee. Its better to have a check
>>
>>>> +        xe_log_err(xe, SYSCTRL, -EINVAL, "Unaligned physical page address: 0x%llx\n",
>>>> +               page_address);
>>> shouldn't we try to log/print other details from the notification?
>> like?
> "Page offline notification for unaligned address: %#x\n"

Sure will fix in v3.


>
>>>> +        return -EINVAL;
> btw, shouldn't we try to fix that address and move on with
> attempt to offline something close to the reported bad page?
>
> or if we think it is very unusual for FW to report that bad
> address, maybe we should escalate to reset ?

The firmware spec says the address will be 4k aligned. This is a
defensive check to log if we see a mismatch.

IMO reset is not necessary, as these are non-fatal errors.

>
> just logging info about bad address seems not enough IMO
>
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * TODO: Call function to handle address fault
>>>> +     * ret = xe_ttm_vram_handle_addr_fault(xe, page_address);
>>>> +     */
>>>> +
>>>> +    /*
>>>> +     * Handle return code from address fault handling function:
>>>> +     *  0: Page soft offlined, decline to firmware
>>>> +     * -EIO: Address belongs to a critical BO/stolen area that cannot be offlined
>>> maybe:
>>>
>>> #define    EADDRINUSE    98    /* Address already in use */
>>>
>>>> +     * -EOPNOTSUPP: Address is valid and can be offlined but user policy is not to offline
>>> #define    EPERM         1    /* Operation not permitted */
>>>
>>>> +     * -EXIST: Address is soft offlined but yet to be offlined by firmware for second occurrence
>>> #define    EUCLEAN        117    /* Structure needs cleaning */
>> These return codes are from https://lore.kernel.org/intel-xe/20260818104055.3833974-14-tejas.upadhyay@intel.com/.
>> Any change will have to be made there as this is dependent on the above patch.
> hmm, as there are strict expectations for each scenario, maybe handle_fault()
> should return one of the custom predefined enum instead of generic int/errno?

That patch is already merged. Any new changes will have to be a separate 
patch series.
++@tejas

Thanks
Riana

>
> see enum irqreturn as example
>
>>>> +     */
>>>> +
>>>> +    switch (ret) {
>>>> +    case 0:
>>>> +        action = XE_RAS_PAGE_ACTION_DECLINE;
>>>> +        xe_log_err(xe, DEVICE_MEMORY, 0,
>>>> +               "Poison detected at physical address 0x%llx, page software offlined\n",
>>>> +               page_address);
>>>> +        break;
>>>> +    /* User policy set to decline page offlining */
>>>> +    case -EOPNOTSUPP:
>>>> +        action = XE_RAS_PAGE_ACTION_DECLINE;
>>>> +        break;
>>>> +    case -EIO:
>>>> +        xe_log_err(xe, DEVICE_MEMORY, -EIO,
>>>> +               "Physical page address belongs to critical BO: 0x%llx\n", page_address);
>>>> +        return ret;
>>>> +    case -EEXIST:
>>>> +        action = XE_RAS_PAGE_ACTION_OFFLINE;
>>>> +        xe_log_err(xe, DEVICE_MEMORY, -EEXIST,
>>>> +               "Double-bit ECC error detected at physical address 0x%llx, page already software offlined\n",
>>>> +               page_address);
>>>> +        break;
>>>> +    default:
>>>> +        xe_log_err_fatal(xe, DEVICE_MEMORY, ret, "Failed to handle address fault 0x%llx\n",
>>>> +                 page_address);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (send_cmd) {
>>>> +        ret = send_page_offline_cmd(xe, page_address, action);
>>>> +        if (ret)
>>>> +            xe_log_err_fatal(xe, SYSCTRL, ret,
>>>> +                     "Failed to offline page for physical address 0x%llx\n",
>>>> +                     page_address);
>>> there are 3x xe_log() in send_page_offline_cmd()
>>> do we need yet another one here?
>> Sure will remove additional log.
>>
>> Thanks
>> Riana
>>
>>
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static bool ras_counter_is_valid(struct xe_device *xe, struct xe_ras_error_class *counter)
>>>>    {
>>>>        u8 severity = counter->common.severity;
>>>> @@ -367,11 +477,12 @@ static u8 handle_soc_internal_errors(struct xe_device *xe, struct xe_ras_error_a
>>>>    static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_array *arr)
>>>>    {
>>>>        struct xe_ras_memory_error *info = (void *)arr->details;
>>>> +    int ret;
>>>>          /*
>>>>         * For memory errors, the recovery action depends on the error category
>>>>         *
>>>> -     * TODO: Double-bit ECC errors: Page offlining
>>>> +     * Double-bit ECC errors: Page offlining
>>>>         * Poison and data parity errors: Log only
>>>>         * For any other memory errors, request a reset as recovery mechanism
>>>>         */
>>>> @@ -383,10 +494,10 @@ static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_
>>>>            xe_info(xe, "[RAS]: Data parity error detected\n");
>>>>            break;
>>>>        case XE_RAS_MEMORY_DB_ECC:
>>>> -        xe_info(xe, "[RAS]: Double-bit ECC error detected at sw address 0x%llx\n",
>>>> -            info->sw_address);
>>>> -        /* TODO: Add page offlining for Double-bit ECC error */
>>>> -        fallthrough;
>>>> +        ret = handle_page_offline(xe, info->sw_address, true);
>>>> +        if (ret)
>>>> +            return XE_RAS_RECOVERY_ACTION_RESET;
>>>> +        break;
>>>>        default:
>>>>            return XE_RAS_RECOVERY_ACTION_RESET;
>>>>        }
>>>> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
>>>> index 99b2466e2062..2fac968879b6 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ras_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
>>>> @@ -17,6 +17,19 @@
>>>>    #define XE_RAS_MEMORY_POISON            BIT(2)
>>>>    #define XE_RAS_MEMORY_DATA_PARITY        BIT(5)
>>>>    +/**
>>>> + * enum xe_ras_page_action - Page offline actions for page offline request
>>>> + *
>>>> + * @XE_RAS_PAGE_ACTION_OFFLINE: Instruct firmware to offline the page
>>>> + * @XE_RAS_PAGE_ACTION_DECLINE: Instruct firmware to remove the page from queue
>>>> + * @XE_RAS_PAGE_ACTION_MAX: Max value
>>>> + */
>>>> +enum xe_ras_page_action {
>>>> +    XE_RAS_PAGE_ACTION_OFFLINE,
>>>> +    XE_RAS_PAGE_ACTION_DECLINE,
>>>> +    XE_RAS_PAGE_ACTION_MAX
>>>> +};
>>>> +
>>>>    /**
>>>>     * enum xe_ras_recovery_action - RAS recovery actions
>>>>     *
>>>> @@ -245,6 +258,28 @@ struct xe_ras_memory_error {
>>>>        u32 reserved2[10];
>>>>    } __packed;
>>>>    +/**
>>>> + * struct xe_ras_page_offline_request - Request for page offline command
>>>> + */
>>>> +struct xe_ras_page_offline_request {
>>>> +    /** @page_address: Page address (4KB aligned) */
>>>> +    u64 page_address;
>>>> +    /** @action: Action to be performed, see &enum xe_ras_page_action */
>>>> +    u32 action;
>>>> +    /** @reserved: Reserved for future use */
>>>> +    u32 reserved;
>>>> +} __packed;
>>> if this is a FW ABI, then please move it to file in abi/ folder
>>>
>>>> +
>>>> +/**
>>>> + * struct xe_ras_page_offline_response - Response from page offline command
>>>> + */
>>>> +struct xe_ras_page_offline_response {
>>>> +    /** @status: Status of the page offline request */
>>>> +    u32 status;
>>>> +    /** @reserved: Reserved for future use */
>>>> +    u32 reserved;
>>>> +} __packed;
>>> ditto
>>>
>>>> +
>>>>    /**
>>>>     * struct xe_ras_get_health_request - Request structure for obtaining gpu health
>>>>     */
>>>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>>>> index d0341538ad05..3363f48da2b7 100644
>>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>>>> @@ -26,6 +26,7 @@ enum xe_sysctrl_group {
>>>>     * @XE_SYSCTRL_CMD_GET_COUNTER: Get error counter value
>>>>     * @XE_SYSCTRL_CMD_CLEAR_COUNTER: Clear error counter value
>>>>     * @XE_SYSCTRL_CMD_GET_PENDING_EVENT: Retrieve pending event
>>>> + * @XE_SYSCTRL_CMD_PAGE_OFFLINE: Instruct firmware to offline/decline a page
>>>>     * @XE_SYSCTRL_CMD_GET_HEALTH: Retrieve gpu health
>>>>     * @XE_SYSCTRL_CMD_SET_HEALTH: Set gpu health
>>>>     */
>>>> @@ -34,6 +35,7 @@ enum xe_sysctrl_gfsp_cmd {
>>>>        XE_SYSCTRL_CMD_GET_COUNTER        = 0x03,
>>>>        XE_SYSCTRL_CMD_CLEAR_COUNTER        = 0x04,
>>>>        XE_SYSCTRL_CMD_GET_PENDING_EVENT    = 0x07,
>>>> +    XE_SYSCTRL_CMD_PAGE_OFFLINE             = 0x08,
>>> ditto
>>>
>>>>        XE_SYSCTRL_CMD_GET_HEALTH        = 0x0B,
>>>>        XE_SYSCTRL_CMD_SET_HEALTH        = 0x0C,
>>>>    };