Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Jun 2020 17:32:32 -0700
From:   Prashant Malani <pmalani@google.com>
To:     Rajat Jain <rajatxjain@gmail.com>
Cc:     Rajat Jain <rajatja@google.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>,
        iommu@lists.linux-foundation.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mika Westerberg <mika.westerberg@intel.com>,
        Ashok Raj <ashok.raj@intel.com>,
        lalithambika.krishnakumar@intel.com, bleung@google.com,
        levinale@google.com, zsm@google.com, mnissler@google.com,
        tbroch@google.com
Subject: Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted
 devices
Message-ID: <20200603003232.GC109930@google.com>
References: <20200602232602.156049-1-rajatja@google.com>
 <20200602234912.GB109930@google.com>
 <CAA93t1r9Kg0OFwoOhNUKYeSbZrFgu5k_eexer34msjsLBNohcg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAA93t1r9Kg0OFwoOhNUKYeSbZrFgu5k_eexer34msjsLBNohcg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

(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
> > >