[PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
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 --- I've only done a compile test on x86 for now, as I can't test these changes easily (SEV might be doable for me, but s390 isn't). --- 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_identity.c | 5 + drivers/virtio/Kconfig | 6 -- drivers/virtio/virtio.c| 5 ++--- include/linux/platform-feature.h | 3 ++- include/linux/virtio_config.h | 9 - 10 files changed, 16 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..8e4fa10c6b12 100644 --- a/arch/s390/mm/init.c +++ b/arch/s390/mm/init.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -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_feature(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..435611d83895 100644 --- a/arch/x86/kernel/cpu/mshyperv.c +++ b/arch/x86/kernel/cpu/mshyperv.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -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_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); + } } } 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_identity.c b/arch/x86/mm/mem_encrypt_identity.c index b43bc24d2bb6..6043ba6cd17d 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp) } else { /* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask; + + /* Set restricted memory access for virtio. */ + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); + goto out; } 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. -conf
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 26.04.22 19:35, Borislav Petkov wrote: On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote: /* protected virtualization */ static void pv_init(void) { if (!is_prot_virt_guest()) return; + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Kinda long-ish for my taste. I'll probably call it: platform_set() as it is implicit that it sets a feature bit. Okay, fine with me. diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index b43bc24d2bb6..6043ba6cd17d 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp) } else { /* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask; + + /* Set restricted memory access for virtio. */ + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Huh, what does that have to do with SME? I picked the function where sev_status is being set, as this seemed to be the correct place to set the feature bit. Looking at it in more detail it might be preferable to do it in sev_setup_arch() instead. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 26.04.22 21:51, Heiko Carstens wrote: On Tue, Apr 26, 2022 at 07:35:43PM +0200, Borislav Petkov wrote: On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote: /* protected virtualization */ static void pv_init(void) { if (!is_prot_virt_guest()) return; + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Kinda long-ish for my taste. I'll probably call it: platform_set() as it is implicit that it sets a feature bit. ...and platform_clear(), instead of platform_reset_feature() please. Fine with me. In any case, yeah, looks ok at a quick glance. It would obviously need for more people to look at it and say whether it makes sense to them and whether that's fine to have in generic code but so far, the experience with cc_platform_* says that it seems to work ok in generic code. We _could_ convert s390's machine flags to this mechanism. Those flags are historically per-cpu, but if I'm not mistaken none of them is performance critical anymore, and those who are could/should probably transformed to jump labels or alternatives anyway. I was planning to look at the x86 cpu features to see whether some of those might be candidates to be switched to platform features instead. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 27.04.22 14:28, Borislav Petkov wrote: On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote: On 26.04.22 19:35, Borislav Petkov wrote: On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote: /* protected virtualization */ static void pv_init(void) { if (!is_prot_virt_guest()) return; + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Kinda long-ish for my taste. I'll probably call it: platform_set() as it is implicit that it sets a feature bit. Okay, fine with me. diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index b43bc24d2bb6..6043ba6cd17d 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp) } else { /* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask; + + /* Set restricted memory access for virtio. */ + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Huh, what does that have to do with SME? I picked the function where sev_status is being set, as this seemed to be the correct place to set the feature bit. What I don't understand is what does restricted memory access have to do with AMD SEV and how does play together with what you guys are trying to do? The big picture pls. Ah, okay. For support of virtio with Xen we want to not only support the virtio devices like KVM, but use grants for letting the guest decide which pages are allowed to be mapped by the backend (dom0). Instead of physical guest addresses the guest will use grant-ids (plus offset). In order to be able to handle this at the basic virtio level instead of the single virtio device drivers, we need to use dedicated dma-ops. And those will be used by virtio only, if the "restricted virtio memory request" flag is set, which is used by SEV, too. In order to let virtio set this flag, we need a way to communicate to virtio that the running system is either a SEV guest or a Xen guest. HTH, Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 27.04.22 14:26, Borislav Petkov wrote: On Wed, Apr 27, 2022 at 08:40:08AM +0200, Juergen Gross wrote: I was planning to look at the x86 cpu features to see whether some of those might be candidates to be switched to platform features instead. I'd say "never touch a running system" unless the platform features are of an advantage... Depends on the use case IMHO. All features being based on a cpuid bit are no candidates. Same applies to all features used for alternative handling (assuming we don't want to expand that to platform features). I really have no idea whether this will leave any candidates. In case it does, it might be interesting to switch those in order to save some per-cpu bits. Other candidates might be the x86_legacy_features. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 4/27/22 07:37, Juergen Gross wrote: On 27.04.22 14:28, Borislav Petkov wrote: On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote: On 26.04.22 19:35, Borislav Petkov wrote: On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote: /* protected virtualization */ static void pv_init(void) { if (!is_prot_virt_guest()) return; + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Kinda long-ish for my taste. I'll probably call it: platform_set() as it is implicit that it sets a feature bit. Okay, fine with me. diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index b43bc24d2bb6..6043ba6cd17d 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp) } else { /* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask; + + /* Set restricted memory access for virtio. */ + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); This is way early in the boot, but it appears that marking the platform feature bitmap as __read_mostly puts this in the .data section, so avoids the issue of bss being cleared. TDX support also uses the arch_has_restricted_virtio_memory_access() function and will need to be updated. Seems like a lot of changes, I just wonder if the the arch_has...() function couldn't be updated to also include a Xen check? Thanks, Tom Huh, what does that have to do with SME? I picked the function where sev_status is being set, as this seemed to be the correct place to set the feature bit. What I don't understand is what does restricted memory access have to do with AMD SEV and how does play together with what you guys are trying to do? The big picture pls. Ah, okay. For support of virtio with Xen we want to not only support the virtio devices like KVM, but use grants for letting the guest decide which pages are allowed to be mapped by the backend (dom0). Instead of physical guest addresses the guest will use grant-ids (plus offset). In order to be able to handle this at the basic virtio level instead of the single virtio device drivers, we need to use dedicated dma-ops. And those will be used by virtio only, if the "restricted virtio memory request" flag is set, which is used by SEV, too. In order to let virtio set this flag, we need a way to communicate to virtio that the running system is either a SEV guest or a Xen guest. HTH, Juergen ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()
On 27.04.22 16:09, Tom Lendacky wrote: On 4/27/22 07:37, Juergen Gross wrote: On 27.04.22 14:28, Borislav Petkov wrote: On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote: On 26.04.22 19:35, Borislav Petkov wrote: On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote: /* protected virtualization */ static void pv_init(void) { if (!is_prot_virt_guest()) return; + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); Kinda long-ish for my taste. I'll probably call it: platform_set() as it is implicit that it sets a feature bit. Okay, fine with me. diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c index b43bc24d2bb6..6043ba6cd17d 100644 --- a/arch/x86/mm/mem_encrypt_identity.c +++ b/arch/x86/mm/mem_encrypt_identity.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp) } else { /* SEV state cannot be controlled by a command line option */ sme_me_mask = me_mask; + + /* Set restricted memory access for virtio. */ + platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS); This is way early in the boot, but it appears that marking the platform feature bitmap as __read_mostly puts this in the .data section, so avoids the issue of bss being cleared. In V2 (not yet posted) I have moved the call to sev_setup_arch(). TDX support also uses the arch_has_restricted_virtio_memory_access() function and will need to be updated. Yes. Seems like a lot of changes, I just wonder if the the arch_has...() function couldn't be updated to also include a Xen check? This was not seen to be a nice solution. And TBH, I think this series is making the code much cleaner. Look at the diffstat of this patch. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization