2022-10-06 07:19:30

by Jürgen Groß

[permalink] [raw]
Subject: [PATCH 0/3] xen/virtio: support grant based virtio on x86

Add basic support for virtio with grants on x86 by defaulting the
backend to be in dom0 in the case the guest kernel was built with
CONFIG_XEN_VIRTIO_FORCE_GRANT.

Juergen Gross (3):
xen/virtio: restructure xen grant dma setup
xen/virtio: use dom0 as default backend for
CONFIG_XEN_VIRTIO_FORCE_GRANT
xen/virtio: enable grant based virtio on x86

arch/x86/xen/enlighten_hvm.c | 2 +-
arch/x86/xen/enlighten_pv.c | 2 +-
drivers/xen/grant-dma-ops.c | 81 +++++++++++++++++++++++++-----------
include/xen/xen-ops.h | 1 +
4 files changed, 59 insertions(+), 27 deletions(-)

--
2.35.3


2022-10-06 07:19:52

by Jürgen Groß

[permalink] [raw]
Subject: [PATCH 1/3] xen/virtio: restructure xen grant dma setup

In order to prepare supporting other means than device tree for
setting up virtio devices under Xen, restructure the functions
xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit.

Signed-off-by: Juergen Gross <[email protected]>
---
drivers/xen/grant-dma-ops.c | 68 +++++++++++++++++++++++--------------
1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 8973fc1e9ccc..f29759d5301f 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -273,22 +273,28 @@ static const struct dma_map_ops xen_grant_dma_ops = {
.dma_supported = xen_grant_dma_supported,
};

-bool xen_is_grant_dma_device(struct device *dev)
+static bool xen_is_dt_grant_dma_device(struct device *dev)
{
struct device_node *iommu_np;
bool has_iommu;

- /* XXX Handle only DT devices for now */
- if (!dev->of_node)
- return false;
-
iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
- has_iommu = iommu_np && of_device_is_compatible(iommu_np, "xen,grant-dma");
+ has_iommu = iommu_np &&
+ of_device_is_compatible(iommu_np, "xen,grant-dma");
of_node_put(iommu_np);

return has_iommu;
}

+bool xen_is_grant_dma_device(struct device *dev)
+{
+ /* XXX Handle only DT devices for now */
+ if (dev->of_node)
+ return xen_is_dt_grant_dma_device(dev);
+
+ return false;
+}
+
bool xen_virtio_mem_acc(struct virtio_device *dev)
{
if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
@@ -297,45 +303,56 @@ bool xen_virtio_mem_acc(struct virtio_device *dev)
return xen_is_grant_dma_device(dev->dev.parent);
}

-void xen_grant_setup_dma_ops(struct device *dev)
+static int xen_dt_grant_setup_dma_ops(struct device *dev,
+ struct xen_grant_dma_data *data)
{
- struct xen_grant_dma_data *data;
struct of_phandle_args iommu_spec;

- data = find_xen_grant_dma_data(dev);
- if (data) {
- dev_err(dev, "Xen grant DMA data is already created\n");
- return;
- }
-
- /* XXX ACPI device unsupported for now */
- if (!dev->of_node)
- goto err;
-
if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
0, &iommu_spec)) {
dev_err(dev, "Cannot parse iommus property\n");
- goto err;
+ return -ESRCH;
}

if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
iommu_spec.args_count != 1) {
dev_err(dev, "Incompatible IOMMU node\n");
of_node_put(iommu_spec.np);
- goto err;
+ return -ESRCH;
}

of_node_put(iommu_spec.np);

+ /*
+ * The endpoint ID here means the ID of the domain where the
+ * corresponding backend is running
+ */
+ data->backend_domid = iommu_spec.args[0];
+
+ return 0;
+}
+
+void xen_grant_setup_dma_ops(struct device *dev)
+{
+ struct xen_grant_dma_data *data;
+
+ data = find_xen_grant_dma_data(dev);
+ if (data) {
+ dev_err(dev, "Xen grant DMA data is already created\n");
+ return;
+ }
+
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
goto err;

- /*
- * The endpoint ID here means the ID of the domain where the corresponding
- * backend is running
- */
- data->backend_domid = iommu_spec.args[0];
+ if (dev->of_node) {
+ if (xen_dt_grant_setup_dma_ops(dev, data))
+ goto err;
+ } else {
+ /* XXX ACPI device unsupported for now */
+ goto err;
+ }

if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
GFP_KERNEL))) {
@@ -348,6 +365,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
return;

err:
+ devm_kfree(dev, data);
dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n");
}

--
2.35.3

2022-10-06 08:03:10

by Jürgen Groß

[permalink] [raw]
Subject: [PATCH 2/3] xen/virtio: use dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT

With CONFIG_XEN_VIRTIO_FORCE_GRANT set the default backend domid to 0,
enabling to use xen_grant_dma_ops for those devices.

Signed-off-by: Juergen Gross <[email protected]>
---
drivers/xen/grant-dma-ops.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index f29759d5301f..a00112235877 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -349,6 +349,9 @@ void xen_grant_setup_dma_ops(struct device *dev)
if (dev->of_node) {
if (xen_dt_grant_setup_dma_ops(dev, data))
goto err;
+ } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
+ dev_info(dev, "Using dom0 as backend\n");
+ data->backend_domid = 0;
} else {
/* XXX ACPI device unsupported for now */
goto err;
--
2.35.3

2022-10-06 08:12:36

by Jürgen Groß

[permalink] [raw]
Subject: [PATCH 3/3] xen/virtio: enable grant based virtio on x86

Use an x86-specific virtio_check_mem_acc_cb() for Xen in order to setup
the correct DMA ops.

Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/xen/enlighten_hvm.c | 2 +-
arch/x86/xen/enlighten_pv.c | 2 +-
drivers/xen/grant-dma-ops.c | 10 ++++++++++
include/xen/xen-ops.h | 1 +
4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 1c1ac418484b..c1cd28e915a3 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -212,7 +212,7 @@ static void __init xen_hvm_guest_init(void)
return;

if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
- virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
+ virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);

init_hvm_pv_info();

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 9b1a58dda935..45b24c1b646a 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -112,7 +112,7 @@ static void __init xen_pv_init_platform(void)
{
/* PV guests can't operate virtio devices without grants. */
if (IS_ENABLED(CONFIG_XEN_VIRTIO))
- virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
+ virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);

populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index a00112235877..60a7acc334ed 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -372,6 +372,16 @@ void xen_grant_setup_dma_ops(struct device *dev)
dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n");
}

+bool xen_virtio_restricted_mem_acc(struct virtio_device *dev)
+{
+ bool ret = xen_virtio_mem_acc(dev);
+
+ if (ret)
+ xen_grant_setup_dma_ops(dev->dev.parent);
+
+ return ret;
+}
+
MODULE_DESCRIPTION("Xen grant DMA-mapping layer");
MODULE_AUTHOR("Juergen Gross <[email protected]>");
MODULE_LICENSE("GPL");
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index dae0f350c678..3dd5aa936f1d 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -219,6 +219,7 @@ static inline void xen_preemptible_hcall_end(void) { }
void xen_grant_setup_dma_ops(struct device *dev);
bool xen_is_grant_dma_device(struct device *dev);
bool xen_virtio_mem_acc(struct virtio_device *dev);
+bool xen_virtio_restricted_mem_acc(struct virtio_device *dev);
#else
static inline void xen_grant_setup_dma_ops(struct device *dev)
{
--
2.35.3

2022-10-06 16:21:50

by Oleksandr Tyshchenko

[permalink] [raw]
Subject: Re: [PATCH 3/3] xen/virtio: enable grant based virtio on x86


On 06.10.22 10:15, Juergen Gross wrote:


Hello Juergen

> Use an x86-specific virtio_check_mem_acc_cb() for Xen in order to setup
> the correct DMA ops.
>
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> arch/x86/xen/enlighten_hvm.c | 2 +-
> arch/x86/xen/enlighten_pv.c | 2 +-
> drivers/xen/grant-dma-ops.c | 10 ++++++++++
> include/xen/xen-ops.h | 1 +
> 4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> index 1c1ac418484b..c1cd28e915a3 100644
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -212,7 +212,7 @@ static void __init xen_hvm_guest_init(void)
> return;
>
> if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
> - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
> + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);
>
> init_hvm_pv_info();
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 9b1a58dda935..45b24c1b646a 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -112,7 +112,7 @@ static void __init xen_pv_init_platform(void)
> {
> /* PV guests can't operate virtio devices without grants. */
> if (IS_ENABLED(CONFIG_XEN_VIRTIO))
> - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
> + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);
>
> populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index a00112235877..60a7acc334ed 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -372,6 +372,16 @@ void xen_grant_setup_dma_ops(struct device *dev)
> dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n");
> }
>
> +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev)
> +{
> + bool ret = xen_virtio_mem_acc(dev);


The grant usage is mandatory for PV guests, right?

Then xen_virtio_mem_acc() should always return true for PV guests (I
mean even if CONFIG_XEN_VIRTIO_FORCE_GRANT is not set).



> +
> + if (ret)
> + xen_grant_setup_dma_ops(dev->dev.parent);
> +
> + return ret;
> +}
> +
> MODULE_DESCRIPTION("Xen grant DMA-mapping layer");
> MODULE_AUTHOR("Juergen Gross <[email protected]>");
> MODULE_LICENSE("GPL");
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index dae0f350c678..3dd5aa936f1d 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -219,6 +219,7 @@ static inline void xen_preemptible_hcall_end(void) { }
> void xen_grant_setup_dma_ops(struct device *dev);
> bool xen_is_grant_dma_device(struct device *dev);
> bool xen_virtio_mem_acc(struct virtio_device *dev);
> +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev);
> #else
> static inline void xen_grant_setup_dma_ops(struct device *dev)
> {


And probably static inline stub always returning false if
CONFIG_XEN_GRANT_DMA_OPS is not set.


--
Regards,

Oleksandr Tyshchenko

2022-10-06 16:57:59

by Oleksandr Tyshchenko

[permalink] [raw]
Subject: Re: [PATCH 1/3] xen/virtio: restructure xen grant dma setup


On 06.10.22 10:14, Juergen Gross wrote:

Hello Juergen

> In order to prepare supporting other means than device tree for
> setting up virtio devices under Xen, restructure the functions
> xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit.
>
> Signed-off-by: Juergen Gross <[email protected]>


Patch looks good,

one NIT, xen_dt_grant_setup_dma_ops() down the code doesn't actually
setup DMA OPS, it retrieves the backend domid via device-tree means and
stores it,

so I would rename to it, maybe something like
xen_dt_grant_setup_backend_domid() or xen_dt_grant_init_backend_domid(),
but I am not sure it would be good alternative.


So, w/ or w/o renaming:

Reviewed-by: Oleksandr Tyshchenko <[email protected]>

also

Tested-by: Oleksandr Tyshchenko <[email protected]> # Arm64
only


> ---
> drivers/xen/grant-dma-ops.c | 68 +++++++++++++++++++++++--------------
> 1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..f29759d5301f 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -273,22 +273,28 @@ static const struct dma_map_ops xen_grant_dma_ops = {
> .dma_supported = xen_grant_dma_supported,
> };
>
> -bool xen_is_grant_dma_device(struct device *dev)
> +static bool xen_is_dt_grant_dma_device(struct device *dev)
> {
> struct device_node *iommu_np;
> bool has_iommu;
>
> - /* XXX Handle only DT devices for now */
> - if (!dev->of_node)
> - return false;
> -
> iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
> - has_iommu = iommu_np && of_device_is_compatible(iommu_np, "xen,grant-dma");
> + has_iommu = iommu_np &&
> + of_device_is_compatible(iommu_np, "xen,grant-dma");
> of_node_put(iommu_np);
>
> return has_iommu;
> }
>
> +bool xen_is_grant_dma_device(struct device *dev)
> +{
> + /* XXX Handle only DT devices for now */
> + if (dev->of_node)
> + return xen_is_dt_grant_dma_device(dev);
> +
> + return false;
> +}
> +
> bool xen_virtio_mem_acc(struct virtio_device *dev)
> {
> if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
> @@ -297,45 +303,56 @@ bool xen_virtio_mem_acc(struct virtio_device *dev)
> return xen_is_grant_dma_device(dev->dev.parent);
> }
>
> -void xen_grant_setup_dma_ops(struct device *dev)
> +static int xen_dt_grant_setup_dma_ops(struct device *dev,
> + struct xen_grant_dma_data *data)
> {
> - struct xen_grant_dma_data *data;
> struct of_phandle_args iommu_spec;
>
> - data = find_xen_grant_dma_data(dev);
> - if (data) {
> - dev_err(dev, "Xen grant DMA data is already created\n");
> - return;
> - }
> -
> - /* XXX ACPI device unsupported for now */
> - if (!dev->of_node)
> - goto err;
> -
> if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
> 0, &iommu_spec)) {
> dev_err(dev, "Cannot parse iommus property\n");
> - goto err;
> + return -ESRCH;
> }
>
> if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
> iommu_spec.args_count != 1) {
> dev_err(dev, "Incompatible IOMMU node\n");
> of_node_put(iommu_spec.np);
> - goto err;
> + return -ESRCH;
> }
>
> of_node_put(iommu_spec.np);
>
> + /*
> + * The endpoint ID here means the ID of the domain where the
> + * corresponding backend is running
> + */
> + data->backend_domid = iommu_spec.args[0];
> +
> + return 0;
> +}
> +
> +void xen_grant_setup_dma_ops(struct device *dev)
> +{
> + struct xen_grant_dma_data *data;
> +
> + data = find_xen_grant_dma_data(dev);
> + if (data) {
> + dev_err(dev, "Xen grant DMA data is already created\n");
> + return;
> + }
> +
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> goto err;
>
> - /*
> - * The endpoint ID here means the ID of the domain where the corresponding
> - * backend is running
> - */
> - data->backend_domid = iommu_spec.args[0];
> + if (dev->of_node) {
> + if (xen_dt_grant_setup_dma_ops(dev, data))
> + goto err;
> + } else {
> + /* XXX ACPI device unsupported for now */
> + goto err;
> + }
>
> if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
> GFP_KERNEL))) {
> @@ -348,6 +365,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
> return;
>
> err:
> + devm_kfree(dev, data);
> dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n");
> }
>

--
Regards,

Oleksandr Tyshchenko

2022-10-06 17:47:14

by Oleksandr Tyshchenko

[permalink] [raw]
Subject: Re: [PATCH 2/3] xen/virtio: use dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT


On 06.10.22 10:14, Juergen Gross wrote:

Hello Juergen

> With CONFIG_XEN_VIRTIO_FORCE_GRANT set the default backend domid to 0,
> enabling to use xen_grant_dma_ops for those devices.
>
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> drivers/xen/grant-dma-ops.c | 3 +++
> 1 file changed, 3 insertions(+)


Reviewed-by: Oleksandr Tyshchenko <[email protected]>

>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index f29759d5301f..a00112235877 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -349,6 +349,9 @@ void xen_grant_setup_dma_ops(struct device *dev)
> if (dev->of_node) {
> if (xen_dt_grant_setup_dma_ops(dev, data))
> goto err;
> + } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
> + dev_info(dev, "Using dom0 as backend\n");
> + data->backend_domid = 0;
> } else {
> /* XXX ACPI device unsupported for now */
> goto err;

--
Regards,

Oleksandr Tyshchenko

2022-10-07 00:37:43

by Stefano Stabellini

[permalink] [raw]
Subject: Re: [PATCH 2/3] xen/virtio: use dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT

On Thu, 6 Oct 2022, Oleksandr Tyshchenko wrote:
> On 06.10.22 10:14, Juergen Gross wrote:
>
> Hello Juergen
>
> > With CONFIG_XEN_VIRTIO_FORCE_GRANT set the default backend domid to 0,
> > enabling to use xen_grant_dma_ops for those devices.
> >
> > Signed-off-by: Juergen Gross <[email protected]>
> > ---
> > drivers/xen/grant-dma-ops.c | 3 +++
> > 1 file changed, 3 insertions(+)
>
>
> Reviewed-by: Oleksandr Tyshchenko <[email protected]>

Acked-by: Stefano Stabellini <[email protected]>


> > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> > index f29759d5301f..a00112235877 100644
> > --- a/drivers/xen/grant-dma-ops.c
> > +++ b/drivers/xen/grant-dma-ops.c
> > @@ -349,6 +349,9 @@ void xen_grant_setup_dma_ops(struct device *dev)
> > if (dev->of_node) {
> > if (xen_dt_grant_setup_dma_ops(dev, data))
> > goto err;
> > + } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
> > + dev_info(dev, "Using dom0 as backend\n");
> > + data->backend_domid = 0;
> > } else {
> > /* XXX ACPI device unsupported for now */
> > goto err;
>
> --
> Regards,
>
> Oleksandr Tyshchenko
>

2022-10-07 00:37:43

by Stefano Stabellini

[permalink] [raw]
Subject: Re: [PATCH 1/3] xen/virtio: restructure xen grant dma setup

On Thu, 6 Oct 2022, Oleksandr Tyshchenko wrote:
> On 06.10.22 10:14, Juergen Gross wrote:
>
> Hello Juergen
>
> > In order to prepare supporting other means than device tree for
> > setting up virtio devices under Xen, restructure the functions
> > xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit.
> >
> > Signed-off-by: Juergen Gross <[email protected]>
>
>
> Patch looks good,
>
> one NIT, xen_dt_grant_setup_dma_ops() down the code doesn't actually
> setup DMA OPS, it retrieves the backend domid via device-tree means and
> stores it,
>
> so I would rename to it, maybe something like
> xen_dt_grant_setup_backend_domid() or xen_dt_grant_init_backend_domid(),
> but I am not sure it would be good alternative.
>
>
> So, w/ or w/o renaming:
>
> Reviewed-by: Oleksandr Tyshchenko <[email protected]>
>
> also
>
> Tested-by: Oleksandr Tyshchenko <[email protected]> # Arm64
> only

Acked-by: Stefano Stabellini <[email protected]>

2022-10-07 04:48:57

by Jürgen Groß

[permalink] [raw]
Subject: Re: [PATCH 1/3] xen/virtio: restructure xen grant dma setup

On 06.10.22 18:35, Oleksandr Tyshchenko wrote:
>
> On 06.10.22 10:14, Juergen Gross wrote:
>
> Hello Juergen
>
>> In order to prepare supporting other means than device tree for
>> setting up virtio devices under Xen, restructure the functions
>> xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit.
>>
>> Signed-off-by: Juergen Gross <[email protected]>
>
>
> Patch looks good,
>
> one NIT, xen_dt_grant_setup_dma_ops() down the code doesn't actually
> setup DMA OPS, it retrieves the backend domid via device-tree means and
> stores it,
>
> so I would rename to it, maybe something like
> xen_dt_grant_setup_backend_domid() or xen_dt_grant_init_backend_domid(),
> but I am not sure it would be good alternative.

I'll go with xen_dt_grant_init_backend_domid().

>
>
> So, w/ or w/o renaming:
>
> Reviewed-by: Oleksandr Tyshchenko <[email protected]>
>
> also
>
> Tested-by: Oleksandr Tyshchenko <[email protected]> # Arm64
> only

Thanks,


Juergen


Attachments:
OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key
OpenPGP_signature (505.00 B)
OpenPGP digital signature
Download all attachments

2022-10-07 04:57:51

by Jürgen Groß

[permalink] [raw]
Subject: Re: [PATCH 3/3] xen/virtio: enable grant based virtio on x86

On 06.10.22 18:04, Oleksandr Tyshchenko wrote:
>
> On 06.10.22 10:15, Juergen Gross wrote:
>
>
> Hello Juergen
>
>> Use an x86-specific virtio_check_mem_acc_cb() for Xen in order to setup
>> the correct DMA ops.
>>
>> Signed-off-by: Juergen Gross <[email protected]>
>> ---
>> arch/x86/xen/enlighten_hvm.c | 2 +-
>> arch/x86/xen/enlighten_pv.c | 2 +-
>> drivers/xen/grant-dma-ops.c | 10 ++++++++++
>> include/xen/xen-ops.h | 1 +
>> 4 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
>> index 1c1ac418484b..c1cd28e915a3 100644
>> --- a/arch/x86/xen/enlighten_hvm.c
>> +++ b/arch/x86/xen/enlighten_hvm.c
>> @@ -212,7 +212,7 @@ static void __init xen_hvm_guest_init(void)
>> return;
>>
>> if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
>> - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
>> + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);
>>
>> init_hvm_pv_info();
>>
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index 9b1a58dda935..45b24c1b646a 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -112,7 +112,7 @@ static void __init xen_pv_init_platform(void)
>> {
>> /* PV guests can't operate virtio devices without grants. */
>> if (IS_ENABLED(CONFIG_XEN_VIRTIO))
>> - virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
>> + virtio_set_mem_acc_cb(xen_virtio_restricted_mem_acc);
>>
>> populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index a00112235877..60a7acc334ed 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -372,6 +372,16 @@ void xen_grant_setup_dma_ops(struct device *dev)
>> dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA ops\n");
>> }
>>
>> +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev)
>> +{
>> + bool ret = xen_virtio_mem_acc(dev);
>
>
> The grant usage is mandatory for PV guests, right?
>
> Then xen_virtio_mem_acc() should always return true for PV guests (I
> mean even if CONFIG_XEN_VIRTIO_FORCE_GRANT is not set).

Yes.

>
>
>
>> +
>> + if (ret)
>> + xen_grant_setup_dma_ops(dev->dev.parent);
>> +
>> + return ret;
>> +}
>> +
>> MODULE_DESCRIPTION("Xen grant DMA-mapping layer");
>> MODULE_AUTHOR("Juergen Gross <[email protected]>");
>> MODULE_LICENSE("GPL");
>> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
>> index dae0f350c678..3dd5aa936f1d 100644
>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -219,6 +219,7 @@ static inline void xen_preemptible_hcall_end(void) { }
>> void xen_grant_setup_dma_ops(struct device *dev);
>> bool xen_is_grant_dma_device(struct device *dev);
>> bool xen_virtio_mem_acc(struct virtio_device *dev);
>> +bool xen_virtio_restricted_mem_acc(struct virtio_device *dev);
>> #else
>> static inline void xen_grant_setup_dma_ops(struct device *dev)
>> {
>
>
> And probably static inline stub always returning false if
> CONFIG_XEN_GRANT_DMA_OPS is not set.

Indeed.


Juergen


Attachments:
OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key
OpenPGP_signature (505.00 B)
OpenPGP digital signature
Download all attachments