Message-ID: <5521adaf-724e-4de8-ae56-74dc349b66c8@intel.com>
Date:   Tue, 12 Sep 2023 08:05:19 -0700
From:   Daniele Ceraolo Spurio <daniele.ceraolospurio@...el.com>
To:     Alexander Usyskin <alexander.usyskin@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
CC:     Tomas Winkler <tomas.winkler@...el.com>,
        Vitaly Lubart <vitaly.lubart@...el.com>,
        <intel-xe@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] drm/xe/gsc: add gsc device support



On 9/12/2023 6:08 AM, Alexander Usyskin wrote:
> From: Vitaly Lubart <vitaly.lubart@...el.com>
>
> Create mei-gscfi auxiliary device and configure interrupts
> to be consumed by mei-gsc device driver.
>
> Signed-off-by: Vitaly Lubart <vitaly.lubart@...el.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> ---
>   drivers/gpu/drm/xe/Kconfig       |   1 +
>   drivers/gpu/drm/xe/Makefile      |   1 +
>   drivers/gpu/drm/xe/xe_gt.c       |   4 +
>   drivers/gpu/drm/xe/xe_gt_gsc.c   | 218 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_gt_gsc.h   |  36 +++++
>   drivers/gpu/drm/xe/xe_gt_types.h |   4 +
>   drivers/gpu/drm/xe/xe_irq.c      |  11 +-
>   7 files changed, 273 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/gpu/drm/xe/xe_gt_gsc.c
>   create mode 100644 drivers/gpu/drm/xe/xe_gt_gsc.h
>
> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> index 0a4ea965645b..9d396ca7708a 100644
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -36,6 +36,7 @@ config DRM_XE
>   	select DRM_SCHED
>   	select MMU_NOTIFIER
>   	select WANT_DEV_COREDUMP
> +	select AUXILIARY_BUS
>   	help
>   	  Experimental driver for Intel Xe series GPUs
>   
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 9a2245ffede2..9ba9389a2cfd 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -65,6 +65,7 @@ xe-y += xe_bb.o \
>   	xe_gt_sysfs.o \
>   	xe_gt_tlb_invalidation.o \
>   	xe_gt_topology.o \
> +	xe_gt_gsc.o \

I don't think this should be inside GT. The HECI GSC is not inside GT in 
HW and the only reason it was added there in i915 was because it made 
the interrupt management easier. This is no longer the case for Xe, so 
this can be added directly under the xe_device and match the HW.

>   	xe_guc.o \
>   	xe_guc_ads.o \
>   	xe_guc_ct.o \
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 4c7f66647a04..f373c139e1a1 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -76,6 +76,8 @@ static void gt_fini(struct drm_device *drm, void *arg)
>   
>   	destroy_workqueue(gt->ordered_wq);
>   
> +	xe_gsc_fini(&gt->gsc);

I'm not sure this is safe. gt_fini is called from the drmm cleanup, 
which means it is called after the Xe driver has already been detached 
from the PCI device. That seems too late to destroy the child device, 
IMO that should be removed before we detach from the PCI device.

> +
>   	for (i = 0; i < XE_ENGINE_CLASS_MAX; ++i)
>   		xe_hw_fence_irq_finish(&gt->fence_irq[i]);
>   }
> @@ -460,6 +462,8 @@ int xe_gt_init(struct xe_gt *gt)
>   	if (err)
>   		return err;
>   
> +	xe_gsc_init(&gt->gsc, gt_to_xe(gt));
> +
>   	err = drmm_add_action_or_reset(&gt_to_xe(gt)->drm, gt_fini, gt);
>   	if (err)
>   		return err;
> diff --git a/drivers/gpu/drm/xe/xe_gt_gsc.c b/drivers/gpu/drm/xe/xe_gt_gsc.c
> new file mode 100644
> index 000000000000..ed825c883da7
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_gsc.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2023, Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/mei_aux.h>
> +#include <linux/pci.h>
> +#include <linux/sizes.h>
> +
> +#include "regs/xe_regs.h"
> +#include "xe_drv.h"
> +#include "xe_gt_gsc.h"
> +#include "xe_gt_types.h"
> +#include "xe_device_types.h"
> +#include "xe_platform_types.h"
> +
> +#define GSC_BAR_LENGTH  0x00000FFC
> +
> +static void gsc_irq_mask(struct irq_data *d)
> +{
> +	/* generic irq handling */
> +}
> +
> +static void gsc_irq_unmask(struct irq_data *d)
> +{
> +	/* generic irq handling */
> +}
> +
> +static struct irq_chip gsc_irq_chip = {
> +	.name = "gsc_irq_chip",
> +	.irq_mask = gsc_irq_mask,
> +	.irq_unmask = gsc_irq_unmask,
> +};
> +
> +static inline struct xe_gt *gsc_to_gt(struct xe_gsc *gsc)
> +{
> +	return container_of(gsc, struct xe_gt, gsc);
> +}
> +
> +static int gsc_irq_init(int irq)
> +{
> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> +				      handle_simple_irq, "gsc_irq_handler");
> +
> +	return irq_set_chip_data(irq, NULL);
> +}
> +
> +/**
> + * struct gsc_def - graphics security controller heci interface definistions
> + *
> + * @name: name of the heci device
> + * @bar: address of the mmio bar
> + * @bar_size: size of the mmio bar
> + * @use_polling: indication of using polling mode for the device
> + * @slow_firmware: indication of whether the device is slow (needs longer timeouts)
> + */
> +struct gsc_def {
> +	const char *name;
> +	unsigned long bar;
> +	size_t bar_size;
> +	bool use_polling;
> +	bool slow_firmware;
> +};
> +
> +/* gsc resources and definitions */
> +static const struct gsc_def gsc_def_dg1 = {
> +	.name = "mei-gscfi",
> +	.bar = DG1_GSC_HECI2_BASE,
> +	.bar_size = GSC_BAR_LENGTH,
> +};
> +
> +static const struct gsc_def gsc_def_dg2 = {
> +	.name = "mei-gscfi",
> +	.bar = DG2_GSC_HECI2_BASE,
> +	.bar_size = GSC_BAR_LENGTH,
> +};
> +
> +static void gsc_release_dev(struct device *dev)
> +{
> +	struct auxiliary_device *aux_dev = to_auxiliary_dev(dev);
> +	struct mei_aux_device *adev = auxiliary_dev_to_mei_aux_dev(aux_dev);
> +
> +	kfree(adev);
> +}
> +
> +void xe_gsc_fini(struct xe_gsc *gsc)
> +{
> +	struct xe_gt *gt = gsc_to_gt(gsc);
> +
> +	if (!HAS_HECI_GSCFI(gt_to_xe(gt)))
> +		return;
> +
> +	if (gsc->adev) {
> +		struct auxiliary_device *aux_dev = &gsc->adev->aux_dev;
> +
> +		auxiliary_device_delete(aux_dev);
> +		auxiliary_device_uninit(aux_dev);
> +		gsc->adev = NULL;
> +	}
> +
> +	if (gsc->irq >= 0)
> +		irq_free_desc(gsc->irq);
> +	gsc->irq = -1;
> +}
> +
> +void xe_gsc_init(struct xe_gsc *gsc, struct xe_device *xe)
> +{
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	struct mei_aux_device *adev;
> +	struct auxiliary_device *aux_dev;
> +	const struct gsc_def *def;
> +	int ret;
> +
> +	if (!HAS_HECI_GSCFI(xe))
> +		return;
> +
> +	gsc->irq = -1;
> +
> +	/*
> +	 * On the multi-tile setups the GSC is functional on the first tile only
> +	 */
> +	if (gsc_to_gt(gsc)->info.id != 0) {
> +		drm_dbg(&xe->drm, "Not initializing gsc for remote tiles\n");
> +		return;
> +	}
> +
> +	if (xe->info.platform == XE_DG1) {
> +		def = &gsc_def_dg1;
> +	} else if (xe->info.platform == XE_DG2) {
> +		def = &gsc_def_dg2;
> +	} else {
> +		drm_warn_once(&xe->drm, "Unknown platform\n");
> +		return;
> +	}
> +
> +	if (!def->name) {
> +		drm_warn_once(&xe->drm, "HECI is not implemented!\n");
> +		return;
> +	}
> +
> +	/* skip irq initialization */
> +	if (def->use_polling)
> +		goto add_device;
> +
> +	gsc->irq = irq_alloc_desc(0);
> +	if (gsc->irq < 0) {
> +		drm_err(&xe->drm, "gsc irq error %d\n", gsc->irq);
> +		goto fail;
> +	}
> +
> +	ret = gsc_irq_init(gsc->irq);
> +	if (ret < 0) {
> +		drm_err(&xe->drm, "gsc irq init failed %d\n", ret);
> +		goto fail;
> +	}
> +
> +add_device:
> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +	if (!adev)
> +		goto fail;
> +	adev->irq = gsc->irq;
> +	adev->bar.parent = &pdev->resource[0];
> +	adev->bar.start = def->bar + pdev->resource[0].start;
> +	adev->bar.end = adev->bar.start + def->bar_size - 1;
> +	adev->bar.flags = IORESOURCE_MEM;
> +	adev->bar.desc = IORES_DESC_NONE;
> +	adev->slow_firmware = def->slow_firmware;
> +
> +	aux_dev = &adev->aux_dev;
> +	aux_dev->name = def->name;
> +	aux_dev->id = (pci_domain_nr(pdev->bus) << 16) |
> +		      PCI_DEVID(pdev->bus->number, pdev->devfn);
> +	aux_dev->dev.parent = &pdev->dev;
> +	aux_dev->dev.release = gsc_release_dev;
> +
> +	ret = auxiliary_device_init(aux_dev);
> +	if (ret < 0) {
> +		drm_err(&xe->drm, "gsc aux init failed %d\n", ret);
> +		kfree(adev);
> +		goto fail;
> +	}
> +
> +	gsc->adev = adev; /* needed by the notifier */
> +	ret = auxiliary_device_add(aux_dev);
> +	if (ret < 0) {
> +		drm_err(&xe->drm, "gsc aux add failed %d\n", ret);
> +		gsc->adev = NULL;
> +
> +		/* adev will be freed with the put_device() and .release sequence */
> +		auxiliary_device_uninit(aux_dev);
> +		goto fail;
> +	}
> +
> +	return;
> +fail:
> +	xe_gsc_fini(gsc);
> +}
> +
> +void xe_gsc_irq_handler(struct xe_gt *gt, u32 iir)
> +{
> +	int ret;
> +
> +	if ((iir & GSC_IRQ_INTF(1)) == 0)
> +		return;
> +
> +	if (!HAS_HECI_GSCFI(gt_to_xe(gt))) {
> +		drm_warn_once(&gt_to_xe(gt)->drm, "GSC irq: not supported");
> +		return;
> +	}
> +
> +	if (gt->gsc.irq < 0)
> +		return;
> +
> +	ret = generic_handle_irq(gt->gsc.irq);
> +	if (ret)
> +		drm_err_ratelimited(&gt_to_xe(gt)->drm, "error handling GSC irq: %d\n", ret);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_gt_gsc.h b/drivers/gpu/drm/xe/xe_gt_gsc.h
> new file mode 100644
> index 000000000000..91dba19e47c1
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_gsc.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2023, Intel Corporation. All rights reserved.
> + */
> +#ifndef __XE_GSC_DEV_H__
> +#define __XE_GSC_DEV_H__
> +
> +#include <linux/types.h>
> +
> +struct xe_device;
> +struct xe_gt;
> +struct mei_aux_device;
> +
> +/*
> + * The HECI1 bit corresponds to bit15 and HECI2 to bit14.
> + * The reason for this is to allow growth for more interfaces in the future.
> + */
> +#define GSC_IRQ_INTF(_x)  BIT(15 - (_x))
> +
> +/**
> + * struct xe_gsc - graphics security controller for xe
> + *
> + * @adev : pointer to mei auxiliary device structure
> + * @irq : irq number
> + *
> + */
> +struct xe_gsc {

Please use a different name for this instead of just xe_gsc. In Xe we're 
likely never going to fully use the GSC via HECI, only the GSCFI/CSC 
part. In MTL and newer we also have the actual GSC part being split off 
and placed inside GT (behind the GSCCS), so if we call this just xe_gsc 
as well it'll be confusing. I suggest calling this something like 
xe_heci_gsc, xe_heci_interface or something like that. I had actually 
suggested this for i915 as well 
(https://patchwork.freedesktop.org/patch/509653/) but Tomas was 
concerned it might make backporting fixes difficult, so I dropped it; 
this is not a concern for Xe right now.

Note that this means renaming all the exposed functions as well.

Daniele

> +	struct mei_aux_device *adev;
> +	int irq;
> +};
> +
> +void xe_gsc_init(struct xe_gsc *gsc, struct xe_device *xe);
> +void xe_gsc_fini(struct xe_gsc *gsc);
> +void xe_gsc_irq_handler(struct xe_gt *gt, u32 iir);
> +
> +#endif /* __XE_GSC_DEV_H__ */
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index f11da12d3146..1b8ab32b488d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -13,6 +13,7 @@
>   #include "xe_reg_sr_types.h"
>   #include "xe_sa_types.h"
>   #include "xe_uc_types.h"
> +#include "xe_gt_gsc.h"
>   
>   struct xe_exec_queue_ops;
>   struct xe_migrate;
> @@ -267,6 +268,9 @@ struct xe_gt {
>   	/** @uc: micro controllers on the GT */
>   	struct xe_uc uc;
>   
> +	/** @gsc: graphics security controller */
> +	struct xe_gsc gsc;
> +
>   	/** @gtidle: idle properties of GT */
>   	struct xe_gt_idle gtidle;
>   
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 1dee3e832eb5..1ae4cc9f1522 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -128,6 +128,7 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>   	struct xe_device *xe = gt_to_xe(gt);
>   	u32 ccs_mask, bcs_mask;
>   	u32 irqs, dmask, smask;
> +	u32 gsc_mask = GSC_IRQ_INTF(1);
>   
>   	if (xe_device_guc_submission_enabled(xe)) {
>   		irqs = GT_RENDER_USER_INTERRUPT |
> @@ -180,6 +181,9 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>   		if (xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_OTHER)) {
>   			xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE, irqs);
>   			xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK, ~irqs);
> +		} else if (HAS_HECI_GSCFI(xe)) {
> +			xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE, gsc_mask);
> +			xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK, ~gsc_mask);
>   		}
>   	}
>   }
> @@ -227,6 +231,8 @@ gt_other_irq_handler(struct xe_gt *gt, const u8 instance, const u16 iir)
>   		return xe_guc_irq_handler(&gt->uc.guc, iir);
>   	if (instance == OTHER_MEDIA_GUC_INSTANCE && xe_gt_is_media_type(gt))
>   		return xe_guc_irq_handler(&gt->uc.guc, iir);
> +	if (instance == OTHER_GSC_INSTANCE)
> +		return xe_gsc_irq_handler(gt, iir);
>   
>   	if (instance != OTHER_GUC_INSTANCE &&
>   	    instance != OTHER_MEDIA_GUC_INSTANCE) {
> @@ -470,8 +476,9 @@ static void gt_irq_reset(struct xe_tile *tile)
>   	if (ccs_mask & (BIT(2)|BIT(3)))
>   		xe_mmio_write32(mmio,  CCS2_CCS3_INTR_MASK, ~0);
>   
> -	if (tile->media_gt &&
> -	    xe_hw_engine_mask_per_class(tile->media_gt, XE_ENGINE_CLASS_OTHER)) {
> +	if ((tile->media_gt &&
> +	     xe_hw_engine_mask_per_class(tile->media_gt, XE_ENGINE_CLASS_OTHER)) ||
> +	    HAS_HECI_GSCFI(tile_to_xe(tile))) {
>   		xe_mmio_write32(mmio, GUNIT_GSC_INTR_ENABLE, 0);
>   		xe_mmio_write32(mmio, GUNIT_GSC_INTR_MASK, ~0);
>   	}

Powered by blists - more mailing lists