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

[Juergen Gross <jgross@suse.com>]

[-- 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 --]