Re: [PATCH v3 3/7] drm/i915/huc: Load GSC-enabled HuC via DMA xfer if the fuse says so

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

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@xxxxxxxxx>
> > Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> > Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   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
}

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;
> >   };
> >     /*