Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Stefano Stabellini
On Wed, 15 Jun 2022, Juergen Gross wrote:
> Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
> Xen grant mappings") introduced a new requirement for using virtio
> devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
> feature.
> 
> This is an undue requirement for non-PV guests, as those can be operated
> with existing backends without any problem, as long as those backends
> are running in dom0.
> 
> Per default allow virtio devices without grant support for non-PV
> guests.
> 
> The setting can be overridden by using the new "xen_virtio_grant"
> command line parameter.
> 
> Add a new config item to always force use of grants for virtio.
> 
> Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen 
> grant mappings")
> Signed-off-by: Juergen Gross 
> ---
>  .../admin-guide/kernel-parameters.txt |  6 +
>  drivers/xen/Kconfig   |  9 
>  drivers/xen/grant-dma-ops.c   | 22 +++
>  include/xen/xen.h | 12 +-
>  4 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index 8090130b544b..7960480c6fe4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6695,6 +6695,12 @@
>   improve timer resolution at the expense of processing
>   more timer interrupts.
>  
> + xen_virtio_grant= [XEN]
> + Control whether virtio devices are required to use
> + grants when running as a Xen guest. The default is
> + "yes" for PV guests or when the kernel has been built
> + with CONFIG_XEN_VIRTIO_FORCE_GRANT set.
> +
>   xen.balloon_boot_timeout= [XEN]
>   The time (in seconds) to wait before giving up to boot
>   in case initial ballooning fails to free enough memory.
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index bfd5f4f706bc..a65bd92121a5 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -355,4 +355,13 @@ config XEN_VIRTIO
>  
> If in doubt, say n.
>  
> +config XEN_VIRTIO_FORCE_GRANT
> + bool "Require Xen virtio support to use grants"
> + depends on XEN_VIRTIO
> + help
> +   Require virtio for Xen guests to use grant mappings.
> +   This will avoid the need to give the backend the right to map all
> +   of the guest memory. This will need support on the backend side
> +   (e.g. qemu or kernel, depending on the virtio device types used).
> +
>  endmenu
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index fc0142484001..d1fae789dfad 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -11,6 +11,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -27,6 +28,27 @@ static DEFINE_XARRAY(xen_grant_dma_devices);
>  
>  #define XEN_GRANT_DMA_ADDR_OFF   (1ULL << 63)
>  
> +static bool __initdata xen_virtio_grants;
> +static bool __initdata xen_virtio_grants_set;
> +static __init int parse_use_grants(char *arg)
> +{
> + if (!strcmp(arg, "yes"))
> + xen_virtio_grants = true;
> + else if (!strcmp(arg, "no"))
> + xen_virtio_grants = false;
> + xen_virtio_grants_set = true;
> +
> + return 0;
> +}
> +early_param("xen_virtio_grant", parse_use_grants);
> +
> +void xen_set_restricted_virtio_memory_access(void)
> +{
> + if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_virtio_grants ||
> + (!xen_virtio_grants_set && xen_pv_domain()))
> + platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> +}

I agree with Christoph on this.

On ARM all guests are HVM guests. Unless I am reading this wrongly, with
this check, the user needs to pass the xen_virtio_grant command line
option or add CONFIG_XEN_VIRTIO_FORCE_GRANT to the build to use virtio
with grants. Instead, it should be automatic.

I am not against adding new command line or compile-time options. But
on ARM we already have all the information we need in device tree with
"iommus" and "xen,grant-dma". We don't need anything more.

On ARM if "xen,grant-dma" is present we need to enable
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS, otherwise we don't.

So I think it should be something like the appended (untested):

- on ARM we call xen_set_restricted_virtio_memory_access if
  xen,grant-dma is present in device tree
- on x86 ideally we would have something like xen,grant-dma in a Xen
  ACPI table, but for now:
- always restrict for PV guests (no change)
- only restrict for HVM guests if a new cmdline option is passed
- so the command line option is only for Xen x86 HVM guests
- no need for another build-time option

diff --git a/D

Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Oleksandr



On 15.06.22 11:48, Juergen Gross wrote:

Hello Juergen


Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
Xen grant mappings") introduced a new requirement for using virtio
devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
feature.

This is an undue requirement for non-PV guests, as those can be operated
with existing backends without any problem, as long as those backends
are running in dom0.

Per default allow virtio devices without grant support for non-PV
guests.

The setting can be overridden by using the new "xen_virtio_grant"
command line parameter.

Add a new config item to always force use of grants for virtio.

Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen grant 
mappings")
Signed-off-by: Juergen Gross 


Thank you for the fix.


I have tested it on Arm64 guest (XEN_HVM_DOMAIN), it works.

With the "__init" fix (pointed out by Viresh) applied you can add my:

Tested-by: Oleksandr Tyshchenko  #Arm64 only


[snip]


--
Regards,

Oleksandr Tyshchenko




Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Juergen Gross

On 15.06.22 15:43, Christoph Hellwig wrote:

On Wed, Jun 15, 2022 at 02:53:54PM +0200, Juergen Gross wrote:

On 15.06.22 13:54, Christoph Hellwig wrote:

On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:

No, it doesn't. I'm working on a qemu patch series enabling the qemu
based backends to support grants with virtio. The code is working fine
on x86, too (apart from the fact that the backends aren't ready yet).


The code right now in mainline only ever sets the ops for DMA.  So
I can't see how you could make it work.


Ah, you are right. I was using a guest with an older version of the series.
Sorry for the noise.


No problem.  But whatever you end up using to enable the grant DMA
ops n x86 should also require the platform access feature.  We already
have that information so we can make use of it.


Yes, of course.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Christoph Hellwig
On Wed, Jun 15, 2022 at 02:53:54PM +0200, Juergen Gross wrote:
> On 15.06.22 13:54, Christoph Hellwig wrote:
> > On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
> > > No, it doesn't. I'm working on a qemu patch series enabling the qemu
> > > based backends to support grants with virtio. The code is working fine
> > > on x86, too (apart from the fact that the backends aren't ready yet).
> > 
> > The code right now in mainline only ever sets the ops for DMA.  So
> > I can't see how you could make it work.
> 
> Ah, you are right. I was using a guest with an older version of the series.
> Sorry for the noise.

No problem.  But whatever you end up using to enable the grant DMA
ops n x86 should also require the platform access feature.  We already
have that information so we can make use of it.



Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Juergen Gross

On 15.06.22 13:54, Christoph Hellwig wrote:

On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:

No, it doesn't. I'm working on a qemu patch series enabling the qemu
based backends to support grants with virtio. The code is working fine
on x86, too (apart from the fact that the backends aren't ready yet).


The code right now in mainline only ever sets the ops for DMA.  So
I can't see how you could make it work.


Ah, you are right. I was using a guest with an older version of the series.
Sorry for the noise.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Christoph Hellwig
On Wed, Jun 15, 2022 at 01:39:01PM +0200, Juergen Gross wrote:
> No, it doesn't. I'm working on a qemu patch series enabling the qemu
> based backends to support grants with virtio. The code is working fine
> on x86, too (apart from the fact that the backends aren't ready yet).

The code right now in mainline only ever sets the ops for DMA.  So
I can't see how you could make it work.



Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Juergen Gross

On 15.06.22 13:28, Christoph Hellwig wrote:

On Wed, Jun 15, 2022 at 01:26:27PM +0200, Juergen Gross wrote:

On 15.06.22 13:24, Christoph Hellwig wrote:

I don't think this command line mess is a good idea.  Instead the
brand new grant devices need to be properly advertised in the device
tree, and any device that isn't a grant device doesn't need
VIRTIO_F_ACCESS_PLATFORM.  No need for a command line or user
intervention.


And on x86?


ACPI tables or whatever mechanism you otherwise use to advertise grant
based DMA.  But that is irrelevant for now, as the code in mainline
only supports DT on arm/arm64 anyay.


No, it doesn't. I'm working on a qemu patch series enabling the qemu
based backends to support grants with virtio. The code is working fine
on x86, too (apart from the fact that the backends aren't ready yet).

I'd be fine to just drop the command line parameter, but I think a
guest admin should be able to specify that he doesn't want the backend
to be able to map arbitrary memory of the guest in order to add
another line of defense.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Christoph Hellwig
On Wed, Jun 15, 2022 at 01:26:27PM +0200, Juergen Gross wrote:
> On 15.06.22 13:24, Christoph Hellwig wrote:
> > I don't think this command line mess is a good idea.  Instead the
> > brand new grant devices need to be properly advertised in the device
> > tree, and any device that isn't a grant device doesn't need
> > VIRTIO_F_ACCESS_PLATFORM.  No need for a command line or user
> > intervention.
> 
> And on x86?

ACPI tables or whatever mechanism you otherwise use to advertise grant
based DMA.  But that is irrelevant for now, as the code in mainline
only supports DT on arm/arm64 anyay.



Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Juergen Gross

On 15.06.22 13:24, Christoph Hellwig wrote:

I don't think this command line mess is a good idea.  Instead the
brand new grant devices need to be properly advertised in the device
tree, and any device that isn't a grant device doesn't need
VIRTIO_F_ACCESS_PLATFORM.  No need for a command line or user
intervention.


And on x86?


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Christoph Hellwig
I don't think this command line mess is a good idea.  Instead the
brand new grant devices need to be properly advertised in the device
tree, and any device that isn't a grant device doesn't need
VIRTIO_F_ACCESS_PLATFORM.  No need for a command line or user
intervention.




Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Juergen Gross

On 15.06.22 11:25, Viresh Kumar wrote:

On 15-06-22, 10:48, Juergen Gross wrote:

Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
Xen grant mappings") introduced a new requirement for using virtio
devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
feature.

This is an undue requirement for non-PV guests, as those can be operated
with existing backends without any problem, as long as those backends
are running in dom0.

Per default allow virtio devices without grant support for non-PV
guests.

The setting can be overridden by using the new "xen_virtio_grant"
command line parameter.

Add a new config item to always force use of grants for virtio.

Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen grant 
mappings")
Signed-off-by: Juergen Gross 
---
  .../admin-guide/kernel-parameters.txt |  6 +
  drivers/xen/Kconfig   |  9 
  drivers/xen/grant-dma-ops.c   | 22 +++
  include/xen/xen.h | 12 +-
  4 files changed, 42 insertions(+), 7 deletions(-)


Thanks for the quick fix.

With CONFIG_DEBUG_SECTION_MISMATCH=y, this generates a warning.

WARNING: modpost: vmlinux.o(.text+0x7a8270): Section mismatch in reference from 
the function xen_set_restricted_virtio_memory_access() to the variable 
.init.data:xen_virtio_grants
The function xen_set_restricted_virtio_memory_access() references
the variable __initdata xen_virtio_grants.
This is often because xen_set_restricted_virtio_memory_access lacks a __initdata
annotation or the annotation of xen_virtio_grants is wrong.


Silly me. Thanks for the notice.



This can be fixed by:

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index d1fae789dfad..1099097b4515 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -42,7 +42,7 @@ static __init int parse_use_grants(char *arg)
  }
  early_param("xen_virtio_grant", parse_use_grants);

-void xen_set_restricted_virtio_memory_access(void)
+void __init xen_set_restricted_virtio_memory_access(void)
  {
 if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_virtio_grants ||
 (!xen_virtio_grants_set && xen_pv_domain()))

With that:

Tested-by: Viresh Kumar 



Thanks,


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature


Re: [PATCH] xen: don't require virtio with grants for non-PV guests

2022-06-15 Thread Viresh Kumar
On 15-06-22, 10:48, Juergen Gross wrote:
> Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
> Xen grant mappings") introduced a new requirement for using virtio
> devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
> feature.
> 
> This is an undue requirement for non-PV guests, as those can be operated
> with existing backends without any problem, as long as those backends
> are running in dom0.
> 
> Per default allow virtio devices without grant support for non-PV
> guests.
> 
> The setting can be overridden by using the new "xen_virtio_grant"
> command line parameter.
> 
> Add a new config item to always force use of grants for virtio.
> 
> Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen 
> grant mappings")
> Signed-off-by: Juergen Gross 
> ---
>  .../admin-guide/kernel-parameters.txt |  6 +
>  drivers/xen/Kconfig   |  9 
>  drivers/xen/grant-dma-ops.c   | 22 +++
>  include/xen/xen.h | 12 +-
>  4 files changed, 42 insertions(+), 7 deletions(-)

Thanks for the quick fix.

With CONFIG_DEBUG_SECTION_MISMATCH=y, this generates a warning.

WARNING: modpost: vmlinux.o(.text+0x7a8270): Section mismatch in reference from 
the function xen_set_restricted_virtio_memory_access() to the variable 
.init.data:xen_virtio_grants
The function xen_set_restricted_virtio_memory_access() references
the variable __initdata xen_virtio_grants.
This is often because xen_set_restricted_virtio_memory_access lacks a __initdata
annotation or the annotation of xen_virtio_grants is wrong.

This can be fixed by:

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index d1fae789dfad..1099097b4515 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -42,7 +42,7 @@ static __init int parse_use_grants(char *arg)
 }
 early_param("xen_virtio_grant", parse_use_grants);

-void xen_set_restricted_virtio_memory_access(void)
+void __init xen_set_restricted_virtio_memory_access(void)
 {
if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_virtio_grants ||
(!xen_virtio_grants_set && xen_pv_domain()))

With that:

Tested-by: Viresh Kumar 

-- 
viresh