Message-ID: <c25d64c6-2910-c137-41fb-14d44c4aac7b@gmail.com>
Date:   Fri, 17 Jun 2022 11:15:21 +0300
From:   Oleksandr <olekstysh@...il.com>
To:     Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>
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 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@...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.


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

Powered by blists - more mailing lists