[kernel,v2,4/4] powerpc/pseries/svm: Allow IOMMU to work in SVM

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(+)

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.