Re: [PATCH 9/9] drm/xe/vf: Custom HuC initialization if VF

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jun 20, 2024 at 01:34:43AM +0200, Michal Wajdeczko wrote:
> 
> 
> On 20.06.2024 01:23, Matthew Brost wrote:
> > On Thu, Jun 20, 2024 at 01:14:29AM +0200, Michal Wajdeczko wrote:
> >>
> >>
> >> On 20.06.2024 01:11, Matthew Brost wrote:
> >>> On Wed, Jun 19, 2024 at 11:45:57PM +0200, Michal Wajdeczko wrote:
> >>>> The HuC firmware is loaded and initialized by the PF driver. Make
> >>>> sure VF driver performs only limited data structure initialization.
> >>>>
> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/xe/xe_huc.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c
> >>>> index 6238fb354914..c88761fe31c9 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_huc.c
> >>>> +++ b/drivers/gpu/drm/xe/xe_huc.c
> >>>> @@ -21,6 +21,7 @@
> >>>>  #include "xe_guc.h"
> >>>>  #include "xe_map.h"
> >>>>  #include "xe_mmio.h"
> >>>> +#include "xe_sriov.h"
> >>>>  #include "xe_uc_fw.h"
> >>>>  
> >>>>  static struct xe_gt *
> >>>> @@ -92,6 +93,9 @@ int xe_huc_init(struct xe_huc *huc)
> >>>>  	if (!xe_uc_fw_is_enabled(&huc->fw))
> >>>>  		return 0;
> >>>>  
> >>>> +	if (IS_SRIOV_VF(xe))
> >>>> +		return 0;
> >>>> +
> >>>
> >>> With this change I assume the main part of xe_huc_auth is never called
> >>> on a VF?
> >>>
> >>> Does xe_uc_fw_is_loadable return false on a VF?
> >>
> >> yes, as on VF it is marked as PRELOADED, so:
> >>
> >> static inline bool xe_uc_fw_is_loadable(struct xe_uc_fw *uc_fw)
> >> {
> >> 	return __xe_uc_fw_status(uc_fw) >= XE_UC_FIRMWARE_LOADABLE &&
> >> 		__xe_uc_fw_status(uc_fw) != XE_UC_FIRMWARE_PRELOADED;
> >> }
> >>
> >> returns false
> >>
> > 
> > To be clear, I'd add asserts to parts of functions which should not be
> > executed on a VF if it is expected to short circuit on another function.
> > 
> > e.g.
> > 
> > xe_huc_auth()
> > 	if (!xe_uc_fw_is_loadable)
> > 		return
> > 
> > 	xe_assert(!VF);
> 
> hmm, but then we might pollute the whole driver with such asserts, as VF
> really can do only limited stuff
> 
> besides, we already track any unwanted access to unavailable registers
> from VFs in xe_mmio_read32()
> 
> 	if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)))
> 		val = xe_gt_sriov_vf_read32(gt, reg);
> 
> ...
> 
> xe_gt_WARN(gt, IS_ENABLED(CONFIG_DRM_XE_DEBUG),
> 	   "VF is trying to read an inaccessible register %#x+%#x\n",
> 	   reg.addr, addr - reg.addr);
> 
> and I'm planning to add similar code (or xe_assert) to xe_mmio_write()
> 
> finally, GuC will report errors if VF will try to execute privileged action
> 
> so IMO spreading xe_assert(!VF) all over is not ideal
> 

As long as some warning pops, I agree we are likely fine then.

Matt

> > 
> > This patch LGTM to me though. With that:
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > 
> >>>
> >>> Matt
> >>>
> >>>>  	if (huc->fw.has_gsc_headers) {
> >>>>  		ret = huc_alloc_gsc_pkt(huc);
> >>>>  		if (ret)
> >>>> -- 
> >>>> 2.43.0
> >>>>