Re: [PATCH V6 3/5] hyper-v: Enable swiotlb bounce buffer for Isolation VM

Tianyu Lan
2021-12-10T13:25:55Z

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.