From: Prashant Malani via iommu <iommu@lists.linux-foundation.org>
To: Rajat Jain <rajatxjain@gmail.com>
Cc: tbroch@google.com, Ashok Raj <ashok.raj@intel.com>,
zsm@google.com, levinale@google.com,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
lalithambika.krishnakumar@intel.com,
iommu@lists.linux-foundation.org, mnissler@google.com,
bleung@google.com, Rajat Jain <rajatja@google.com>,
David Woodhouse <dwmw2@infradead.org>,
Mika Westerberg <mika.westerberg@intel.com>
Subject: Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Date: Tue, 2 Jun 2020 17:32:32 -0700 [thread overview]
Message-ID: <20200603003232.GC109930@google.com> (raw)
In-Reply-To: <CAA93t1r9Kg0OFwoOhNUKYeSbZrFgu5k_eexer34msjsLBNohcg@mail.gmail.com>
(Trimming text)
On Wed, Jun 03, 2020 at 12:23:48AM +0000, Rajat Jain wrote:
> On Tue, Jun 2, 2020 at 4:49 PM Prashant Malani <pmalani@google.com> wrote:
> >
> > Hi Rajat,
>
> Hi Prashant, thanks for taking a look.
>
> >
> > On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> > > +static bool risky_device(struct pci_dev *pdev)
> > > +{
> > > + if (pdev->untrusted) {
> > > + pci_warn(pdev,
> > > + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> > > + " PCI link. Please check with your BIOS/Platform"
> > > + " vendor about this\n", pdev->vendor, pdev->device);
> > > + return true;
> > > + }
> > > + return false;
> > minor suggestion: Perhaps you could use a guard clause here? It would save you
> > a level of indentation, and possibly allow better string splitting
> > (e.g keeping "untrusted PCI" together). So something like:
> >
> > if (!pdev->untrusted)
> > return false;
>
> I personally have found double negation expressions always confusing,
> even if negation is part of the variable. (For e.g. I have found I
> need to be always stop and convince myself that:
>
> "if (!pdev->untrusted)"
> <do something>
>
> conceptually implies
>
> "if (pdev->trusted)".
> <do something>
>
> So I tend to keep negations to minimum. In this case, it doesn't buy
> us much either, so I'd prefer to keep it the same unless there are
> more opinions on this. OTOH I don't mind changing it too if you feel
> strongly about this.
Ordinarily, I'd agree with you regarding double-negatives.
However, in this case the condition phrasing is so brief ("not untrusted") that I'd
argue the indentation savings outweigh possible interpretation issues.
That said, I don't have a strong opinion here, so will defer to the maintainer's preference.
Best,
>
> Thanks,
>
> Rajat
>
>
> >
> > pci_warn(...);
> >
> > I also hear the column limit warning is now for 100 chars [1], though
> > I'm not sure how it's being handled in this file.
> >
> > Best regards,
> >
> > -Prashant
> >
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> >
> > > +}
> > > +
> > > const struct iommu_ops intel_iommu_ops = {
> > > .capable = intel_iommu_capable,
> > > .domain_alloc = intel_iommu_domain_alloc,
> > > @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
> > >
> > > static void quirk_iommu_igfx(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > > dmar_map_gfx = 0;
> > > }
> > > @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
> > >
> > > static void quirk_iommu_rwbf(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > /*
> > > * Mobile 4 Series Chipset neglects to set RWBF capability,
> > > * but needs it. Same seems to hold for the desktop versions.
> > > @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
> > > {
> > > unsigned short ggc;
> > >
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > if (pci_read_config_word(dev, GGC, &ggc))
> > > return;
> > >
> > > @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> > > pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> > > if (!pdev)
> > > return;
> > > +
> > > + if (risky_device(pdev)) {
> > > + pci_dev_put(pdev);
> > > + return;
> > > + }
> > > +
> > > pci_dev_put(pdev);
> > >
> > > /* System Management Registers. Might be hidden, in which case
> > > @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> > > if (!pdev)
> > > return;
> > >
> > > + if (risky_device(pdev)) {
> > > + pci_dev_put(pdev);
> > > + return;
> > > + }
> > > +
> > > if (pci_read_config_dword(pdev, 0x188, &vtisochctrl)) {
> > > pci_dev_put(pdev);
> > > return;
> > > --
> > > 2.27.0.rc2.251.g90737beb825-goog
> > >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2020-06-03 0:32 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-02 23:26 [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted devices Rajat Jain via iommu
2020-06-02 23:28 ` Raj, Ashok
2020-06-02 23:49 ` Prashant Malani via iommu
2020-06-03 0:23 ` Rajat Jain
2020-06-03 0:32 ` Prashant Malani via iommu [this message]
2020-06-03 5:30 ` Mika Westerberg
2020-06-03 13:03 ` Rajat Jain via iommu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200603003232.GC109930@google.com \
--to=iommu@lists.linux-foundation.org \
--cc=ashok.raj@intel.com \
--cc=bleung@google.com \
--cc=dwmw2@infradead.org \
--cc=lalithambika.krishnakumar@intel.com \
--cc=levinale@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mika.westerberg@intel.com \
--cc=mnissler@google.com \
--cc=pmalani@google.com \
--cc=rajatja@google.com \
--cc=rajatxjain@gmail.com \
--cc=tbroch@google.com \
--cc=zsm@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).