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;
Subject: Re: [PATCH 2/2] swiotlb-xen: override common mmap and get_sgtable dma
 ops
To:     Roman Skakun <rm.skakun@gmail.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        xen-devel@lists.xenproject.org, iommu@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, Christoph Hellwig <hch@lst.de>
Cc:     Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
        Volodymyr Babchuk <volodymyr_babchuk@epam.com>,
        Roman Skakun <roman_skakun@epam.com>,
        Andrii Anisov <andrii_anisov@epam.com>
References: <855a58e2-1e03-4763-cb56-81367b73762c@oracle.com>
 <20210616114205.38902-1-roman_skakun@epam.com>
 <20210616114205.38902-2-roman_skakun@epam.com>
From:   Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <2834cdc0-534c-4f07-1901-e468a7713c1f@oracle.com>
Date:   Wed, 16 Jun 2021 10:12:55 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.11.0
In-Reply-To: <20210616114205.38902-2-roman_skakun@epam.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [10.74.102.136] (160.34.88.136) by SJ0PR05CA0152.namprd05.prod.outlook.com (2603:10b6:a03:339::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4264.7 via Frontend Transport; Wed, 16 Jun 2021 14:12:59 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: aa76d82e-ff93-4f88-4148-08d930d0da67
X-MS-TrafficTypeDiagnostic: BLAPR10MB5010:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BLAPR10MB5010217FBC106EA9B3FC25A78A0F9@BLAPR10MB5010.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:8273;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 9WqY3a8BulNSroI+92f5uZf/SbIOjBy6tQJalZafuswYUfzDfGhopKg2h45D4ezJKbdtrpGZtsee0TPFaVXxlm5eo3RsQEic9UukS5kSaGrAUVdnyNgkeeBNdVs9qjFt0rmx6k/xxPNr7+1i4Q1I/O/cy5dP1jXSAcvQDkd2beSy9C0bu3r7EFeDlVcU2RENdvNaBKdyTg71fLq0ms+MFBIL7wxxvit/+3bgTTjOMuMG88zUjm80eQt5lEAnuAtala6hrFQQctDCfiMxlvB+71HzMBbzU+x8ENcOjwJn1h3hx1X3SBUBLfJx2IvpkKF6p8JRz2/JzukZrUhqr9J9vmA1czOt92ID14S+GtcAlnA8xEwyBHk+cgXIvT/PwMH9zCHhdhkjary7odRH+Bof1/1Wp4HqebauBUhz4XISaM3w2rcA+6vdwICf0GkxZg9/c3xMOS4N5llhPqGg76DuUeK9C+XJaRzWBWoWP8VLWQuzRt0qjl03UyIl3FSZwTGaj6fXGZFjmkJa2VvuXe+dXv0AYgjJG04zQwfE8QXRvUMMiigg5adDU43DnXAS/b7lD4kK7XK3AWYWAQEkzAfPBdt/P2lZwvkGKkowRXRXMQul9Ben3xmoGLxPDpDsNoum+8O4rh7QHp3OZ8RdSCMI0QfYkRqEEDC2F2ANk6HdjdOU/+rIe/kLo+tmG2Wj4dYu
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BLAPR10MB5009.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(136003)(376002)(396003)(346002)(39860400002)(366004)(66476007)(66556008)(8936002)(2906002)(4326008)(44832011)(16526019)(478600001)(6666004)(83380400001)(186003)(7416002)(8676002)(38100700002)(54906003)(110136005)(5660300002)(2616005)(16576012)(26005)(31696002)(316002)(956004)(86362001)(31686004)(53546011)(6486002)(36756003)(66946007)(45980500001)(43740500002);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?clVQVTRkTUlHcERvdFFvMDUwZ3RmRDZ5c1RQQUdNNVV3dmsrUEVVRFF2dzVn?=
 =?utf-8?B?VUhvQ0I2NlIveDBCVkV5Ti9qMlFUdldkWDFaQmkrNUp3ck12NHhZTHZmUDVs?=
 =?utf-8?B?Zk02UmJqMHVkOWxiYmlOWFpGanZTZFF4ZU8rOFJuZHpTUjVOdXluczlLR0RQ?=
 =?utf-8?B?QkdKMFRQMG1rVmpJbGltOEpXNjd5TEhsU3lRZ0gxMWp3V1JjTFlBUTE0M3N6?=
 =?utf-8?B?S1ZreEQ4RUxiaWVnSFdkbEVjdTM1NU1GU3orTlVjN3ZFdEh0akJvejdNZ2dq?=
 =?utf-8?B?aDZiclZwSmpXYk9JdEgxWVk4bnMwNlRsdEVGTW1pN25kN2NJQm8rQTVJUDhF?=
 =?utf-8?B?WUtPcmVxQ3c1anJBQVRqcFFLT2V4R1lNczl4WkZHT2pkNjNjRXdyZWJiTUdm?=
 =?utf-8?B?WnNXNmFwbGd6VzRXeHJvYlU2c2NXazVLVzlTVGhBNnYwU2dhU250dFR5cmN0?=
 =?utf-8?B?QllySEpKYUJVdHE5N2NYZmVaNGkrZlRweE9MNmppaWhjdXpZNWVzQkVsRTJk?=
 =?utf-8?B?c1NLbHZLU214VU9vUkxtUE5EQnJ2VmVQOVVobHQ1T210RmxzNllEcHBHRHhG?=
 =?utf-8?B?aW9XZDk3OFNFRHdxakU3OHMzVjVETWFPTHQyNmpucTkwOHZDa3RwWDJQeE8y?=
 =?utf-8?B?NjVlb29BUkh2MW1peklQYWZ4VEpvNTRNK0l2U2srbFBtTWdCN1VuOXZwTHdB?=
 =?utf-8?B?MFBueDVTaHZsTzJ5ci9ZVnN4YXZIdGVLcEl3ZlBkNmFBSUtxRlJMaFlLaVhy?=
 =?utf-8?B?ekhBNUVKNkdYM2x4Yk8wMEdPQUhCNmhNeE1NMDdmczdVdWs1QjlNaURjaHpF?=
 =?utf-8?B?Ty9NdUZrcUVVWU1na3d2YnVhWFZ5RmMzOHpBVTdVRnkybHlSU1Nldks5Y3Iv?=
 =?utf-8?B?T1puT1RGMHhjVWdSdGRzcjVOUHRTSzR4ejZSa3JISWovZHdTN1pKUHdkbUtv?=
 =?utf-8?B?MUxsK2ZjWTVidUJvOWN4SWl2SW9rd3ZndzdNWFhzV3NmVVNpamhNTVVPWE9x?=
 =?utf-8?B?bzhVdHhLaERmV1ZEQ01obTVLUHJWMDhva2tMSHQ4VDZjYVl0SFN6eXovbG5p?=
 =?utf-8?B?eFBEVldGRlhCcnk3b0NYQit2eHJ1MjdHU2NzMUE2T2VxNmdCdDdCVTZmWXNm?=
 =?utf-8?B?SkNEL1loTGFVL3ovUHE1aU9tMGxDZDY1T1pjbHNGMzVMR1pkdFIyWjkwc1Jm?=
 =?utf-8?B?UEdQWDhIV1liNlZ3Q2ZNdXpRcWh6RnhjMkRuZ3UvVDlLQmVra1YyaW9VK2c4?=
 =?utf-8?B?dzdhWFEwOHM4VGhsS2ZjU1dSK3lJTzZSaTZqVW5kdUdEWWh5QnJLTXFXSXVM?=
 =?utf-8?B?R2w2TkViMDdIbzJrOG8xbzBaZFVVaVdaRG1SZ3F1YkFLZGFSaFArTml3aHZV?=
 =?utf-8?B?UnNmSEhpeFZnaHRhQVg3eU1YRTdGM1I5ZHBhRU9pZ3hOK2F2ZUxBdnZ2RTB3?=
 =?utf-8?B?QjNPMG5QRmdyOW9IOU5OVGs1Yk8vYkZnaDl4cHhCYkNVUDF2QndrTThxWVF0?=
 =?utf-8?B?THZmSmVVeG0xQy9EdWxoemZ2RW9aL0phRDhTeWtiNmRYVXhpb2ZaQ1BOTXNU?=
 =?utf-8?B?OE8xUEY4SndhWXd3U2tUc3B5TGsyMjQ3WlZFUHJRaGs3azZlS2J6TGYrdmFH?=
 =?utf-8?B?Rm05cEJhenFPZ3hGWStJeDhDY3NYMEYrc0V4WDY2ZWlRSEFiRVFkdkhNYmQ5?=
 =?utf-8?B?aVlzQ2RQSTRlWDl1L01sV1BpYWZ5M3h2R0V5bzltN01HZnBWR2JKbmx0RmNO?=
 =?utf-8?Q?J47h/hknymDYuAMp0nsYTfBEX2wXjP7lpNQP+zV?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: aa76d82e-ff93-4f88-4148-08d930d0da67
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB5009.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2021 14:13:02.6670
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4DUX2/d7H8Nh5WUow0Xu3IguO83SfOSSzhqDZ07uad5oABLIdAgDloOyH1RbwZbHLKcvanyfgUOQMGjwCLpE4a0/MPgz6CkPsyMEGQeeLGY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5010
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10016 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 mlxlogscore=999 spamscore=0 adultscore=0 bulkscore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2106160083
X-Proofpoint-GUID: 8eyhWaWUFcFNEW6BVBQAZX7jHMuNc9vm
X-Proofpoint-ORIG-GUID: 8eyhWaWUFcFNEW6BVBQAZX7jHMuNc9vm
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/16/21 7:42 AM, Roman Skakun wrote:
> This commit is dedicated to fix incorrect conversion from
> cpu_addr to page address in cases when we get virtual
> address which allocated through xen_swiotlb_alloc_coherent()
> and can be mapped in the vmalloc range.
> As the result, virt_to_page() cannot convert this address
> properly and return incorrect page address.
>
> Need to detect such cases and obtains the page address using
> vmalloc_to_page() instead.
>
> The reference code for mmap() and get_sgtable() was copied
> from kernel/dma/ops_helpers.c and modified to provide
> additional detections as described above.
>
> In order to simplify code there was added a new
> dma_cpu_addr_to_page() helper.
>
> Signed-off-by: Roman Skakun <roman_skakun@epam.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  drivers/xen/swiotlb-xen.c | 42 +++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 90bc5fc321bc..9331a8500547 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -118,6 +118,14 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
>  	return 0;
>  }
>  
> +static struct page *cpu_addr_to_page(void *cpu_addr)
> +{
> +	if (is_vmalloc_addr(cpu_addr))
> +		return vmalloc_to_page(cpu_addr);
> +	else
> +		return virt_to_page(cpu_addr);
> +}
> +
>  static int
>  xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
>  {
> @@ -337,7 +345,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>  	int order = get_order(size);
>  	phys_addr_t phys;
>  	u64 dma_mask = DMA_BIT_MASK(32);
> -	struct page *page;
> +	struct page *page = cpu_addr_to_page(vaddr);
>  
>  	if (hwdev && hwdev->coherent_dma_mask)
>  		dma_mask = hwdev->coherent_dma_mask;
> @@ -349,11 +357,6 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>  	/* Convert the size to actually allocated. */
>  	size = 1UL << (order + XEN_PAGE_SHIFT);
>  
> -	if (is_vmalloc_addr(vaddr))
> -		page = vmalloc_to_page(vaddr);
> -	else
> -		page = virt_to_page(vaddr);
> -
>  	if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
>  		     range_straddles_page_boundary(phys, size)) &&
>  	    TestClearPageXenRemapped(page))
> @@ -573,7 +576,23 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>  		     void *cpu_addr, dma_addr_t dma_addr, size_t size,
>  		     unsigned long attrs)
>  {
> -	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, attrs);
> +	unsigned long user_count = vma_pages(vma);
> +	unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +	unsigned long off = vma->vm_pgoff;
> +	struct page *page = cpu_addr_to_page(cpu_addr);
> +	int ret;
> +
> +	vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> +
> +	if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> +		return ret;
> +
> +	if (off >= count || user_count > count - off)
> +		return -ENXIO;
> +
> +	return remap_pfn_range(vma, vma->vm_start,
> +			page_to_pfn(page) + vma->vm_pgoff,
> +			user_count << PAGE_SHIFT, vma->vm_page_prot);
>  }


I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there.


Christoph, would that work?  I.e. something like


diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
index 910ae69cae77..43411c2fa47b 100644
--- a/kernel/dma/ops_helpers.c
+++ b/kernel/dma/ops_helpers.c
@@ -12,7 +12,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
                 void *cpu_addr, dma_addr_t dma_addr, size_t size,
                 unsigned long attrs)
 {
-       struct page *page = virt_to_page(cpu_addr);
+       struct page *page = cpu_addr_to_page(cpu_addr);
        int ret;
 
        ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
@@ -43,7 +43,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
                return -ENXIO;
 
        return remap_pfn_range(vma, vma->vm_start,
-                       page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
+                       page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
                        user_count << PAGE_SHIFT, vma->vm_page_prot);
 #else
        return -ENXIO;


-boris


>  
>  /*
> @@ -585,7 +604,14 @@ xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>  			void *cpu_addr, dma_addr_t handle, size_t size,
>  			unsigned long attrs)
>  {
> -	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
> +	struct page *page = cpu_addr_to_page(cpu_addr);
> +	int ret;
> +
> +	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> +	if (!ret)
> +		sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
> +
> +	return ret;
>  }
>  
>  const struct dma_map_ops xen_swiotlb_dma_ops = {