Messages in this thread | | | Subject | Re: [PATCH v2] xen: don't require virtio with grants for non-PV guests | From | Oleksandr <> | Date | Fri, 17 Jun 2022 11:15:21 +0300 |
| |
On 17.06.22 08:49, Juergen Gross wrote:
Hello Juergen, Stefano
> 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.
agree
> > 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.
If I got the idea correctly, I think this will work too. Sounds fine to me.
> > > > Juergen
-- Regards,
Oleksandr Tyshchenko
| |