netdev - Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <pj41zltutlnojd.fsf@u68c7b5b1d2d758.ant.amazon.com>
Date:   Thu, 19 Nov 2020 21:18:30 +0200
From:   Shay Agroskin <shayagr@...zon.com>
To:     Heiner Kallweit <hkallweit1@...il.com>
CC:     <kuba@...nel.org>, <netdev@...r.kernel.org>, <dwmw@...zon.com>,
        <zorik@...zon.com>, <matua@...zon.com>, <saeedb@...zon.com>,
        <msw@...zon.com>, <aliguori@...zon.com>, <nafea@...zon.com>,
        <gtzalik@...zon.com>, <netanel@...zon.com>, <alisaidi@...zon.com>,
        <benh@...zon.com>, <akiyano@...zon.com>, <sameehj@...zon.com>,
        <ndagan@...zon.com>, Mike Cui <mikecui@...zon.com>
Subject: Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid
 intel iommu issue


Heiner Kallweit <hkallweit1@...il.com> writes:

> Am 18.11.2020 um 23:35 schrieb Heiner Kallweit:
>> Am 18.11.2020 um 22:59 schrieb Shay Agroskin:
>>> The ENA driver uses the readless mechanism, which uses DMA, to 
>>> find
>>> out what the DMA mask is supposed to be.
>>>
>>> If DMA is used without setting the dma_mask first, it causes 
>>> the
>>> Intel IOMMU driver to think that ENA is a 32-bit device and 
>>> therefore
>>> disables IOMMU passthrough permanently.
>>>
>>> This patch sets the dma_mask to be 
>>> ENA_MAX_PHYS_ADDR_SIZE_BITS=48
>>> before readless initialization in
>>> ena_device_init()->ena_com_mmio_reg_read_request_init(),
>>> which is large enough to workaround the intel_iommu issue.
>>>
>>> DMA mask is set again to the correct value after it's received 
>>> from the
>>> device after readless is initialized.
>>>
>>> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon 
>>> Elastic Network Adapters (ENA)")
>>> Signed-off-by: Mike Cui <mikecui@...zon.com>
>>> Signed-off-by: Arthur Kiyanovski <akiyano@...zon.com>
>>> Signed-off-by: Shay Agroskin <shayagr@...zon.com>
>>> ---
>>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 13 
>>>  +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c 
>>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> index 574c2b5ba21e..854a22e692bf 100644
>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> @@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev 
>>> *pdev, const struct pci_device_id *ent)
>>>  		return rc;
>>>  	}
>>>  
>>> +	rc = pci_set_dma_mask(pdev, 
>>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>> +	if (rc) {
>>> +		dev_err(&pdev->dev, "pci_set_dma_mask failed 
>>> %d\n", rc);
>>> +		goto err_disable_device;
>>> +	}
>>> +
>>> +	rc = pci_set_consistent_dma_mask(pdev, 
>>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>> +	if (rc) {
>>> +		dev_err(&pdev->dev, 
>>> "err_pci_set_consistent_dma_mask failed %d\n",
>>> +			rc);
>>> +		goto err_disable_device;
>>> +	}
>>> +
>>>  	pci_set_master(pdev);
>>>  
>>>  	ena_dev = vzalloc(sizeof(*ena_dev));
>>>
>> 
>> The old pci_ dma wrappers are being phased out and shouldn't be 
>> used in
>> new code. See e.g. e059c6f340f6 ("tulip: switch from 'pci_' to 
>> 'dma_' API").
>> So better use:
>> dma_set_mask_and_coherent(&pdev->dev, 
>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));

Thank you for reviewing these patches. We will switch to using 
dma_set_...() instead

>> 
>
> However instead of dev_err(&pdev->dev, ..) you could use 
> pci_err(pdev, ..).

I see that pci_err evaluates to dev_err. While I see how using 
pci_* log function helps code readability, I prefer using
dev_err here to keep the code consistent with the rest of the 
driver. We'll discuss changing all log functions in future patches 
to net-next if that's okay.