linux-kernel - Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <X/8GCc9e/2GmYOyz@myrica>
Date:   Wed, 13 Jan 2021 15:39:05 +0100
From:   Jean-Philippe Brucker <jean-philippe@...aro.org>
To:     Zhangfei Gao <zhangfei.gao@...aro.org>
Cc:     Bjorn Helgaas <helgaas@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>, kenneth-lee-2012@...mail.com,
        wangzhou1@...ilicon.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        wanghuiqiang <wanghuiqiang@...wei.com>
Subject: Re: [PATCH] PCI: Add a quirk to enable SVA for HiSilicon chip

On Wed, Jan 13, 2021 at 08:05:11PM +0800, Zhangfei Gao wrote:
> > > +	/* Device-tree can set the stall property */
> > > +	if (!pdev->dev.of_node &&
> > > +	    device_add_properties(&pdev->dev, properties))
> > Does this mean "dma-can-stall" *can* be set via DT, and if it is, this
> > quirk is not needed?  So is this quirk basically a workaround for an
> > old or broken DT?
> The quirk is still needed for uefi case, since uefi can not describe the
> endpoints (peripheral devices).

Yes, this comment isn't very clear. How about
	/*
	 * Set the dma-can-stall property on ACPI platforms. Device tree
	 * can set it directly.
	 */ 

> > 
> > > +		pci_warn(pdev, "could not add stall property");
> > > +}
> > > +
> > Remove this blank line to follow the style of the rest of the file.
> > 
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa255, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa256, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa258, quirk_huawei_pcie_sva);
> > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa259, quirk_huawei_pcie_sva);
> > > +
> > >   /*
> > >    * It's possible for the MSI to get corrupted if SHPC and ACPI are used
> > >    * together on certain PXH-based systems.
> 
> How about changes like this
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 68f53f7..886ea26 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2466,6 +2466,9 @@ static int arm_smmu_enable_pasid(struct
> arm_smmu_master *master)
>      if (num_pasids <= 0)
>          return num_pasids;
> 
> +    if (master->stall_enabled)
> +        pdev->pasid_no_tlp = 1;
> +

>From the SMMU perspective there is no relation between stall and pasid, so
I don't think this makes a lot of sense. Could we instead set pasid_no_tlp
for the list of device IDs above?

I agree with splitting the patches. PASID support for SMMUv3 is upstream,
but the introduction of dma-can-stall, which this depends on, is still
pending on the list.

Thanks,
Jean