RE: [PATCH v2 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Michael Kelley (LINUX)]

From: Juergen Gross <jgross@xxxxxxxx> Sent: Wednesday, April 27, 2022 8:34 AM
> 
> Instead of using arch_has_restricted_virtio_memory_access() together
> with CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, replace those
> with platform_has() and a new platform feature
> PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - move setting of PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS in SEV case
>   to sev_setup_arch().
> ---
>  arch/s390/Kconfig                |  1 -
>  arch/s390/mm/init.c              | 13 +++----------
>  arch/x86/Kconfig                 |  1 -
>  arch/x86/kernel/cpu/mshyperv.c   |  5 ++++-
>  arch/x86/mm/mem_encrypt.c        |  6 ------
>  arch/x86/mm/mem_encrypt_amd.c    |  4 ++++
>  drivers/virtio/Kconfig           |  6 ------
>  drivers/virtio/virtio.c          |  5 ++---
>  include/linux/platform-feature.h |  3 ++-
>  include/linux/virtio_config.h    |  9 ---------
>  10 files changed, 15 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index e084c72104f8..f97a22ae69a8 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -772,7 +772,6 @@ menu "Virtualization"
>  config PROTECTED_VIRTUALIZATION_GUEST
>  	def_bool n
>  	prompt "Protected virtualization guest support"
> -	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>  	help
>  	  Select this option, if you want to be able to run this
>  	  kernel as a protected virtualization KVM guest.
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 86ffd0d51fd5..2c3b451813ed 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -31,6 +31,7 @@
>  #include <linux/cma.h>
>  #include <linux/gfp.h>
>  #include <linux/dma-direct.h>
> +#include <linux/platform-feature.h>
>  #include <asm/processor.h>
>  #include <linux/uaccess.h>
>  #include <asm/pgalloc.h>
> @@ -168,22 +169,14 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
> 
> -#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> -
> -int arch_has_restricted_virtio_memory_access(void)
> -{
> -	return is_prot_virt_guest();
> -}
> -EXPORT_SYMBOL(arch_has_restricted_virtio_memory_access);
> -
> -#endif
> -
>  /* protected virtualization */
>  static void pv_init(void)
>  {
>  	if (!is_prot_virt_guest())
>  		return;
> 
> +	platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> +
>  	/* make sure bounce buffers are shared */
>  	swiotlb_force = SWIOTLB_FORCE;
>  	swiotlb_init(1);
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b0142e01002e..20ac72546ae4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1515,7 +1515,6 @@ config X86_CPA_STATISTICS
>  config X86_MEM_ENCRYPT
>  	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
>  	select DYNAMIC_PHYSICAL_MASK
> -	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>  	def_bool n
> 
>  config AMD_MEM_ENCRYPT
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 4b67094215bb..965518b9d14b 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -19,6 +19,7 @@
>  #include <linux/i8253.h>
>  #include <linux/random.h>
>  #include <linux/swiotlb.h>
> +#include <linux/platform-feature.h>
>  #include <asm/processor.h>
>  #include <asm/hypervisor.h>
>  #include <asm/hyperv-tlfs.h>
> @@ -347,8 +348,10 @@ static void __init ms_hyperv_init_platform(void)
>  #endif
>  		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
>  		if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> -			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> +			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE) {
>  				cc_set_vendor(CC_VENDOR_HYPERV);
> +
> 	platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> +			}
>  		}
>  	}
> 

Unless I'm misunderstanding something, the Hyper-V specific change isn't
needed.   Hyper-V doesn't support virtio in the first place, so it's a bit
unexpected be setting a virtio-related flag in Hyper-V code.   Also, Hyper-V
guests call sev_setup_arch() with CC_ATTR_GUEST_MEM_ENCRYPT set,
so this virtio-related flag will get set anyway via that path.

Michael

> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 50d209939c66..9b6a7c98b2b1 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -76,9 +76,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/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
> index 6169053c2854..39b71084d36b 100644
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -21,6 +21,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/virtio_config.h>
>  #include <linux/cc_platform.h>
> +#include <linux/platform-feature.h>
> 
>  #include <asm/tlbflush.h>
>  #include <asm/fixmap.h>
> @@ -206,6 +207,9 @@ void __init sev_setup_arch(void)
>  	size = total_mem * 6 / 100;
>  	size = clamp_val(size, IO_TLB_DEFAULT_SIZE, SZ_1G);
>  	swiotlb_adjust_size(size);
> +
> +	/* Set restricted memory access for virtio. */
> +	platform_set(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>  }
> 
>  static unsigned long pg_level_to_pfn(int level, pte_t *kpte, pgprot_t *ret_prot)
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index b5adf6abd241..a6dc8b5846fe 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -6,12 +6,6 @@ config VIRTIO
>  	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
>  	  or CONFIG_S390_GUEST.
> 
> -config ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> -	bool
> -	help
> -	  This option is selected if the architecture may need to enforce
> -	  VIRTIO_F_ACCESS_PLATFORM
> -
>  config VIRTIO_PCI_LIB
>  	tristate
>  	help
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..371e16b18381 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -5,6 +5,7 @@
>  #include <linux/module.h>
>  #include <linux/idr.h>
>  #include <linux/of.h>
> +#include <linux/platform-feature.h>
>  #include <uapi/linux/virtio_ids.h>
> 
>  /* Unique numbering for virtio devices. */
> @@ -170,12 +171,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status);
>  static int virtio_features_ok(struct virtio_device *dev)
>  {
>  	unsigned status;
> -	int ret;
> 
>  	might_sleep();
> 
> -	ret = arch_has_restricted_virtio_memory_access();
> -	if (ret) {
> +	if (platform_has(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS)) {
>  		if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>  			dev_warn(&dev->dev,
>  				 "device must provide VIRTIO_F_VERSION_1\n");
> diff --git a/include/linux/platform-feature.h b/include/linux/platform-feature.h
> index 6ed859928b97..5e2f08554b38 100644
> --- a/include/linux/platform-feature.h
> +++ b/include/linux/platform-feature.h
> @@ -6,7 +6,8 @@
>  #include <asm/platform-feature.h>
> 
>  /* The platform features are starting with the architecture specific ones. */
> -#define PLATFORM_FEAT_N				(0 +
> PLATFORM_ARCH_FEAT_N)
> +#define PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS	(0 +
> PLATFORM_ARCH_FEAT_N)
> +#define PLATFORM_FEAT_N				(1 +
> PLATFORM_ARCH_FEAT_N)
> 
>  void platform_set(unsigned int feature);
>  void platform_clear(unsigned int feature);
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index b341dd62aa4d..79498298519d 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -559,13 +559,4 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
>  		_r;							\
>  	})
> 
> -#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> -int arch_has_restricted_virtio_memory_access(void);
> -#else
> -static inline int arch_has_restricted_virtio_memory_access(void)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS */
> -
>  #endif /* _LINUX_VIRTIO_CONFIG_H */
> --
> 2.34.1