[PATCH v3 3/7] drm/i915/huc: Load GSC-enabled HuC via DMA xfer if the fuse says so
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Wed May 31 00:26:45 UTC 2023
On 5/30/2023 5:20 PM, John Harrison wrote:
> On 5/30/2023 17:11, Ceraolo Spurio, Daniele wrote:
>> On 5/30/2023 4:51 PM, John Harrison wrote:
>>> On 5/26/2023 17:52, Daniele Ceraolo Spurio wrote:
>>>> In the previous patch we extracted the offset of the legacy-style HuC
>>>> binary located within the GSC-enabled blob, so now we can use that to
>>>> load the HuC via DMA if the fuse is set that way.
>>>> Note that we now need to differentiate between "GSC-enabled binary"
>>>> and
>>>> "loaded by GSC", so the former case has been renamed to "has GSC
>>>> headers"
>>>> for clarity, while the latter is now based on the fuse instead of the
>>>> binary format. This way, all the legacy load paths are automatically
>>>> taken (including the auth by GuC) without having to implement further
>>>> code changes.
>>>>
>>>> v2: s/is_meu_binary/has_gsc_headers/, clearer logs (John)
>>>>
>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>> <daniele.ceraolospurio at intel.com>
>>>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>>>> Cc: John Harrison <John.C.Harrison at Intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29
>>>> ++++++++++++++---------
>>>> drivers/gpu/drm/i915/gt/uc/intel_huc.h | 4 +++-
>>>> drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +-
>>>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 12 +++++-----
>>>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 +-
>>>> 5 files changed, 29 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> index 6d795438b3e4..37c6a8ca5c71 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> @@ -298,31 +298,38 @@ void intel_huc_init_early(struct intel_huc *huc)
>>>> static int check_huc_loading_mode(struct intel_huc *huc)
>>>> {
>>>> struct intel_gt *gt = huc_to_gt(huc);
>>>> - bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc);
>>>> - bool hw_uses_gsc = false;
>>>> + bool gsc_enabled = huc->fw.has_gsc_headers;
>>>> /*
>>>> * The fuse for HuC load via GSC is only valid on platforms
>>>> that have
>>>> * GuC deprivilege.
>>>> */
>>>> if (HAS_GUC_DEPRIVILEGE(gt->i915))
>>>> - hw_uses_gsc = intel_uncore_read(gt->uncore,
>>>> GUC_SHIM_CONTROL2) &
>>>> - GSC_LOADS_HUC;
>>>> + huc->loaded_via_gsc = intel_uncore_read(gt->uncore,
>>>> GUC_SHIM_CONTROL2) &
>>>> + GSC_LOADS_HUC;
>>>> - if (fw_needs_gsc != hw_uses_gsc) {
>>>> - huc_err(huc, "mismatch between FW (%s) and HW (%s) load
>>>> modes\n",
>>>> - HUC_LOAD_MODE_STRING(fw_needs_gsc),
>>>> HUC_LOAD_MODE_STRING(hw_uses_gsc));
>>>> + if (huc->loaded_via_gsc && !gsc_enabled) {
>>>> + huc_err(huc, "HW requires a GSC-enabled blob, but we found
>>>> a legacy one\n");
>>>> return -ENOEXEC;
>>>> }
>>>> - /* make sure we can access the GSC via the mei driver if we
>>>> need it */
>>>> + /*
>>>> + * Newer GSC_enabled blobs contain the old FW structure
>>>> inside. If we
>>>> + * found that, we can use it to load the legacy way.
>>>> + */
>>>> + if (!huc->loaded_via_gsc && gsc_enabled &&
>>>> !huc->fw.dma_start_offset) {
>>>> + huc_err(huc,"HW in DMA mode, but we have an incompatible
>>>> GSC-enabled blob\n");
>>>> + return -ENOEXEC;
>>>> + }
>>> The comment above doesn't seem to match the check. The comment says
>>> 'we can load the old way if possible' whereas the check is more 'can
>>> we load the old way if we need to'.
>>
>> I'll reword it.
>>
>>>
>>>> +
>>>> + /* make sure we can access the GSC if we need it */
>>>> if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) &&
>>>> IS_ENABLED(CONFIG_INTEL_MEI_GSC)) &&
>>>> - fw_needs_gsc) {
>>>> - huc_info(huc, "can't load due to missing MEI modules\n");
>>>> + !HAS_ENGINE(gt, GSC0) && huc->loaded_via_gsc) {
>>> This test still looks wrong when you read it. I think it needs a
>>> more detailed comment. Some kind of explanation that the modules
>>> only apply to one platform and the engine to a different platform.
>>> Or even breaking into two separate tests with an 'if(DG2)' between
>>> them? It certainly confuses me every time I look at it.
>>
>> Is it clearer if I split it like this?
>>
>> /*
>> * if the FW is loaded via GSC and we're not on a platform that
>> * has the GSC CS, we need the mei modules to access the GSC.
>> */
>> if (huc->loaded_via_gsc && !HAS_ENGINE(gt, GSC0)) {
>> if (!IS_ENABLED(MEI_PXP) || !IS_ENABLED(MEI_GSC)
>> // error
>> }
>>
> It is guaranteed that if the GSC engine is present then no modules are
> required? And that if the GSC engine is not present, then the modules
> are all that is required?
>
> What happens if the GSC engine has been fused off? Or disabled by some
> future module override, workaround, etc.?
>
> I thinking it would be both clearer and safer to say 'if (DG2) {check
> DG2 specific stuff} else {check other stuff}'.
ok, will do.
Daniele
>
> John.
>
>
>> Daniele
>>
>>>
>>> John.
>>>
>>>> + huc_info(huc, "can't load due to missing mei modules or
>>>> GSCCS\n");
>>>> return -EIO;
>>>> }
>>>> - huc_dbg(huc, "loaded by GSC = %s\n", str_yes_no(fw_needs_gsc));
>>>> + huc_dbg(huc, "loaded by GSC = %s\n",
>>>> str_yes_no(huc->loaded_via_gsc));
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> index 0789184d81a2..112f0dce4702 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> @@ -39,6 +39,8 @@ struct intel_huc {
>>>> struct notifier_block nb;
>>>> enum intel_huc_delayed_load_status status;
>>>> } delayed_load;
>>>> +
>>>> + bool loaded_via_gsc;
>>>> };
>>>> int intel_huc_sanitize(struct intel_huc *huc);
>>>> @@ -73,7 +75,7 @@ static inline bool intel_huc_is_used(struct
>>>> intel_huc *huc)
>>>> static inline bool intel_huc_is_loaded_by_gsc(const struct
>>>> intel_huc *huc)
>>>> {
>>>> - return huc->fw.loaded_via_gsc;
>>>> + return huc->loaded_via_gsc;
>>>> }
>>>> static inline bool intel_huc_wait_required(struct intel_huc *huc)
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> index 037d2ad4879d..3355dc1e2bc6 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> @@ -50,7 +50,7 @@ int intel_huc_fw_get_binary_info(struct
>>>> intel_uc_fw *huc_fw, const void *data, s
>>>> size_t min_size = sizeof(*header);
>>>> int i;
>>>> - if (!huc_fw->loaded_via_gsc) {
>>>> + if (!huc_fw->has_gsc_headers) {
>>>> huc_err(huc, "Invalid FW type GSC header parsing!\n");
>>>> return -EINVAL;
>>>> }
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>>> index 9ced8dbf1253..b752a7f1ed99 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>>>> @@ -186,7 +186,7 @@ struct __packed uc_fw_blob {
>>>> u8 major;
>>>> u8 minor;
>>>> u8 patch;
>>>> - bool loaded_via_gsc;
>>>> + bool has_gsc_headers;
>>>> };
>>>> #define UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>>>> @@ -197,7 +197,7 @@ struct __packed uc_fw_blob {
>>>> #define UC_FW_BLOB_NEW(major_, minor_, patch_, gsc_, path_) \
>>>> { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>>>> - .legacy = false, .loaded_via_gsc = gsc_ }
>>>> + .legacy = false, .has_gsc_headers = gsc_ }
>>>> #define UC_FW_BLOB_OLD(major_, minor_, patch_, path_) \
>>>> { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>>>> @@ -310,7 +310,7 @@ __uc_fw_auto_select(struct drm_i915_private
>>>> *i915, struct intel_uc_fw *uc_fw)
>>>> uc_fw->file_wanted.ver.major = blob->major;
>>>> uc_fw->file_wanted.ver.minor = blob->minor;
>>>> uc_fw->file_wanted.ver.patch = blob->patch;
>>>> - uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
>>>> + uc_fw->has_gsc_headers = blob->has_gsc_headers;
>>>> found = true;
>>>> break;
>>>> }
>>>> @@ -736,7 +736,7 @@ static int check_fw_header(struct intel_gt *gt,
>>>> if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
>>>> return 0;
>>>> - if (uc_fw->loaded_via_gsc)
>>>> + if (uc_fw->has_gsc_headers)
>>>> err = check_gsc_manifest(gt, fw, uc_fw);
>>>> else
>>>> err = check_ccs_header(gt, fw, uc_fw);
>>>> @@ -998,7 +998,7 @@ static int uc_fw_xfer(struct intel_uc_fw
>>>> *uc_fw, u32 dst_offset, u32 dma_flags)
>>>> intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>>>> /* Set the source address for the uCode */
>>>> - offset = uc_fw->vma_res.start;
>>>> + offset = uc_fw->vma_res.start + uc_fw->dma_start_offset;
>>>> GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
>>>> intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW,
>>>> lower_32_bits(offset));
>>>> intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH,
>>>> upper_32_bits(offset));
>>>> @@ -1237,7 +1237,7 @@ size_t intel_uc_fw_copy_rsa(struct
>>>> intel_uc_fw *uc_fw, void *dst, u32 max_len)
>>>> {
>>>> struct intel_memory_region *mr = uc_fw->obj->mm.region;
>>>> u32 size = min_t(u32, uc_fw->rsa_size, max_len);
>>>> - u32 offset = sizeof(struct uc_css_header) + uc_fw->ucode_size;
>>>> + u32 offset = uc_fw->dma_start_offset + sizeof(struct
>>>> uc_css_header) + uc_fw->ucode_size;
>>>> struct sgt_iter iter;
>>>> size_t count = 0;
>>>> int idx;
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>>> index b3daba9526eb..054f02811971 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>>>> @@ -120,7 +120,7 @@ struct intel_uc_fw {
>>>> u32 dma_start_offset;
>>>> - bool loaded_via_gsc;
>>>> + bool has_gsc_headers;
>>>> };
>>>> /*
>>>
>>
>
More information about the dri-devel
mailing list