* [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer
@ 2022-10-05 17:48 Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-05 17:48 UTC (permalink / raw)
To: xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Hello all.
These are several fixes I collected when playing with virtio-net device
using Xen grant mappings.
Tested with both virtio-blk and virtio-net devices.
Oleksandr Tyshchenko (2):
xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
xen/virtio: Fix potential deadlock when accessing
xen_grant_dma_devices
drivers/xen/grant-dma-ops.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-05 17:48 [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Oleksandr Tyshchenko
@ 2022-10-05 17:48 ` Oleksandr Tyshchenko
2022-10-06 0:07 ` Stefano Stabellini
` (2 more replies)
2022-10-05 17:48 ` [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices Oleksandr Tyshchenko
2022-10-07 5:19 ` [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Juergen Gross
2 siblings, 3 replies; 13+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-05 17:48 UTC (permalink / raw)
To: xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Take page offset into the account when calculating the number of pages
to be granted.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
---
drivers/xen/grant-dma-ops.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 8973fc1e9ccc..1998d0e8ce82 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
unsigned long attrs)
{
struct xen_grant_dma_data *data;
- unsigned int i, n_pages = PFN_UP(size);
+ unsigned int i, n_pages = PFN_UP(offset + size);
grant_ref_t grant;
dma_addr_t dma_handle;
@@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
unsigned long attrs)
{
struct xen_grant_dma_data *data;
- unsigned int i, n_pages = PFN_UP(size);
+ unsigned long offset = dma_handle & (PAGE_SIZE - 1);
+ unsigned int i, n_pages = PFN_UP(offset + size);
grant_ref_t grant;
if (WARN_ON(dir == DMA_NONE))
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices
2022-10-05 17:48 [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
@ 2022-10-05 17:48 ` Oleksandr Tyshchenko
2022-10-06 0:19 ` Stefano Stabellini
2022-10-06 5:08 ` Juergen Gross
2022-10-07 5:19 ` [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Juergen Gross
2 siblings, 2 replies; 13+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-05 17:48 UTC (permalink / raw)
To: xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
As find_xen_grant_dma_data() is called from both interrupt and process
contexts, the access to xen_grant_dma_devices XArray must be protected
by xa_lock_irqsave to avoid deadlock scenario.
As XArray API doesn't provide xa_store_irqsave helper, call lockless
__xa_store directly and guard it externally.
Also move the storage of the XArray's entry to a separate helper.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
---
drivers/xen/grant-dma-ops.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 1998d0e8ce82..c66f56d24013 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -25,7 +25,7 @@ struct xen_grant_dma_data {
bool broken;
};
-static DEFINE_XARRAY(xen_grant_dma_devices);
+static DEFINE_XARRAY_FLAGS(xen_grant_dma_devices, XA_FLAGS_LOCK_IRQ);
#define XEN_GRANT_DMA_ADDR_OFF (1ULL << 63)
@@ -42,14 +42,29 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
static struct xen_grant_dma_data *find_xen_grant_dma_data(struct device *dev)
{
struct xen_grant_dma_data *data;
+ unsigned long flags;
- xa_lock(&xen_grant_dma_devices);
+ xa_lock_irqsave(&xen_grant_dma_devices, flags);
data = xa_load(&xen_grant_dma_devices, (unsigned long)dev);
- xa_unlock(&xen_grant_dma_devices);
+ xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
return data;
}
+static int store_xen_grant_dma_data(struct device *dev,
+ struct xen_grant_dma_data *data)
+{
+ unsigned long flags;
+ int ret;
+
+ xa_lock_irqsave(&xen_grant_dma_devices, flags);
+ ret = xa_err(__xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
+
+ return ret;
+}
+
/*
* DMA ops for Xen frontends (e.g. virtio).
*
@@ -338,8 +353,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
*/
data->backend_domid = iommu_spec.args[0];
- if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
- GFP_KERNEL))) {
+ if (store_xen_grant_dma_data(dev, data)) {
dev_err(dev, "Cannot store Xen grant DMA data\n");
goto err;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
@ 2022-10-06 0:07 ` Stefano Stabellini
2022-10-06 5:06 ` Juergen Gross
2022-10-06 7:35 ` Xenia Ragiadakou
2 siblings, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2022-10-06 0:07 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: xen-devel, linux-kernel, Oleksandr Tyshchenko,
Stefano Stabellini, Juergen Gross
On Wed, 5 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Take page offset into the account when calculating the number of pages
> to be granted.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> drivers/xen/grant-dma-ops.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
> unsigned long attrs)
> {
> struct xen_grant_dma_data *data;
> - unsigned int i, n_pages = PFN_UP(size);
> + unsigned int i, n_pages = PFN_UP(offset + size);
> grant_ref_t grant;
> dma_addr_t dma_handle;
>
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> unsigned long attrs)
> {
> struct xen_grant_dma_data *data;
> - unsigned int i, n_pages = PFN_UP(size);
> + unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> + unsigned int i, n_pages = PFN_UP(offset + size);
> grant_ref_t grant;
>
> if (WARN_ON(dir == DMA_NONE))
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices
2022-10-05 17:48 ` [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices Oleksandr Tyshchenko
@ 2022-10-06 0:19 ` Stefano Stabellini
2022-10-06 5:08 ` Juergen Gross
1 sibling, 0 replies; 13+ messages in thread
From: Stefano Stabellini @ 2022-10-06 0:19 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: xen-devel, linux-kernel, Oleksandr Tyshchenko,
Stefano Stabellini, Juergen Gross
On Wed, 5 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> As find_xen_grant_dma_data() is called from both interrupt and process
> contexts, the access to xen_grant_dma_devices XArray must be protected
> by xa_lock_irqsave to avoid deadlock scenario.
> As XArray API doesn't provide xa_store_irqsave helper, call lockless
> __xa_store directly and guard it externally.
>
> Also move the storage of the XArray's entry to a separate helper.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> drivers/xen/grant-dma-ops.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 1998d0e8ce82..c66f56d24013 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -25,7 +25,7 @@ struct xen_grant_dma_data {
> bool broken;
> };
>
> -static DEFINE_XARRAY(xen_grant_dma_devices);
> +static DEFINE_XARRAY_FLAGS(xen_grant_dma_devices, XA_FLAGS_LOCK_IRQ);
>
> #define XEN_GRANT_DMA_ADDR_OFF (1ULL << 63)
>
> @@ -42,14 +42,29 @@ static inline grant_ref_t dma_to_grant(dma_addr_t dma)
> static struct xen_grant_dma_data *find_xen_grant_dma_data(struct device *dev)
> {
> struct xen_grant_dma_data *data;
> + unsigned long flags;
>
> - xa_lock(&xen_grant_dma_devices);
> + xa_lock_irqsave(&xen_grant_dma_devices, flags);
> data = xa_load(&xen_grant_dma_devices, (unsigned long)dev);
> - xa_unlock(&xen_grant_dma_devices);
> + xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
>
> return data;
> }
>
> +static int store_xen_grant_dma_data(struct device *dev,
> + struct xen_grant_dma_data *data)
> +{
> + unsigned long flags;
> + int ret;
> +
> + xa_lock_irqsave(&xen_grant_dma_devices, flags);
> + ret = xa_err(__xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
> + GFP_ATOMIC));
> + xa_unlock_irqrestore(&xen_grant_dma_devices, flags);
> +
> + return ret;
> +}
> +
> /*
> * DMA ops for Xen frontends (e.g. virtio).
> *
> @@ -338,8 +353,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
> */
> data->backend_domid = iommu_spec.args[0];
>
> - if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
> - GFP_KERNEL))) {
> + if (store_xen_grant_dma_data(dev, data)) {
> dev_err(dev, "Cannot store Xen grant DMA data\n");
> goto err;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
2022-10-06 0:07 ` Stefano Stabellini
@ 2022-10-06 5:06 ` Juergen Gross
2022-10-06 7:35 ` Xenia Ragiadakou
2 siblings, 0 replies; 13+ messages in thread
From: Juergen Gross @ 2022-10-06 5:06 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 437 bytes --]
On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Take page offset into the account when calculating the number of pages
> to be granted.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices
2022-10-05 17:48 ` [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices Oleksandr Tyshchenko
2022-10-06 0:19 ` Stefano Stabellini
@ 2022-10-06 5:08 ` Juergen Gross
1 sibling, 0 replies; 13+ messages in thread
From: Juergen Gross @ 2022-10-06 5:08 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 734 bytes --]
On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> As find_xen_grant_dma_data() is called from both interrupt and process
> contexts, the access to xen_grant_dma_devices XArray must be protected
> by xa_lock_irqsave to avoid deadlock scenario.
> As XArray API doesn't provide xa_store_irqsave helper, call lockless
> __xa_store directly and guard it externally.
>
> Also move the storage of the XArray's entry to a separate helper.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
2022-10-06 0:07 ` Stefano Stabellini
2022-10-06 5:06 ` Juergen Gross
@ 2022-10-06 7:35 ` Xenia Ragiadakou
2022-10-06 8:05 ` Juergen Gross
2 siblings, 1 reply; 13+ messages in thread
From: Xenia Ragiadakou @ 2022-10-06 7:35 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross
On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Take page offset into the account when calculating the number of pages
> to be granted.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access under Xen")
> ---
> drivers/xen/grant-dma-ops.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..1998d0e8ce82 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page,
> unsigned long attrs)
> {
> struct xen_grant_dma_data *data;
> - unsigned int i, n_pages = PFN_UP(size);
> + unsigned int i, n_pages = PFN_UP(offset + size);
Here, why do we use PFN_UP and not XEN_PFN_UP?
Also, since the variable 'n_pages' seems to refer to the number of
grants (unless I got it all entirely wrong ...), wouldn't it be more
suitable to call explicitly gnttab_count_grant()?
If the above questions have been already answered in the past, please
ignore.
> grant_ref_t grant;
> dma_addr_t dma_handle;
>
> @@ -185,7 +185,8 @@ static void xen_grant_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> unsigned long attrs)
> {
> struct xen_grant_dma_data *data;
> - unsigned int i, n_pages = PFN_UP(size);
> + unsigned long offset = dma_handle & (PAGE_SIZE - 1);
> + unsigned int i, n_pages = PFN_UP(offset + size);
> grant_ref_t grant;
>
> if (WARN_ON(dir == DMA_NONE))
--
Xenia
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-06 7:35 ` Xenia Ragiadakou
@ 2022-10-06 8:05 ` Juergen Gross
2022-10-06 10:14 ` Oleksandr Tyshchenko
0 siblings, 1 reply; 13+ messages in thread
From: Juergen Gross @ 2022-10-06 8:05 UTC (permalink / raw)
To: Xenia Ragiadakou, Oleksandr Tyshchenko, xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 1482 bytes --]
On 06.10.22 09:35, Xenia Ragiadakou wrote:
>
> On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Take page offset into the account when calculating the number of pages
>> to be granted.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory access
>> under Xen")
>> ---
>> drivers/xen/grant-dma-ops.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index 8973fc1e9ccc..1998d0e8ce82 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device
>> *dev, struct page *page,
>> unsigned long attrs)
>> {
>> struct xen_grant_dma_data *data;
>> - unsigned int i, n_pages = PFN_UP(size);
>> + unsigned int i, n_pages = PFN_UP(offset + size);
>
> Here, why do we use PFN_UP and not XEN_PFN_UP?
> Also, since the variable 'n_pages' seems to refer to the number of grants
> (unless I got it all entirely wrong ...), wouldn't it be more suitable to call
> explicitly gnttab_count_grant()?
Good point.
I think this will need another patch for switching grant-dma-ops.c to
use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-06 8:05 ` Juergen Gross
@ 2022-10-06 10:14 ` Oleksandr Tyshchenko
2022-10-06 10:24 ` Juergen Gross
0 siblings, 1 reply; 13+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-06 10:14 UTC (permalink / raw)
To: Juergen Gross, Xenia Ragiadakou
Cc: xen-devel, linux-kernel, Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]
On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <jgross@suse.com> wrote:
> On 06.10.22 09:35, Xenia Ragiadakou wrote:
>
Hello Xenia, Juergen
[sorry for the possible format issues]
> >
> > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>
> >> Take page offset into the account when calculating the number of pages
> >> to be granted.
> >>
> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory
> access
> >> under Xen")
> >> ---
> >> drivers/xen/grant-dma-ops.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> >> index 8973fc1e9ccc..1998d0e8ce82 100644
> >> --- a/drivers/xen/grant-dma-ops.c
> >> +++ b/drivers/xen/grant-dma-ops.c
> >> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct
> device
> >> *dev, struct page *page,
> >> unsigned long attrs)
> >> {
> >> struct xen_grant_dma_data *data;
> >> - unsigned int i, n_pages = PFN_UP(size);
> >> + unsigned int i, n_pages = PFN_UP(offset + size);
> >
> > Here, why do we use PFN_UP and not XEN_PFN_UP?
> > Also, since the variable 'n_pages' seems to refer to the number of
> grants
> > (unless I got it all entirely wrong ...), wouldn't it be more suitable
> to call
> > explicitly gnttab_count_grant()?
>
> Good point.
>
+1
>
> I think this will need another patch for switching grant-dma-ops.c to
> use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
>
+1
I can create a separate patch for converting on top of this series, it
would be nice to clarify one point.
So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT
respectively (where appropriate).
Should the PFN_UP be converted to XEN_PFN_UP *or* use
gnttab_count_grant() explicitly? Personally I would prefer the former, but
would also be ok with the latter.
>
>
> Juergen
>
>
--
Regards,
Oleksandr Tyshchenko
[-- Attachment #2: Type: text/html, Size: 3925 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-06 10:14 ` Oleksandr Tyshchenko
@ 2022-10-06 10:24 ` Juergen Gross
2022-10-06 10:30 ` Oleksandr Tyshchenko
0 siblings, 1 reply; 13+ messages in thread
From: Juergen Gross @ 2022-10-06 10:24 UTC (permalink / raw)
To: Oleksandr Tyshchenko, Xenia Ragiadakou
Cc: xen-devel, linux-kernel, Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 2665 bytes --]
On 06.10.22 12:14, Oleksandr Tyshchenko wrote:
>
>
> On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <jgross@suse.com
> <mailto:jgross@suse.com>> wrote:
>
> On 06.10.22 09:35, Xenia Ragiadakou wrote:
>
>
>
> Hello Xenia, Juergen
>
> [sorry for the possible format issues]
>
> >
> > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com
> <mailto:oleksandr_tyshchenko@epam.com>>
> >>
> >> Take page offset into the account when calculating the number of pages
> >> to be granted.
> >>
> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com
> <mailto:oleksandr_tyshchenko@epam.com>>
> >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict memory
> access
> >> under Xen")
> >> ---
> >> drivers/xen/grant-dma-ops.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> >> index 8973fc1e9ccc..1998d0e8ce82 100644
> >> --- a/drivers/xen/grant-dma-ops.c
> >> +++ b/drivers/xen/grant-dma-ops.c
> >> @@ -153,7 +153,7 @@ static dma_addr_t xen_grant_dma_map_page(struct device
> >> *dev, struct page *page,
> >> unsigned long attrs)
> >> {
> >> struct xen_grant_dma_data *data;
> >> - unsigned int i, n_pages = PFN_UP(size);
> >> + unsigned int i, n_pages = PFN_UP(offset + size);
> >
> > Here, why do we use PFN_UP and not XEN_PFN_UP?
> > Also, since the variable 'n_pages' seems to refer to the number of grants
> > (unless I got it all entirely wrong ...), wouldn't it be more suitable to
> call
> > explicitly gnttab_count_grant()?
>
> Good point.
>
>
> +1
>
>
> I think this will need another patch for switching grant-dma-ops.c to
> use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
>
>
> +1
>
> I can create a separate patch for converting on top of this series, it would be
> nice to clarify one point.
>
> So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT
> respectively (where appropriate).
Yes, that would be the idea.
> Should the PFN_UP be converted to XEN_PFN_UP *or* use
> gnttab_count_grant() explicitly? Personally I would prefer the former, but would
> also be ok with the latter.
I agree XEN_PFN_UP would be better, especially as XEN_PAGE_SIZE/XEN_PAGE_SHIFT
will be used in the same functions.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
2022-10-06 10:24 ` Juergen Gross
@ 2022-10-06 10:30 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Tyshchenko @ 2022-10-06 10:30 UTC (permalink / raw)
To: Juergen Gross
Cc: Xenia Ragiadakou, xen-devel, linux-kernel, Oleksandr Tyshchenko,
Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]
On Thu, Oct 6, 2022 at 1:24 PM Juergen Gross <jgross@suse.com> wrote:
Hello Juergen
[sorry for the possible format issues]
On 06.10.22 12:14, Oleksandr Tyshchenko wrote:
> >
> >
> > On Thu, Oct 6, 2022 at 11:05 AM Juergen Gross <jgross@suse.com
> > <mailto:jgross@suse.com>> wrote:
> >
> > On 06.10.22 09:35, Xenia Ragiadakou wrote:
> >
> >
> >
> > Hello Xenia, Juergen
> >
> > [sorry for the possible format issues]
> >
> > >
> > > On 10/5/22 20:48, Oleksandr Tyshchenko wrote:
> > >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com
> > <mailto:oleksandr_tyshchenko@epam.com>>
> > >>
> > >> Take page offset into the account when calculating the number of
> pages
> > >> to be granted.
> > >>
> > >> Signed-off-by: Oleksandr Tyshchenko <
> oleksandr_tyshchenko@epam.com
> > <mailto:oleksandr_tyshchenko@epam.com>>
> > >> Fixes: d6aca3504c7d ("xen/grant-dma-ops: Add option to restrict
> memory
> > access
> > >> under Xen")
> > >> ---
> > >> drivers/xen/grant-dma-ops.c | 5 +++--
> > >> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/xen/grant-dma-ops.c
> b/drivers/xen/grant-dma-ops.c
> > >> index 8973fc1e9ccc..1998d0e8ce82 100644
> > >> --- a/drivers/xen/grant-dma-ops.c
> > >> +++ b/drivers/xen/grant-dma-ops.c
> > >> @@ -153,7 +153,7 @@ static dma_addr_t
> xen_grant_dma_map_page(struct device
> > >> *dev, struct page *page,
> > >> unsigned long attrs)
> > >> {
> > >> struct xen_grant_dma_data *data;
> > >> - unsigned int i, n_pages = PFN_UP(size);
> > >> + unsigned int i, n_pages = PFN_UP(offset + size);
> > >
> > > Here, why do we use PFN_UP and not XEN_PFN_UP?
> > > Also, since the variable 'n_pages' seems to refer to the number
> of grants
> > > (unless I got it all entirely wrong ...), wouldn't it be more
> suitable to
> > call
> > > explicitly gnttab_count_grant()?
> >
> > Good point.
> >
> >
> > +1
> >
> >
> > I think this will need another patch for switching grant-dma-ops.c to
> > use XEN_PAGE_SIZE and XEN_PAGE_SHIFT.
> >
> >
> > +1
> >
> > I can create a separate patch for converting on top of this series, it
> would be
> > nice to clarify one point.
> >
> > So I will convert PAGE_SIZE/PAGE_SHIFT to XEN_PAGE_SIZE/XEN_PAGE_SHIFT
> > respectively (where appropriate).
>
> Yes, that would be the idea.
>
> > Should the PFN_UP be converted to XEN_PFN_UP *or* use
> > gnttab_count_grant() explicitly? Personally I would prefer the former,
> but would
> > also be ok with the latter.
>
> I agree XEN_PFN_UP would be better, especially as
> XEN_PAGE_SIZE/XEN_PAGE_SHIFT
> will be used in the same functions.
>
Thanks for the clarification.
>
>
> Juergen
>
--
Regards,
Oleksandr Tyshchenko
[-- Attachment #2: Type: text/html, Size: 5237 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer
2022-10-05 17:48 [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices Oleksandr Tyshchenko
@ 2022-10-07 5:19 ` Juergen Gross
2 siblings, 0 replies; 13+ messages in thread
From: Juergen Gross @ 2022-10-07 5:19 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel, linux-kernel
Cc: Oleksandr Tyshchenko, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 680 bytes --]
On 05.10.22 19:48, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Hello all.
>
> These are several fixes I collected when playing with virtio-net device
> using Xen grant mappings.
>
> Tested with both virtio-blk and virtio-net devices.
>
> Oleksandr Tyshchenko (2):
> xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page()
> xen/virtio: Fix potential deadlock when accessing
> xen_grant_dma_devices
>
> drivers/xen/grant-dma-ops.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
Series pushed to xen/tip.git for-linus-6.1
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-10-07 5:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-05 17:48 [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 1/2] xen/virtio: Fix n_pages calculation in xen_grant_dma_map(unmap)_page() Oleksandr Tyshchenko
2022-10-06 0:07 ` Stefano Stabellini
2022-10-06 5:06 ` Juergen Gross
2022-10-06 7:35 ` Xenia Ragiadakou
2022-10-06 8:05 ` Juergen Gross
2022-10-06 10:14 ` Oleksandr Tyshchenko
2022-10-06 10:24 ` Juergen Gross
2022-10-06 10:30 ` Oleksandr Tyshchenko
2022-10-05 17:48 ` [PATCH 2/2] xen/virtio: Fix potential deadlock when accessing xen_grant_dma_devices Oleksandr Tyshchenko
2022-10-06 0:19 ` Stefano Stabellini
2022-10-06 5:08 ` Juergen Gross
2022-10-07 5:19 ` [PATCH 0/2] Misc fixes for Xen grant DMA-mapping layer Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).