Message ID | 20191216041924.42318-5-aik@ozlabs.ru |
---|---|
State | Not Applicable |
Headers | show |
Series | [kernel,v2,1/4] Revert "powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests" | expand |
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > H_PUT_TCE_INDIRECT uses a shared page to send up to 512 TCE to > a hypervisor in a single hypercall. This does not work for secure VMs > as the page needs to be shared or the VM should use H_PUT_TCE instead. > > This disables H_PUT_TCE_INDIRECT by clearing the FW_FEATURE_PUT_TCE_IND > feature bit so SVMs will map TCEs using H_PUT_TCE. > > This is not a part of init_svm() as it is called too late after FW > patching is done and may result in a warning like this: > > [ 3.727716] Firmware features changed after feature patching! > [ 3.727965] WARNING: CPU: 0 PID: 1 at (...)arch/powerpc/lib/feature-fixups.c:466 check_features+0xa4/0xc0 > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
On Mon, Dec 16, 2019 at 03:19:24PM +1100, Alexey Kardashevskiy wrote: > H_PUT_TCE_INDIRECT uses a shared page to send up to 512 TCE to > a hypervisor in a single hypercall. Actually H_PUT_TCE_INDIRECT never used shared page. It would have used shared pages if the 'shared-page' solution was accepted. :) > This does not work for secure VMs > as the page needs to be shared or the VM should use H_PUT_TCE instead. Maybe you should say something like this.. ? H_PUT_TCE_INDIRECT does not work for secure VMs, since the page containing the TCE entries is not accessible to the hypervisor. > > This disables H_PUT_TCE_INDIRECT by clearing the FW_FEATURE_PUT_TCE_IND > feature bit so SVMs will map TCEs using H_PUT_TCE. > > This is not a part of init_svm() as it is called too late after FW > patching is done and may result in a warning like this: > > [ 3.727716] Firmware features changed after feature patching! > [ 3.727965] WARNING: CPU: 0 PID: 1 at (...)arch/powerpc/lib/feature-fixups.c:466 check_features+0xa4/0xc0 > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Ram Pai <linuxram@us.ibm.com> > --- > Changes: > v2 > * new in the patchset > --- > arch/powerpc/platforms/pseries/firmware.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c > index d3acff23f2e3..3e49cc23a97a 100644 > --- a/arch/powerpc/platforms/pseries/firmware.c > +++ b/arch/powerpc/platforms/pseries/firmware.c > @@ -22,6 +22,7 @@ > #include <asm/firmware.h> > #include <asm/prom.h> > #include <asm/udbg.h> > +#include <asm/svm.h> > > #include "pseries.h" > > @@ -101,6 +102,12 @@ static void __init fw_hypertas_feature_init(const char *hypertas, > } > } > > + if (is_secure_guest() && > + (powerpc_firmware_features & FW_FEATURE_PUT_TCE_IND)) { > + powerpc_firmware_features &= ~FW_FEATURE_PUT_TCE_IND; > + pr_debug("SVM: disabling PUT_TCE_IND firmware feature\n"); > + } > + > pr_debug(" <- fw_hypertas_feature_init()\n"); > } > > -- > 2.17.1
On Thu, Jan 02, 2020 at 02:21:06PM -0800, Ram Pai wrote: > On Mon, Dec 16, 2019 at 03:19:24PM +1100, Alexey Kardashevskiy wrote: > > H_PUT_TCE_INDIRECT uses a shared page to send up to 512 TCE to > > a hypervisor in a single hypercall. > > Actually H_PUT_TCE_INDIRECT never used shared page. It would have > used shared pages if the 'shared-page' solution was accepted. :) Well, it depends what you mean by "shared". In the non-PEF case we do use a shared page in the sense that it is accessed by both guest and hypervisor. It's just not shared in the PEF sense. > > This does not work for secure VMs > > as the page needs to be shared or the VM should use H_PUT_TCE instead. > > Maybe you should say something like this.. ? > > H_PUT_TCE_INDIRECT does not work for secure VMs, since the page > containing the TCE entries is not accessible to the hypervisor. > > > > > This disables H_PUT_TCE_INDIRECT by clearing the FW_FEATURE_PUT_TCE_IND > > feature bit so SVMs will map TCEs using H_PUT_TCE. > > > > This is not a part of init_svm() as it is called too late after FW > > patching is done and may result in a warning like this: > > > > [ 3.727716] Firmware features changed after feature patching! > > [ 3.727965] WARNING: CPU: 0 PID: 1 at (...)arch/powerpc/lib/feature-fixups.c:466 check_features+0xa4/0xc0 > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > Reviewed-by: Ram Pai <linuxram@us.ibm.com> > > > > --- > > Changes: > > v2 > > * new in the patchset > > --- > > arch/powerpc/platforms/pseries/firmware.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c > > index d3acff23f2e3..3e49cc23a97a 100644 > > --- a/arch/powerpc/platforms/pseries/firmware.c > > +++ b/arch/powerpc/platforms/pseries/firmware.c > > @@ -22,6 +22,7 @@ > > #include <asm/firmware.h> > > #include <asm/prom.h> > > #include <asm/udbg.h> > > +#include <asm/svm.h> > > > > #include "pseries.h" > > > > @@ -101,6 +102,12 @@ static void __init fw_hypertas_feature_init(const char *hypertas, > > } > > } > > > > + if (is_secure_guest() && > > + (powerpc_firmware_features & FW_FEATURE_PUT_TCE_IND)) { > > + powerpc_firmware_features &= ~FW_FEATURE_PUT_TCE_IND; > > + pr_debug("SVM: disabling PUT_TCE_IND firmware feature\n"); > > + } > > + > > pr_debug(" <- fw_hypertas_feature_init()\n"); > > } > > >
On Fri, Jan 03, 2020 at 11:08:49AM +1100, David Gibson wrote: > On Thu, Jan 02, 2020 at 02:21:06PM -0800, Ram Pai wrote: > > On Mon, Dec 16, 2019 at 03:19:24PM +1100, Alexey Kardashevskiy wrote: > > > H_PUT_TCE_INDIRECT uses a shared page to send up to 512 TCE to > > > a hypervisor in a single hypercall. > > > > Actually H_PUT_TCE_INDIRECT never used shared page. It would have > > used shared pages if the 'shared-page' solution was accepted. :) > > Well, it depends what you mean by "shared". In the non-PEF case we do > use a shared page in the sense that it is accessed by both guest and > hypervisor. It's just not shared in the PEF sense. Ah..I see. That sentence can be right or wrong based on the reader's interpretion of the phrase 'shared page'. To me a 'shared page' is a page that is **explicitly** shared with the hypervisor. However I can see 'shared page' to mean a page that is simply shared; either implicitly or explicitly. Given that, there is a possibility for mis-interpretation, I think, it might be better to avoid the phrase 'shared page' if possible. > > > > This does not work for secure VMs > > > as the page needs to be shared or the VM should use H_PUT_TCE instead. > > > > Maybe you should say something like this.. ? > > > > H_PUT_TCE_INDIRECT does not work for secure VMs, since the page > > containing the TCE entries is not accessible to the hypervisor. > > > > > ..snip.
diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c index d3acff23f2e3..3e49cc23a97a 100644 --- a/arch/powerpc/platforms/pseries/firmware.c +++ b/arch/powerpc/platforms/pseries/firmware.c @@ -22,6 +22,7 @@ #include <asm/firmware.h> #include <asm/prom.h> #include <asm/udbg.h> +#include <asm/svm.h> #include "pseries.h" @@ -101,6 +102,12 @@ static void __init fw_hypertas_feature_init(const char *hypertas, } } + if (is_secure_guest() && + (powerpc_firmware_features & FW_FEATURE_PUT_TCE_IND)) { + powerpc_firmware_features &= ~FW_FEATURE_PUT_TCE_IND; + pr_debug("SVM: disabling PUT_TCE_IND firmware feature\n"); + } + pr_debug(" <- fw_hypertas_feature_init()\n"); }
H_PUT_TCE_INDIRECT uses a shared page to send up to 512 TCE to a hypervisor in a single hypercall. This does not work for secure VMs as the page needs to be shared or the VM should use H_PUT_TCE instead. This disables H_PUT_TCE_INDIRECT by clearing the FW_FEATURE_PUT_TCE_IND feature bit so SVMs will map TCEs using H_PUT_TCE. This is not a part of init_svm() as it is called too late after FW patching is done and may result in a warning like this: [ 3.727716] Firmware features changed after feature patching! [ 3.727965] WARNING: CPU: 0 PID: 1 at (...)arch/powerpc/lib/feature-fixups.c:466 check_features+0xa4/0xc0 Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v2 * new in the patchset --- arch/powerpc/platforms/pseries/firmware.c | 7 +++++++ 1 file changed, 7 insertions(+)