Messages in this thread | | | Subject | Re: [PATCH 03/13] x86/HV: Add new hvcall guest address host visibility support | From | Tianyu Lan <> | Date | Thu, 29 Jul 2021 20:49:31 +0800 |
| |
Hi Dave: Thanks for your review.
On 7/28/2021 11:29 PM, Dave Hansen wrote: > On 7/28/21 7:52 AM, Tianyu Lan wrote: >> @@ -1986,7 +1988,9 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc) >> int ret; >> >> /* Nothing to do if memory encryption is not active */ >> - if (!mem_encrypt_active()) >> + if (hv_is_isolation_supported()) >> + return hv_set_mem_enc(addr, numpages, enc); >> + else if (!mem_encrypt_active()) >> return 0; > > __set_memory_enc_dec() is turning into a real mess. SEV, TDX and now > Hyper-V are messing around in here. > > It doesn't help that these additions are totally uncommented. Even > worse is that hv_set_mem_enc() was intentionally named "enc" when it > presumably has nothing to do with encryption. > > This needs to be refactored. The current __set_memory_enc_dec() can > become __set_memory_enc_pgtable(). It gets used for the hypervisors > that get informed about "encryption" status via page tables: SEV and TDX. > > Then, rename hv_set_mem_enc() to hv_set_visible_hcall(). You'll end up > with: > > int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc) > { > if (hv_is_isolation_supported()) > return hv_set_visible_hcall(...); > > if (mem_encrypt_active() || ...) > return __set_memory_enc_pgtable(); > > /* Nothing to do */ > return 0; > } > > That tells the story pretty effectively, in code.
Yes, this is good idea. Thanks for your suggestion.
> >> +int hv_set_mem_enc(unsigned long addr, int numpages, bool enc) >> +{ >> + return hv_set_mem_host_visibility((void *)addr, >> + numpages * HV_HYP_PAGE_SIZE, >> + enc ? VMBUS_PAGE_NOT_VISIBLE >> + : VMBUS_PAGE_VISIBLE_READ_WRITE); >> +} > > I know this is off in Hyper-V code, but this just makes my eyes bleed. > I'd much rather see something which is less compact but readable.
OK. Will update.
> >> +/* Hyper-V GPA map flags */ >> +#define VMBUS_PAGE_NOT_VISIBLE 0 >> +#define VMBUS_PAGE_VISIBLE_READ_ONLY 1 >> +#define VMBUS_PAGE_VISIBLE_READ_WRITE 3 > > That looks suspiciously like an enum. >
OK. Will update.
| |