Messages in this thread | | | Date | Fri, 17 Jun 2022 07:49:05 +0200 | From | Juergen Gross <> | Subject | Re: [PATCH v2] xen: don't require virtio with grants for non-PV guests |
| |
On 17.06.22 02:03, Stefano Stabellini wrote: > On Thu, 16 Jun 2022, Oleksandr wrote: >> On 16.06.22 11:56, Juergen Gross wrote: >>> On 16.06.22 09:31, Oleksandr wrote: >>>> >>>> On 16.06.22 08:37, 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. >>>>> >>>>> 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") >>>>> Reported-by: Viresh Kumar <viresh.kumar@linaro.org> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>> --- >>>>> V2: >>>>> - remove command line parameter (Christoph Hellwig) >>>>> --- >>>>> drivers/xen/Kconfig | 9 +++++++++ >>>>> include/xen/xen.h | 2 +- >>>>> 2 files changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> 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/include/xen/xen.h b/include/xen/xen.h >>>>> index 0780a81e140d..4d4188f20337 100644 >>>>> --- a/include/xen/xen.h >>>>> +++ b/include/xen/xen.h >>>>> @@ -56,7 +56,7 @@ extern u64 xen_saved_max_mem_size; >>>>> static inline void xen_set_restricted_virtio_memory_access(void) >>>>> { >>>>> - if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain()) >>>>> + if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || xen_pv_domain()) >>>>> platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); >>>> >>>> >>>> Looks like, the flag will be *always* set for paravirtualized guests even >>>> if CONFIG_XEN_VIRTIO disabled. >>>> >>>> Maybe we should clarify the check? >>>> >>>> >>>> if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || >>>> IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_pv_domain()) >>>> >>>> platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); >>>> >>> >>> Yes, we should. I had the function in grant-dma-ops.c in V1, and could drop >>> the >>> CONFIG_XEN_VIRTIO dependency for that reason. >>> >>> I'll wait for more comments before sending V3, though. >> >> ok >> >> >> >> Please note, I am happy with current patch and it works in my Arm64 based >> environment. >> >> Just one moment to consider. >> >> >> As it was already mentioned earlier in current thread the >> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS (former >> arch_has_restricted_virtio_memory_access()) is not per device but about the >> whole guest. Being set it makes VIRTIO_F_ACCESS_PLATFORM and >> VIRTIO_F_VERSION_1 features mandatory for *all* virtio devices in the guest. >> >> The question is “Do we want/need to lift this restriction for some devices >> (which backends are trusted so can access all guest memory) at the same time”? >> Copy here the original Viresh's question for the convenience: >> >> "I understand from your email that the backends need to offer the >> VIRTIO_F_ACCESS_PLATFORM flag now, but should this requirement be a bit soft? >> I mean shouldn't we allow both types of backends to run with the same kernel, >> ones that offer this feature and others that don't? The ones that don't offer >> the feature, should continue to work like they used to, i.e. without the >> restricted memory access feature." >> >> Technically this can be possible with HVM. >> >> Let's imagine the following situation: >> >> - Dom0 with backends which don't offer required features for some reason(s) >> >> But running in Dom0 (trusted domain) these backends are not obliged to offer >> it (yes they can offer the required features and support grant mappings for >> the virtio, but this is not strictly necessary, as they are considered as >> trusted so are allowed to access all guest memory). >> >> - DomD with backend which do offer them and require grant mappings for the >> virtio >> >> If this is a valid and correct use-case, then we indeed need an ability to >> control that per device, otherwise - what is written below doesn't really >> matter. >> >> I am wondering whether we can avoid using global >> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS for Xen guests at all? I assume that all >> we need to do (when CONFIG_XEN_VIRTIO is enabled) is to make sure that *only* >> Xen grant DMA devices in HVM guests and *all* devices in PV guests offer >> required flags. >> >> Below the diff how this could be done w/o an extra options (not completely >> tested), although I realize it might look hackish, and a lot more effort is >> needed to get it right. In my Arm64 based environment it works, I have tried >> to run two backends, the first offered required features and the corresponding >> device node had required property, but the second didn’t and there was no >> property. >> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c >> index 1f9c3ba..07eb69f 100644 >> --- a/arch/arm/xen/enlighten.c >> +++ b/arch/arm/xen/enlighten.c >> @@ -443,8 +443,6 @@ static int __init xen_guest_init(void) >> if (!xen_domain()) >> return 0; >> >> - xen_set_restricted_virtio_memory_access(); >> - >> if (!acpi_disabled) >> xen_acpi_guest_init(); >> else >> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c >> index 8b71b1d..517a9d8 100644 >> --- a/arch/x86/xen/enlighten_hvm.c >> +++ b/arch/x86/xen/enlighten_hvm.c >> @@ -195,8 +195,6 @@ static void __init xen_hvm_guest_init(void) >> if (xen_pv_domain()) >> return; >> >> - xen_set_restricted_virtio_memory_access(); >> - >> init_hvm_pv_info(); >> >> reserve_shared_info(); >> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c >> index 30d24fe..ca85d14 100644 >> --- a/arch/x86/xen/enlighten_pv.c >> +++ b/arch/x86/xen/enlighten_pv.c >> @@ -108,8 +108,6 @@ static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc); >> >> static void __init xen_pv_init_platform(void) >> { >> - xen_set_restricted_virtio_memory_access(); >> - >> populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP)); >> >> set_fixmap(FIX_PARAVIRT_BOOTMAP, xen_start_info->shared_info); >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index 371e16b..875690a 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, >> unsigned int status) >> } >> EXPORT_SYMBOL_GPL(virtio_add_status); >> >> +int __weak device_has_restricted_virtio_memory_access(struct device *dev) >> +{ >> + return platform_has(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); >> +} >> + >> /* Do some validation, then set FEATURES_OK */ >> static int virtio_features_ok(struct virtio_device *dev) >> { >> @@ -174,7 +179,7 @@ static int virtio_features_ok(struct virtio_device *dev) >> >> might_sleep(); >> >> - if (platform_has(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS)) { >> + if (device_has_restricted_virtio_memory_access(dev->dev.parent)) { >> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) { >> dev_warn(&dev->dev, >> "device must provide VIRTIO_F_VERSION_1\n"); >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c >> index 6586152..da938f6 100644 >> --- a/drivers/xen/grant-dma-ops.c >> +++ b/drivers/xen/grant-dma-ops.c >> @@ -11,6 +11,7 @@ >> #include <linux/dma-map-ops.h> >> #include <linux/of.h> >> #include <linux/pfn.h> >> +#include <linux/virtio_config.h> >> #include <linux/xarray.h> >> #include <xen/xen.h> >> #include <xen/grant_table.h> >> @@ -286,6 +287,11 @@ bool xen_is_grant_dma_device(struct device *dev) >> return has_iommu; >> } >> >> +int device_has_restricted_virtio_memory_access(struct device *dev) >> +{ >> + return (xen_pv_domain() || xen_is_grant_dma_device(dev)); >> +} >> + >> void xen_grant_setup_dma_ops(struct device *dev) >> { >> struct xen_grant_dma_data *data; >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h >> index 7949829..b3a455b 100644 >> --- a/include/linux/virtio_config.h >> +++ b/include/linux/virtio_config.h >> @@ -559,4 +559,6 @@ static inline void virtio_cwrite64(struct virtio_device >> *vdev, >> _r; \ >> }) >> >> +int device_has_restricted_virtio_memory_access(struct device *dev); >> + >> #endif /* _LINUX_VIRTIO_CONFIG_H */ >> diff --git a/include/xen/xen.h b/include/xen/xen.h >> index 0780a81..a99bab8 100644 >> --- a/include/xen/xen.h >> +++ b/include/xen/xen.h >> @@ -52,14 +52,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, >> extern u64 xen_saved_max_mem_size; >> #endif >> >> -#include <linux/platform-feature.h> >> - >> -static inline void xen_set_restricted_virtio_memory_access(void) >> -{ >> - if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain()) >> - platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); >> -} >> - >> #ifdef CONFIG_XEN_UNPOPULATED_ALLOC >> int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages); >> void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages); >> (END) >> >> >> I think when x86 HVM gains required support (via ACPI or other means) to >> communicate the x86's alternative of "xen,grant-dma" then >> xen_is_grant_dma_device() will be just extended to handle that. > > Yeah I like this approach: > > - on ARM it bases the setting of PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS > on "xen,grant-dma", as it should be > - it goes beyond my suggestion and it is capable of doing that > per-device, which is awesome > - on x86, it always enables for PV guests as they have no other choice > > On top of this we could add a command line option or kconfig option to > force-enable it as well for the benefit of x86/HVM, but I would make > that option x86 specific.
In the end the proper solution would be a per-device setting, as Christoph already said.
So basically I think we can rip out the PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS flag again (which would mean we could rip out the whole platform feature support again). Instead we should have a platform specific callback in virtio which replaces the test for PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS. The callback would have the virtio device as a parameter.
This callback would be pre-initialized with a function returning always "false". SEV, TDX and /390 PV could replace it with a function returning always "true". When CONFIG_XEN_VIRTIO_FORCE_GRANT is set, Xen guests would return always "true", otherwise they can check whether e.g. "xen,grant-dma" was set for the device in the device table and return "true" if this is the case. This scheme would IMO cover all needs.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |