| Message ID | 20211207075602.2452-4-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/Hyper-V: Add Hyper-V Isolation VM support(Second part) | expand |
From: Tianyu Lan <ltykernel@gmail.com> Sent: Monday, December 6, 2021 11:56 PM > > hyperv Isolation VM requires bounce buffer support to copy > data from/to encrypted memory and so enable swiotlb force > mode to use swiotlb bounce buffer for DMA transaction. > > In Isolation VM with AMD SEV, the bounce buffer needs to be > accessed via extra address space which is above shared_gpa_boundary > (E.G 39 bit address line) reported by Hyper-V CPUID ISOLATION_CONFIG. > The access physical address will be original physical address + > shared_gpa_boundary. The shared_gpa_boundary in the AMD SEV SNP > spec is called virtual top of memory(vTOM). Memory addresses below > vTOM are automatically treated as private while memory above > vTOM is treated as shared. > > Swiotlb bounce buffer code calls set_memory_decrypted() > to mark bounce buffer visible to host and map it in extra > address space via memremap. Populate the shared_gpa_boundary > (vTOM) via swiotlb_unencrypted_base variable. > > The map function memremap() can't work in the early place > (e.g ms_hyperv_init_platform()) and so call swiotlb_update_mem_ > attributes() in the hyperv_init(). > > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com> > --- > Change since v4: > * Remove Hyper-V IOMMU IOMMU_INIT_FINISH related functions > and set SWIOTLB_FORCE and swiotlb_unencrypted_base in the > ms_hyperv_init_platform(). Call swiotlb_update_mem_attributes() > in the hyperv_init(). > > Change since v3: > * Add comment in pci-swiotlb-xen.c to explain why add > dependency between hyperv_swiotlb_detect() and pci_ > xen_swiotlb_detect(). > * Return directly when fails to allocate Hyper-V swiotlb > buffer in the hyperv_iommu_swiotlb_init(). > --- > arch/x86/hyperv/hv_init.c | 10 ++++++++++ > arch/x86/kernel/cpu/mshyperv.c | 11 ++++++++++- > include/linux/hyperv.h | 8 ++++++++ > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c > index 24f4a06ac46a..9e18a280f89d 100644 > --- a/arch/x86/hyperv/hv_init.c > +++ b/arch/x86/hyperv/hv_init.c > @@ -28,6 +28,7 @@ > #include <linux/syscore_ops.h> > #include <clocksource/hyperv_timer.h> > #include <linux/highmem.h> > +#include <linux/swiotlb.h> > > int hyperv_init_cpuhp; > u64 hv_current_partition_id = ~0ull; > @@ -502,6 +503,15 @@ void __init hyperv_init(void) > > /* Query the VMs extended capability once, so that it can be cached. */ > hv_query_ext_cap(0); > + > + /* > + * Swiotlb bounce buffer needs to be mapped in extra address > + * space. Map function doesn't work in the early place and so > + * call swiotlb_update_mem_attributes() here. > + */ > + if (hv_is_isolation_supported()) > + swiotlb_update_mem_attributes(); > + > return; > > clean_guest_os_id: > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c > index 4794b716ec79..baf3a0873552 100644 > --- a/arch/x86/kernel/cpu/mshyperv.c > +++ b/arch/x86/kernel/cpu/mshyperv.c > @@ -18,6 +18,7 @@ > #include <linux/kexec.h> > #include <linux/i8253.h> > #include <linux/random.h> > +#include <linux/swiotlb.h> > #include <asm/processor.h> > #include <asm/hypervisor.h> > #include <asm/hyperv-tlfs.h> > @@ -319,8 +320,16 @@ static void __init ms_hyperv_init_platform(void) > pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n", > ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b); > > - if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) > + if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) { > static_branch_enable(&isolation_type_snp); > + swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary; > + } > + > + /* > + * Enable swiotlb force mode in Isolation VM to > + * use swiotlb bounce buffer for dma transaction. > + */ > + swiotlb_force = SWIOTLB_FORCE; I'm good with this approach that directly updates the swiotlb settings here rather than in IOMMU initialization code. It's a lot more straightforward. However, there's an issue if building for X86_32 without PAE, in that the swiotlb module may not be built, resulting in compile and link errors. The swiotlb.h file needs to be updated to provide a stub function for swiotlb_update_mem_attributes(). swiotlb_unencrypted_base probably needs wrapper functions to get/set it, which can be stubs when CONFIG_SWIOTLB is not set. swiotlb_force is a bit of a mess in that it already has a stub definition that assumes it will only be read, and not set. A bit of thinking will be needed to sort that out. > } > > if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) { > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index b823311eac79..1f037e114dc8 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -1726,6 +1726,14 @@ int hyperv_write_cfg_blk(struct pci_dev *dev, void *buf, unsigned int len, > int hyperv_reg_block_invalidate(struct pci_dev *dev, void *context, > void (*block_invalidate)(void *context, > u64 block_mask)); > +#if IS_ENABLED(CONFIG_HYPERV) > +int __init hyperv_swiotlb_detect(void); > +#else > +static inline int __init hyperv_swiotlb_detect(void) > +{ > + return 0; > +} > +#endif I don't think hyperv_swiotlb_detect() is used any longer, so this change should be dropped. > > struct hyperv_pci_block_ops { > int (*read_block)(struct pci_dev *dev, void *buf, unsigned int buf_len, > -- > 2.25.1
On 12/10/2021 4:09 AM, Michael Kelley (LINUX) wrote: >> @@ -319,8 +320,16 @@ static void __init ms_hyperv_init_platform(void) >> pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n", >> ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b); >> >> - if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) >> + if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) { >> static_branch_enable(&isolation_type_snp); >> + swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary; >> + } >> + >> + /* >> + * Enable swiotlb force mode in Isolation VM to >> + * use swiotlb bounce buffer for dma transaction. >> + */ >> + swiotlb_force = SWIOTLB_FORCE; > I'm good with this approach that directly updates the swiotlb settings here > > rather than in IOMMU initialization code. It's a lot more straightforward. > > However, there's an issue if building for X86_32 without PAE, in that the > swiotlb module may not be built, resulting in compile and link errors. The > swiotlb.h file needs to be updated to provide a stub function for > swiotlb_update_mem_attributes(). swiotlb_unencrypted_base probably > needs wrapper functions to get/set it, which can be stubs when > CONFIG_SWIOTLB is not set. swiotlb_force is a bit of a mess in that it already > has a stub definition that assumes it will only be read, and not set. A bit of > thinking will be needed to sort that out. It's ok to fix the issue via selecting swiotlb when CONFIG_HYPERV is set? > >> } >> >> if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) { >> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h >> index b823311eac79..1f037e114dc8 100644 >> --- a/include/linux/hyperv.h >> +++ b/include/linux/hyperv.h >> @@ -1726,6 +1726,14 @@ int hyperv_write_cfg_blk(struct pci_dev *dev, void *buf, unsigned int len, >> int hyperv_reg_block_invalidate(struct pci_dev *dev, void *context, >> void (*block_invalidate)(void *context, >> u64 block_mask)); >> +#if IS_ENABLED(CONFIG_HYPERV) >> +int __init hyperv_swiotlb_detect(void); >> +#else >> +static inline int __init hyperv_swiotlb_detect(void) >> +{ >> + return 0; >> +} >> +#endif > I don't think hyperv_swiotlb_detect() is used any longer, so this change > should be dropped. Yes, will update.
On 12/10/2021 9:25 PM, Tianyu Lan wrote: >>> @@ -319,8 +320,16 @@ static void __init ms_hyperv_init_platform(void) >>> pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B >>> 0x%x\n", >>> ms_hyperv.isolation_config_a, >>> ms_hyperv.isolation_config_b); >>> >>> - if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) >>> + if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) { >>> static_branch_enable(&isolation_type_snp); >>> + swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary; >>> + } >>> + >>> + /* >>> + * Enable swiotlb force mode in Isolation VM to >>> + * use swiotlb bounce buffer for dma transaction. >>> + */ >>> + swiotlb_force = SWIOTLB_FORCE; >> I'm good with this approach that directly updates the swiotlb settings >> here >> >> rather than in IOMMU initialization code. It's a lot more >> straightforward. >> >> However, there's an issue if building for X86_32 without PAE, in that the >> swiotlb module may not be built, resulting in compile and link >> errors. The >> swiotlb.h file needs to be updated to provide a stub function for >> swiotlb_update_mem_attributes(). swiotlb_unencrypted_base probably >> needs wrapper functions to get/set it, which can be stubs when >> CONFIG_SWIOTLB is not set. swiotlb_force is a bit of a mess in that >> it already >> has a stub definition that assumes it will only be read, and not set. >> A bit of >> thinking will be needed to sort that out. > > It's ok to fix the issue via selecting swiotlb when CONFIG_HYPERV is > set? > Sorry. ignore the previous statement. These codes doesn't depend on CONFIG_HYPERV. How about making these code under #ifdef CONFIG_X86_64 or CONFIG_SWIOTLB?
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c index 24f4a06ac46a..9e18a280f89d 100644 --- a/arch/x86/hyperv/hv_init.c +++ b/arch/x86/hyperv/hv_init.c @@ -28,6 +28,7 @@ #include <linux/syscore_ops.h> #include <clocksource/hyperv_timer.h> #include <linux/highmem.h> +#include <linux/swiotlb.h> int hyperv_init_cpuhp; u64 hv_current_partition_id = ~0ull; @@ -502,6 +503,15 @@ void __init hyperv_init(void) /* Query the VMs extended capability once, so that it can be cached. */ hv_query_ext_cap(0); + + /* + * Swiotlb bounce buffer needs to be mapped in extra address + * space. Map function doesn't work in the early place and so + * call swiotlb_update_mem_attributes() here. + */ + if (hv_is_isolation_supported()) + swiotlb_update_mem_attributes(); + return; clean_guest_os_id: diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c index 4794b716ec79..baf3a0873552 100644 --- a/arch/x86/kernel/cpu/mshyperv.c +++ b/arch/x86/kernel/cpu/mshyperv.c @@ -18,6 +18,7 @@ #include <linux/kexec.h> #include <linux/i8253.h> #include <linux/random.h> +#include <linux/swiotlb.h> #include <asm/processor.h> #include <asm/hypervisor.h> #include <asm/hyperv-tlfs.h> @@ -319,8 +320,16 @@ static void __init ms_hyperv_init_platform(void) pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n", ms_hyperv.isolation_config_a, ms_hyperv.isolation_config_b); - if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) + if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) { static_branch_enable(&isolation_type_snp); + swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary; + } + + /* + * Enable swiotlb force mode in Isolation VM to + * use swiotlb bounce buffer for dma transaction. + */ + swiotlb_force = SWIOTLB_FORCE; } if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) { diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index b823311eac79..1f037e114dc8 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1726,6 +1726,14 @@ int hyperv_write_cfg_blk(struct pci_dev *dev, void *buf, unsigned int len, int hyperv_reg_block_invalidate(struct pci_dev *dev, void *context, void (*block_invalidate)(void *context, u64 block_mask)); +#if IS_ENABLED(CONFIG_HYPERV) +int __init hyperv_swiotlb_detect(void); +#else +static inline int __init hyperv_swiotlb_detect(void) +{ + return 0; +} +#endif struct hyperv_pci_block_ops { int (*read_block)(struct pci_dev *dev, void *buf, unsigned int buf_len,