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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > ---
> > > > 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.