Re: [RFC PATCH 2/6] virtio: add option to restrict memory access under Xen

[Oleksandr <olekstysh@gmail.com>]

Hello Stefano, Juergen


On 19.04.22 09:21, Juergen Gross wrote:
> On 18.04.22 21:11, Stefano Stabellini wrote:
>> On Sun, 17 Apr 2022, Oleksandr wrote:
>>> On 16.04.22 01:01, Stefano Stabellini wrote:
>>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
>>>>> From: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> In order to support virtio in Xen guests add a config option enabling
>>>>> the user to specify whether in all Xen guests virtio should be 
>>>>> able to
>>>>> access memory via Xen grant mappings only on the host side.
>>>>>
>>>>> This applies to fully virtualized guests only, as for paravirtualized
>>>>> guests this is mandatory.
>>>>>
>>>>> This requires to switch arch_has_restricted_virtio_memory_access()
>>>>> from a pure stub to a real function on x86 systems (Arm systems are
>>>>> not covered by now).
>>>>>
>>>>> Add the needed functionality by providing a special set of DMA ops
>>>>> handling the needed grant operations for the I/O pages.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>>    arch/x86/mm/init.c        |  15 ++++
>>>>>    arch/x86/mm/mem_encrypt.c |   5 --
>>>>>    arch/x86/xen/Kconfig      |   9 +++
>>>>>    drivers/xen/Kconfig       |  20 ++++++
>>>>>    drivers/xen/Makefile      |   1 +
>>>>>    drivers/xen/xen-virtio.c  | 177
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/xen/xen-ops.h     |   8 +++
>>>>>    7 files changed, 230 insertions(+), 5 deletions(-)
>>>>>    create mode 100644 drivers/xen/xen-virtio.c
>>>>>
>>>>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>>>>> index d8cfce2..526a3b2 100644
>>>>> --- a/arch/x86/mm/init.c
>>>>> +++ b/arch/x86/mm/init.c
>>>>> @@ -8,6 +8,8 @@
>>>>>    #include <linux/kmemleak.h>
>>>>>    #include <linux/sched/task.h>
>>>>>    +#include <xen/xen.h>
>>>>> +
>>>>>    #include <asm/set_memory.h>
>>>>>    #include <asm/e820/api.h>
>>>>>    #include <asm/init.h>
>>>>> @@ -1065,3 +1067,16 @@ unsigned long max_swapfile_size(void)
>>>>>        return pages;
>>>>>    }
>>>>>    #endif
>>>>> +
>>>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>>> +{
>>>>> +    if (IS_ENABLED(CONFIG_XEN_PV_VIRTIO) && xen_pv_domain())
>>>>> +        return 1;
>>>>> +    if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain())
>>>>> +        return 1;
>>>> I think these two checks could be moved to a separate function in a 
>>>> Xen
>>>> header, e.g. xen_restricted_virtio_memory_access, and here you could
>>>> just
>>>>
>>>> if (xen_restricted_virtio_memory_access())
>>>>       return 1;
>>>
>>> Agree, will do
>>>
>>>
>>>>
>>>>
>>>>
>>>>> +    return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>>> +#endif
>>>>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>>>>> index 50d2099..dda020f 100644
>>>>> --- a/arch/x86/mm/mem_encrypt.c
>>>>> +++ b/arch/x86/mm/mem_encrypt.c
>>>>> @@ -77,8 +77,3 @@ void __init mem_encrypt_init(void)
>>>>>        print_mem_encrypt_feature_info();
>>>>>    }
>>>>>    -int arch_has_restricted_virtio_memory_access(void)
>>>>> -{
>>>>> -    return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
>>>>> -}
>>>>> -EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
>>>>> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
>>>>> index 85246dd..dffdffd 100644
>>>>> --- a/arch/x86/xen/Kconfig
>>>>> +++ b/arch/x86/xen/Kconfig
>>>>> @@ -92,3 +92,12 @@ config XEN_DOM0
>>>>>        select X86_X2APIC if XEN_PVH && X86_64
>>>>>        help
>>>>>          Support running as a Xen Dom0 guest.
>>>>> +
>>>>> +config XEN_PV_VIRTIO
>>>>> +    bool "Xen virtio support for PV guests"
>>>>> +    depends on XEN_VIRTIO && XEN_PV
>>>>> +    default y
>>>>> +    help
>>>>> +      Support virtio for running as a paravirtualized guest. This 
>>>>> will
>>>>> +      need support on the backend side (qemu or kernel, depending 
>>>>> on the
>>>>> +      virtio device types used).
>>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>>>>> index 120d32f..fc61f7a 100644
>>>>> --- a/drivers/xen/Kconfig
>>>>> +++ b/drivers/xen/Kconfig
>>>>> @@ -335,4 +335,24 @@ config XEN_UNPOPULATED_ALLOC
>>>>>          having to balloon out RAM regions in order to obtain 
>>>>> physical memory
>>>>>          space to create such mappings.
>>>>>    +config XEN_VIRTIO
>>>>> +    bool "Xen virtio support"
>>>>> +    default n
>>>>> +    depends on VIRTIO && DMA_OPS
>>>>> +    select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>>> +    help
>>>>> +      Enable virtio support for running as Xen guest. Depending 
>>>>> on the
>>>>> +      guest type this will require special support on the backend 
>>>>> side
>>>>> +      (qemu or kernel, depending on the virtio device types used).
>>>>> +
>>>>> +config XEN_HVM_VIRTIO_GRANT
>>>>> +    bool "Require virtio for fully virtualized guests to use grant
>>>>> mappings"
>>>>> +    depends on XEN_VIRTIO && X86_64
>>>>> +    default y
>>>>> +    help
>>>>> +      Require virtio for fully virtualized 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
>>>>> +      (qemu or kernel, depending on the virtio device types used).
>>>> I don't think we need 3 visible kconfig options for this.
>>>>
>>>> In fact, I would only add one: XEN_VIRTIO. We can have any X86 (or 
>>>> ARM)
>>>> specific dependencies in the "depends" line under XEN_VIRTIO. And I
>>>> don't think we need XEN_HVM_VIRTIO_GRANT as a kconfig option
>>>> necessarely. It doesn't seem like some we want as build time 
>>>> option. At
>>>> most, it could be a runtime option (like a command line) or a debug
>>>> option (like an #define at the top of the source file.)
>>>
>>>
>>> I don't know what was the initial idea of having and extra 
>>> XEN_HVM_VIRTIO and
>>> XEN_PV_VIRTIO options, but taking into the account that
>>> they are only used in arch_has_restricted_virtio_memory_access() 
>>> currently, I
>>> share your opinion regarding a single XEN_VIRTIO option.
>>>
>>> Looking ahead (including changes in the commit #4), we can imagine the
>>> resulting option:
>>>
>>> config XEN_VIRTIO
>>>      bool "Xen virtio support"
>>>      default n
>>>      depends on VIRTIO && DMA_OPS
>>>      depends on (X86_64 || ARM || ARM64)
>>>      select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>      help
>>>        Enable virtio support for running as Xen guest. Depending on the
>>>        guest type this will require special support on the backend side
>>>        (qemu or kernel, depending on the virtio device types used).
>>>
>>>
>>> and then arch_has_restricted_virtio_memory_access() per arch:
>>>
>>>
>>> 1. x86:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return (xen_has_restricted_virtio_memory_access() ||
>>>              cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>>> }
>>>
>>>
>>> 2. Arm:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return xen_has_restricted_virtio_memory_access();
>>> }
>>>
>>>
>>> 3. xen.h:
>>>
>>> static inline int xen_has_restricted_virtio_memory_access(void)
>>> {
>>>      if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() ||
>>> xen_hvm_domain()))
>>>          return 1;
>>>
>>>      return 0;
>>> }
>>>
>>>
>>> Actually, as domain type on Arm is always XEN_HVM_DOMAIN, we could 
>>> probably
>>> have the following on Arm:
>>>
>>> int arch_has_restricted_virtio_memory_access(void)
>>> {
>>>      return IS_ENABLED(CONFIG_XEN_VIRTIO);
>>> }
>>>
>>> but I would prefer not to diverge and use common
>>> xen_has_restricted_virtio_memory_access().
>>>
>>> Any thoughts?
>>
>> Yes, I would also prefer not to diverge between the x86 and arm versions
>> of xen_has_restricted_virtio_memory_access. But what case are we trying
>> to catch with (xen_pv_domain() || xen_hvm_domain()) ? Even on x86, it is
>> not going to leave much out. Is it really meant only to exclude pvh
>> domains?

Good question. By leaving (xen_pv_domain() || xen_hvm_domain()) here I 
tried to retain what the *initial* version of 
arch_has_restricted_virtio_memory_access() covered.


>
> It wouldn't exclude pvh domains.


ok


>
>>
>> I have the feeling that we could turn this check into:
>>
>> static inline int xen_has_restricted_virtio_memory_access(void)
>> {
>>      return IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain();
>> }
>>
>> even on x86, but one of the xen/x86 maintainers should confirm.
>
> I do confirm this is better and functionally equivalent.


Perfect, thank you for confirming. Will use that check.


>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko