Message-ID: <ab59158f-c718-f109-074c-7fc193c5406d@suse.com>
Date:   Fri, 17 Jun 2022 07:49:05 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr <olekstysh@...il.com>
Cc:     hch@...radead.org, xen-devel@...ts.xenproject.org,
        linux-kernel@...r.kernel.org, viresh.kumar@...aro.org,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>
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@...aro.org>
>>>>> Signed-off-by: Juergen Gross <jgross@...e.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

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)

Powered by blists - more mailing lists