Re: [PATCH] xen: don't require virtio with grants for non-PV guests
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
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
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
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
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
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
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
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
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
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
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
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