| Message ID | 20231201132618.555031-10-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4bc8ff6f170c78f64446c5d5f9ef6771eefd3416 |
| Headers | show |
| Series | KVM: PPC: Nested APIv2 : Performance improvements | expand |
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > From: Jordan Niethe <jniethe5@gmail.com> > > H_COPY_TOFROM_GUEST is part of the nestedv1 API and so should not be > called by a nestedv2 host. Do not attempt to call it. > May be we should use firmware_has_feature(FW_FEATURE_H_COPY_TOFROM_GUEST))? the nestedv2 can end up using the above hcall if it is supported by the hypervisor right? In its absence we will have to translate the guest ea using xlate and then use kvm_guest_read to read location using the guest real address right? That xlate will also involves multiple kvm_guest_read. > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > arch/powerpc/kvm/book3s_64_mmu_radix.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c > index 916af6c153a5..4a1abb9f7c05 100644 > --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c > +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c > @@ -40,6 +40,9 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid, > unsigned long quadrant, ret = n; > bool is_load = !!to; > > + if (kvmhv_is_nestedv2()) > + return H_UNSUPPORTED; > + > /* Can't access quadrants 1 or 2 in non-HV mode, call the HV to do it */ > if (kvmhv_on_pseries()) > return plpar_hcall_norets(H_COPY_TOFROM_GUEST, lpid, pid, eaddr, > -- > 2.42.0
Hi Aneesh, Thanks for looking into this patch. My responses inline: "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes: <snip> > May be we should use > firmware_has_feature(FW_FEATURE_H_COPY_TOFROM_GUEST))? > > the nestedv2 can end up using the above hcall if it is supported by the > hypervisor right? In its absence we will have to translate the guest ea > using xlate and then use kvm_guest_read to read location using the guest > real address right? That xlate will also involves multiple kvm_guest_read. > > Yes, Agreed and thats a nice suggestion. However ATM the hypervisor supporting Nestedv2 doesnt have support for this hcall. In future once we have support for this hcall for nestedv2 from the hypervisor we can replace this branch with a firmware_has_feature() test. >> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >> --- >> arch/powerpc/kvm/book3s_64_mmu_radix.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c >> index 916af6c153a5..4a1abb9f7c05 100644 >> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c >> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c >> @@ -40,6 +40,9 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid, >> unsigned long quadrant, ret = n; >> bool is_load = !!to; >> >> + if (kvmhv_is_nestedv2()) >> + return H_UNSUPPORTED; >> + >> /* Can't access quadrants 1 or 2 in non-HV mode, call the HV to do it */ >> if (kvmhv_on_pseries()) >> return plpar_hcall_norets(H_COPY_TOFROM_GUEST, lpid, pid, eaddr, >> -- >> 2.42.0
On 12/11/23 9:26 AM, Vaibhav Jain wrote: > Hi Aneesh, > > Thanks for looking into this patch. My responses inline: > > "Aneesh Kumar K.V (IBM)" <aneesh.kumar@kernel.org> writes: > > <snip> >> May be we should use >> firmware_has_feature(FW_FEATURE_H_COPY_TOFROM_GUEST))? >> >> the nestedv2 can end up using the above hcall if it is supported by >> the hypervisor right? In its absence we will have to translate the >> guest ea using xlate and then use kvm_guest_read to read location >> using the guest real address right? That xlate will also involves >> multiple kvm_guest_read. >> >> > Yes, Agreed and thats a nice suggestion. However ATM the hypervisor > supporting Nestedv2 doesnt have support for this hcall. In future > once we have support for this hcall for nestedv2 from the hypervisor > we can replace this branch with a firmware_has_feature() test. > What I am suggesting is we convert that conditional to firmware_has_feature so that later when hypervisor supports this hcall all older kernel can make use of the copy_tofrom_guest without any code change. >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- >>> arch/powerpc/kvm/book3s_64_mmu_radix.c | 3 +++ 1 file changed, 3 >>> insertions(+) >>> >>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c >>> b/arch/powerpc/kvm/book3s_64_mmu_radix.c index >>> 916af6c153a5..4a1abb9f7c05 100644 --- >>> a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ >>> b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -40,6 +40,9 @@ >>> unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid, >>> unsigned long quadrant, ret = n; bool is_load = !!to; >>> >>> + if (kvmhv_is_nestedv2()) + return H_UNSUPPORTED; + /* Can't >>> access quadrants 1 or 2 in non-HV mode, call the HV to do it */ >>> if (kvmhv_on_pseries()) return >>> plpar_hcall_norets(H_COPY_TOFROM_GUEST, lpid, pid, eaddr, -- >>> 2.42.0 >
Hi Aneesh, "Aneesh Kumar K.V" <aneesh.kumar@kernel.org> writes: <snip> >> Yes, Agreed and thats a nice suggestion. However ATM the hypervisor >> supporting Nestedv2 doesnt have support for this hcall. In future >> once we have support for this hcall for nestedv2 from the hypervisor >> we can replace this branch with a firmware_has_feature() test. >> > > What I am suggesting is we convert that conditional to firmware_has_feature so that > later when hypervisor supports this hcall all older kernel can make > use of the copy_tofrom_guest without any code change. AFAIK for firmware_has_feature to work we either need: - A way to call this hcall with some invalid args. However lpid/pid for guest arent allocated during boot. - A way for hypervisor to advertise support for this hcall before the L1 kernel boots. ATM L0 dosent support for any of these two ways. I can do a follow up patch later when we have a clarity on how we want to advertise support for this hcall. For now current kernel supporting nestedv2 wont be using this hcall assuming its not supported. Future kernels can use one of the two ways above to set the firmware_has_feature flag to take advantage of this hcall.
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > Hi Aneesh, > > "Aneesh Kumar K.V" <aneesh.kumar@kernel.org> writes: > > <snip> >>> Yes, Agreed and thats a nice suggestion. However ATM the hypervisor >>> supporting Nestedv2 doesnt have support for this hcall. In future >>> once we have support for this hcall for nestedv2 from the hypervisor >>> we can replace this branch with a firmware_has_feature() test. >>> >> >> What I am suggesting is we convert that conditional to firmware_has_feature so that >> later when hypervisor supports this hcall all older kernel can make >> use of the copy_tofrom_guest without any code change. > > AFAIK for firmware_has_feature to work we either need: > - A way to call this hcall with some invalid args. However lpid/pid for > guest arent allocated during boot. > > - A way for hypervisor to advertise support for this hcall before the L1 > kernel boots. > > ATM L0 dosent support for any of these two ways. I can do a follow up > patch later when we have a clarity on how we want to advertise support > for this hcall. For now current kernel supporting nestedv2 wont be > using this hcall assuming its not supported. Future kernels can use one > of the two ways above to set the firmware_has_feature flag to take > advantage of this hcall. > We can use the second option and have L0 publish the firmware feature when it adds the new hcall. The good part about this is that all existing L1 kernels will automatically use the new hcall. Something like. diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h index 69ae9cf57d50..0ef97b56f999 100644 --- a/arch/powerpc/include/asm/firmware.h +++ b/arch/powerpc/include/asm/firmware.h @@ -57,6 +57,7 @@ #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000) #define FW_FEATURE_WATCHDOG ASM_CONST(0x0000080000000000) #define FW_FEATURE_PLPKS ASM_CONST(0x0000100000000000) +#define FW_FEATURE_H_COPY_TOFROM_GUEST ASM_CONST(0x0000200000000000) #ifndef __ASSEMBLY__ diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c index 282d1b54b073..8fc598b4767a 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -39,6 +39,9 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid, unsigned long quadrant, ret = n; bool is_load = !!to; + if (!firmware_has_feature(FW_FEATURE_H_COPY_TOFROM_GUEST)) + return H_UNSUPPORTED; + /* Can't access quadrants 1 or 2 in non-HV mode, call the HV to do it */ if (kvmhv_on_pseries()) return plpar_hcall_norets(H_COPY_TOFROM_GUEST, lpid, pid, eaddr, diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c index 18447e5fa17d..d49b5c52e7b8 100644 --- a/arch/powerpc/platforms/pseries/firmware.c +++ b/arch/powerpc/platforms/pseries/firmware.c @@ -69,6 +69,8 @@ hypertas_fw_features_table[] = { {FW_FEATURE_ENERGY_SCALE_INFO, "hcall-energy-scale-info"}, {FW_FEATURE_WATCHDOG, "hcall-watchdog"}, {FW_FEATURE_PLPKS, "hcall-pks"}, + {FW_FEATURE_H_COPY_TOFROM_GUEST, + "hcall-h-copy_tofrom-guest"}, }; /* Build up the firmware features bitmask using the contents of
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c index 916af6c153a5..4a1abb9f7c05 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -40,6 +40,9 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid, unsigned long quadrant, ret = n; bool is_load = !!to; + if (kvmhv_is_nestedv2()) + return H_UNSUPPORTED; + /* Can't access quadrants 1 or 2 in non-HV mode, call the HV to do it */ if (kvmhv_on_pseries()) return plpar_hcall_norets(H_COPY_TOFROM_GUEST, lpid, pid, eaddr,