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

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

[Juergen Gross]

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);
 }
 
-- 
2.35.3

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

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 07:37:15AM +0200, Juergen Gross wrote:
> 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.

What Í'd really expect here is to only set the limitations for the
actual grant-based devic.  Unfortunately
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS is global instead of per-device,
but this is what coms closest to that intention without major
refactoring:

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 1f9c3ba328333..07eb69f9e7df3 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 8b71b1dd76396..517a9d8d8f94d 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 e3297b15701c6..f33a4421e7cd6 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -109,8 +109,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/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index fc01424840017..f9bbacb5b5456 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/platform-feature.h>
 #include <linux/dma-map-ops.h>
 #include <linux/of.h>
 #include <linux/pfn.h>
@@ -333,6 +334,8 @@ void xen_grant_setup_dma_ops(struct device *dev)
 		goto err;
 	}
 
+	/* XXX: this really should be per-device instead of blobal */
+	platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
 	dev->dma_ops = &xen_grant_dma_ops;
 
 	return;
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 0780a81e140de..a99bab8175234 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);

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1539 bytes --]

On 16.06.22 08:03, Christoph Hellwig wrote:
> On Thu, Jun 16, 2022 at 07:37:15AM +0200, Juergen Gross wrote:
>> 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.
> 
> What Í'd really expect here is to only set the limitations for the
> actual grant-based devic.  Unfortunately
> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS is global instead of per-device,

I think the global setting is fine, as it serves a specific purpose:
don't allow ANY virtio devices without the special handling (like the
/390 PV case, SEV, TDX, or Xen PV-guests). Those cases can't sensibly
work without the special DMA ops.

In case the special DMA ops are just a "nice to have" like for Xen HVM
guests, PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS won't be set.

And if someone wants a guest only to use grant based virtio devices,
the guest kernel can be built with CONFIG_XEN_VIRTIO_FORCE_GRANT (e.g.
in case the backends are running in some less privileged environment
and thus can't map arbitrary guest memory pages).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Oleksandr]

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);


>   }
>   

-- 
Regards,

Oleksandr Tyshchenko

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2900 bytes --]

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.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Oleksandr]

On 16.06.22 11:56, Juergen Gross wrote:

Hello Juergen, all


> 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.


bool xen_is_grant_dma_device(struct device *dev)
{
     struct device_node *iommu_np;
     bool has_iommu;

     /* XXX Handle only DT devices for now */
     if (!dev->of_node)
         return false;

     iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
     has_iommu = iommu_np && of_device_is_compatible(iommu_np, 
"xen,grant-dma");
     of_node_put(iommu_np);

     return has_iommu;
}



>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 11167 bytes --]

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.

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 12474 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Oleksandr]

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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4063 bytes --]

On 16.06.22 22:33, Oleksandr wrote:
> 
> On 16.06.22 11:56, Juergen Gross wrote:
> 
> Hello Juergen, all
> 
> 
>> 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”? 

No, if you need some virtio devices to not use grants, then don't set
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS.

Please see my answer to Stefano's alternative solution for my idea how to
resolve this via a per-device setting.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Stefano Stabellini]

On Thu, 16 Jun 2022, Juergen Gross wrote:
> 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);
>  }

Hi Juergen, you might have seen my email:
https://marc.info/?l=linux-kernel&m=165533636607801&w=2

Linux is always running as HVM on ARM, so if you want to introduce
XEN_VIRTIO_FORCE_GRANT, then XEN_VIRTIO_FORCE_GRANT should be
automatically selected on ARM. I don't think there should be a visible
menu option for XEN_VIRTIO_FORCE_GRANT on ARM.

I realize we have a conflict between HVM guests on ARM and x86:

- on ARM, PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS should be enabled when
  "xen,grant-dma" is present
- on x86, due to the lack of "xen,grant-dma", it should be off by
  default and based on a kconfig or command line option

To be honest, like Christoph suggested, I think even on x86 there should
be a firmware table to trigger setting
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS. We have 2 Xen-specific ACPI
tables, and we could have 1 more to define this. Or an HVM param or
a feature flag?

I think that would be the cleanest way to do this, but it is a lot of
more work compared to adding a couple of lines of code to Linux, so this
is why I suggested:
https://marc.info/?l=linux-kernel&m=165533636607801&w=2

ARM uses "xen,grant-dma" to detect whether
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS needs setting.

One day x86 could check an ACPI property or HVM param or feature flag.
None of them are available now, so for now use a command line option as
a workaround. It is totally fine to use an x86-only kconfig option
instead of a command line option.

Would you be OK with that?

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 5357 bytes --]

On 16.06.22 20:20, Stefano Stabellini wrote:
> On Thu, 16 Jun 2022, Juergen Gross wrote:
>> 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);
>>   }
> 
> Hi Juergen, you might have seen my email:
> https://marc.info/?l=linux-kernel&m=165533636607801&w=2
> 
> Linux is always running as HVM on ARM, so if you want to introduce
> XEN_VIRTIO_FORCE_GRANT, then XEN_VIRTIO_FORCE_GRANT should be
> automatically selected on ARM. I don't think there should be a visible
> menu option for XEN_VIRTIO_FORCE_GRANT on ARM.

No, I don't think so. I think you are mixing up different things here.

Setting XEN_VIRTIO_FORCE_GRANT requires to support grants for all
virtio devices of the guest, while there might be perfect reasons to
have that for some special devices only (or to allow to use no grants
for some special devices).

Your suggestion would result in an "all or nothing" approach, while
many users could very well want a mixed setup.

> I realize we have a conflict between HVM guests on ARM and x86:
> 
> - on ARM, PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS should be enabled when
>    "xen,grant-dma" is present

Again, why? Why should one virtio device with a backend running in
a driver domain require _all_ virtio devices to use grants?

> - on x86, due to the lack of "xen,grant-dma", it should be off by
>    default and based on a kconfig or command line option

See above. I don't see a major difference for Arm here.

> To be honest, like Christoph suggested, I think even on x86 there should
> be a firmware table to trigger setting
> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS. We have 2 Xen-specific ACPI
> tables, and we could have 1 more to define this. Or an HVM param or
> a feature flag?

Please don't mix up PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS (which will
end in requiring grants for _all_ virtio devices) and the use case
per device.

I agree that x86 needs a way to transport the grant setting per
device, if only for communicating the backend domain id.

I can see two different solutions for that: either ACPI or Xenstore.
A HVM param doesn't seem to do the job, as the backend domain id still
needs to be communicated somehow.

When considering which one to use (there are maybe other alternatives)
we should have in mind, that the solution should support PV guests
(which in the general case don't see an ACPI table today) as well as
virtio device hotplugging (is this possible on Arm via device tree? -
I guess it should be, but I'm not sure how difficult that would be).

> I think that would be the cleanest way to do this, but it is a lot of
> more work compared to adding a couple of lines of code to Linux, so this
> is why I suggested:
> https://marc.info/?l=linux-kernel&m=165533636607801&w=2

I'll answer to this one separately.

> 
> ARM uses "xen,grant-dma" to detect whether
> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS needs setting.
> 
> One day x86 could check an ACPI property or HVM param or feature flag.
> None of them are available now, so for now use a command line option as
> a workaround. It is totally fine to use an x86-only kconfig option
> instead of a command line option.
> 
> Would you be OK with that?

See above. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]