| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Feb 2022 09:26:18 +0000
From: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To: "Usyskin, Alexander" <alexander.usyskin@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
"Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Winkler, Tomas" <tomas.winkler@...el.com>,
"Lubart, Vitaly" <vitaly.lubart@...el.com>,
"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
auxiliary device
On 16/02/2022 17:14, Usyskin, Alexander wrote:
>
>
>> -----Original Message-----
>> From: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
>> Sent: Wednesday, February 16, 2022 14:04
>> To: Usyskin, Alexander <alexander.usyskin@...el.com>; Greg Kroah-
>> Hartman <gregkh@...uxfoundation.org>; Jani Nikula
>> <jani.nikula@...ux.intel.com>; Joonas Lahtinen
>> <joonas.lahtinen@...ux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@...el.com>;
>> David Airlie <airlied@...ux.ie>; Daniel Vetter <daniel@...ll.ch>
>> Cc: linux-kernel@...r.kernel.org; Winkler, Tomas
>> <tomas.winkler@...el.com>; Lubart, Vitaly <vitaly.lubart@...el.com>; intel-
>> gfx@...ts.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
>> auxiliary device
>>
>>
>>
>> On 15/02/2022 15:22, Usyskin, Alexander wrote:
>>
>>>>> +{
>>>>> + irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
>>>>> + handle_simple_irq, "gsc_irq_handler");
>>>>> +
>>>>> + return irq_set_chip_data(irq, dev_priv);
>>>>
>>>> I am not familiar with this interrupt scheme - does dev_priv get used at
>>>> all by handle_simple_irq, or anyone, after being set here?
>>
>> What about this? Is dev_priv required or you could pass in NULL just as
>> well?
>>
>
> It is not used, will remove
>
>>>>
>>>>> +}
>>>>> +
>>>>> +struct intel_gsc_def {
>>>>> + const char *name;
>>>>> + const unsigned long bar;
>>>>
>>>> Unusual, why const out of curiosity? And is it "bar" or "base" would be
>>>> more accurate?
>>>>
>>> Some leftover, thanks for spotting this!
>>> It is a base of bar. I prefer bar name here. But not really matter.
>>
>> Is it?
>>
>> + adev->bar.start = def->bar + pdev->resource[0].start;
>>
>> Looks like offset on top of BAR, no?
>>
>
> Offset on top of DG bar; but start of HECI1/2 bar too.
Ok. :)
>>>>> +{
>>>>> + struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>>>>> + struct mei_aux_device *adev;
>>>>> + struct auxiliary_device *aux_dev;
>>>>> + const struct intel_gsc_def *def;
>>>>> + int ret;
>>>>> +
>>>>> + intf->irq = -1;
>>>>> + intf->id = intf_id;
>>>>> +
>>>>> + if (intf_id == 0 && !HAS_HECI_PXP(dev_priv))
>>>>> + return;
>>>>
>>>> Isn't inf_id == 0 always a bug with this patch, regardless of
>>>> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
>>>> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
>>>>
>>> There will be patches for other cards that have pxp as soon as this is
>> reviewed.
>>> It is better to have infra prepared for two heads.
>>
>> My point is things are half-prepared since you don't have the id 0 in
>> the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but
>> if you add a patch which enables it to be true, you have to modify the
>> array at the same time or risk a broken patch in the middle.
>>
>> I don't see the point of the condition making it sound like there are
>> two criteria to enter below, while in fact there is only one in current
>> code, and that it that it must not be entered because array is incomplete!
>>
>
> We initialize both cells in gsc->intf array, the first one with defaults (two lines before this line)
> for systems without working PXP, like DG1.
> The code on GSC level does not know that we don't have PXP and don't want to know.
By defaults you mean "-1" ?
My point is intel_gsc_def_dg1[] does not contain anything valid for
interface zero. If you change HAS_HECI_PXP to return true, the code
below does:
def = &intel_gsc_def_dg1[intf_id];
And points to template data not populated.
So you have to change two in conjuction. Hence safest code for this
patch would simply be:
if (intf_id == 0) {
drm_WARN_ON_ONCE(, "Code not implemented yet!\n");
return;
}
When you add entries to intel_gsc_def_dg1[] in a later series/patch,
then you simply remove the above lines altogether.
>
>>>>> +
>>>>> + if (!HAS_HECI_GSC(gt->i915))
>>>>> + return;
>>>>
>>>> Likewise?
>>>>
>>>>> +
>>>>> + if (gt->gsc.intf[intf_id].irq <= 0) {
>>>>> + DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
>>>> set");
>>>>
>>>> Like this, but use logging functions which say which device please.
>>>>
>>> drm_err_ratelimited fits here?
>>
>> AFAICT it would be a programming bug and not something that can happen
>> at runtime hence drm_warn_on_once sounds correct for both.
>>
>
> Sure, will do
>
>>>>> }
>>>>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>>> /* Disable RCS, BCS, VCS and VECS class engines. */
>>>>> intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
>>>> 0);
>>>>> intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0);
>>>>> + if (HAS_HECI_GSC(gt->i915))
>>>>> + intel_uncore_write(uncore,
>>>> GEN11_GUNIT_CSME_INTR_ENABLE, 0);
>>>>>
>>>>> /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */
>>>>> intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~0);
>>>>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>>> intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
>>>> ~0);
>>>>> if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>>>>> intel_uncore_write(uncore,
>>>> GEN12_VECS2_VECS3_INTR_MASK, ~0);
>>>>> + if (HAS_HECI_GSC(gt->i915))
>>>>> + intel_uncore_write(uncore,
>>>> GEN11_GUNIT_CSME_INTR_MASK, ~0);
>>>>>
>>>>> intel_uncore_write(uncore,
>>>> GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
>>>>> intel_uncore_write(uncore,
>>>> GEN11_GPM_WGBOXPERF_INTR_MASK, ~0);
>>>>> @@ -209,6 +216,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>>>>> {
>>>>> struct intel_uncore *uncore = gt->uncore;
>>>>> u32 irqs = GT_RENDER_USER_INTERRUPT;
>>>>> + const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
>>>>
>>>> Why enable the one which is not supported by the patch? No harm doing
>> it?
>>>>
>>> No harm and the next patch will be soon, this patch unfortunately is long
>> overdue.
>>
>> Just feels a bit lazy. You are adding two feature test macros to
>> prepare, so why not use them.
>>
>
> I've been told that better to enable them both from the HW perspective,
> the real interrupt enable magic happens in GSC FW, not here.
Well whatever.. As long as logging of spurious/unexpected interrupts is
in place I can live with that.
Regards,
Tvrtko
Powered by blists - more mailing lists