Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Prashant Malani]

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