[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200603003232.GC109930@google.com>
Date: Tue, 2 Jun 2020 17:32:32 -0700
From: Prashant Malani <pmalani@...gle.com>
To: Rajat Jain <rajatxjain@...il.com>
Cc: Rajat Jain <rajatja@...gle.com>,
David Woodhouse <dwmw2@...radead.org>,
Lu Baolu <baolu.lu@...ux.intel.com>,
Joerg Roedel <joro@...tes.org>,
iommu@...ts.linux-foundation.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Mika Westerberg <mika.westerberg@...el.com>,
Ashok Raj <ashok.raj@...el.com>,
lalithambika.krishnakumar@...el.com, bleung@...gle.com,
levinale@...gle.com, zsm@...gle.com, mnissler@...gle.com,
tbroch@...gle.com
Subject: Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted
devices
(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@...gle.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
> > >
Powered by blists - more mailing lists