* [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-04 21:28 ` Ram Pai
0 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-04 21:28 UTC (permalink / raw)
To: linuxppc-dev
Cc: andmike, mst, aik, linuxram, mdroth, linux-kernel, ram.n.pai,
cai, tglx, sukadev, hch, bauerman, david
The hypervisor needs to access the contents of the page holding the TCE
entries while setting up the TCE entries in the IOMMU's TCE table. For
SecureVMs, since this page is encrypted, the hypervisor cannot access
valid entries. Share the page with the hypervisor. This ensures that the
hypervisor sees the valid entries.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 8d9c2b1..07f0847 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -37,6 +37,7 @@
#include <asm/mmzone.h>
#include <asm/plpar_wrappers.h>
#include <asm/svm.h>
+#include <asm/ultravisor.h>
#include "pseries.h"
@@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
static DEFINE_PER_CPU(__be64 *, tce_page);
+/*
+ * Allocate a tce page. If secure VM, share the page with the hypervisor.
+ */
+static __be64 *alloc_tce_page(void)
+{
+ __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
+
+ if (tcep && is_secure_guest())
+ uv_share_page(PHYS_PFN(__pa(tcep)), 1);
+
+ return tcep;
+}
+
static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
long npages, unsigned long uaddr,
enum dma_data_direction direction,
@@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
* from iommu_alloc{,_sg}()
*/
if (!tcep) {
- tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
- /* If allocation fails, fall back to the loop implementation */
+ tcep = alloc_tce_page();
if (!tcep) {
local_irq_restore(flags);
return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
@@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
return rc;
}
+
static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
unsigned long num_pfn, const void *arg)
{
@@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
tcep = __this_cpu_read(tce_page);
if (!tcep) {
- tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
+ tcep = alloc_tce_page();
if (!tcep) {
local_irq_enable();
return -ENOMEM;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
2019-11-04 21:28 ` Ram Pai
@ 2019-11-04 21:28 ` Ram Pai
-1 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-04 21:28 UTC (permalink / raw)
To: linuxppc-dev
Cc: benh, david, mpe, paulus, mdroth, hch, linuxram, andmike,
sukadev, mst, ram.n.pai, aik, cai, tglx, bauerman, linux-kernel
This enables IOMMU support for pseries Secure VMs.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/platforms/pseries/iommu.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 07f0847..189717b 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
of_reconfig_notifier_register(&iommu_reconfig_nb);
register_memory_notifier(&iommu_mem_nb);
- /*
- * Secure guest memory is inacessible to devices so regular DMA isn't
- * possible.
- *
- * In that case keep devices' dma_map_ops as NULL so that the generic
- * DMA code path will use SWIOTLB to bounce buffers for DMA.
- */
- if (!is_secure_guest())
- set_pci_dma_ops(&dma_iommu_ops);
+ set_pci_dma_ops(&dma_iommu_ops);
}
static int __init disable_multitce(char *str)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
@ 2019-11-04 21:28 ` Ram Pai
0 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-04 21:28 UTC (permalink / raw)
To: linuxppc-dev
Cc: andmike, mst, aik, linuxram, mdroth, linux-kernel, ram.n.pai,
cai, tglx, sukadev, hch, bauerman, david
This enables IOMMU support for pseries Secure VMs.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/platforms/pseries/iommu.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 07f0847..189717b 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
of_reconfig_notifier_register(&iommu_reconfig_nb);
register_memory_notifier(&iommu_mem_nb);
- /*
- * Secure guest memory is inacessible to devices so regular DMA isn't
- * possible.
- *
- * In that case keep devices' dma_map_ops as NULL so that the generic
- * DMA code path will use SWIOTLB to bounce buffers for DMA.
- */
- if (!is_secure_guest())
- set_pci_dma_ops(&dma_iommu_ops);
+ set_pci_dma_ops(&dma_iommu_ops);
}
static int __init disable_multitce(char *str)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
2019-11-04 21:28 ` Ram Pai
@ 2019-11-07 10:26 ` Michael Ellerman
-1 siblings, 0 replies; 32+ messages in thread
From: Michael Ellerman @ 2019-11-07 10:26 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: benh, david, paulus, mdroth, hch, linuxram, andmike, sukadev,
mst, ram.n.pai, aik, cai, tglx, bauerman, linux-kernel
Ram Pai <linuxram@us.ibm.com> writes:
> This enables IOMMU support for pseries Secure VMs.
Can you give us some more explanation please?
This is basically a revert of commit:
edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")
But neglects to remove the now unnecessary include of svm.h.
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 07f0847..189717b 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
> of_reconfig_notifier_register(&iommu_reconfig_nb);
> register_memory_notifier(&iommu_mem_nb);
>
> - /*
> - * Secure guest memory is inacessible to devices so regular DMA isn't
> - * possible.
> - *
> - * In that case keep devices' dma_map_ops as NULL so that the generic
> - * DMA code path will use SWIOTLB to bounce buffers for DMA.
Please explain what has changed to make this no longer necessary.
cheers
> - */
> - if (!is_secure_guest())
> - set_pci_dma_ops(&dma_iommu_ops);
> + set_pci_dma_ops(&dma_iommu_ops);
> }
>
> static int __init disable_multitce(char *str)
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
@ 2019-11-07 10:26 ` Michael Ellerman
0 siblings, 0 replies; 32+ messages in thread
From: Michael Ellerman @ 2019-11-07 10:26 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: andmike, mst, aik, linuxram, mdroth, linux-kernel, ram.n.pai,
cai, tglx, sukadev, hch, bauerman, david
Ram Pai <linuxram@us.ibm.com> writes:
> This enables IOMMU support for pseries Secure VMs.
Can you give us some more explanation please?
This is basically a revert of commit:
edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")
But neglects to remove the now unnecessary include of svm.h.
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 07f0847..189717b 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
> of_reconfig_notifier_register(&iommu_reconfig_nb);
> register_memory_notifier(&iommu_mem_nb);
>
> - /*
> - * Secure guest memory is inacessible to devices so regular DMA isn't
> - * possible.
> - *
> - * In that case keep devices' dma_map_ops as NULL so that the generic
> - * DMA code path will use SWIOTLB to bounce buffers for DMA.
Please explain what has changed to make this no longer necessary.
cheers
> - */
> - if (!is_secure_guest())
> - set_pci_dma_ops(&dma_iommu_ops);
> + set_pci_dma_ops(&dma_iommu_ops);
> }
>
> static int __init disable_multitce(char *str)
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
2019-11-07 10:26 ` Michael Ellerman
@ 2019-11-08 5:49 ` Ram Pai
-1 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-08 5:49 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, benh, david, paulus, mdroth, hch, andmike, sukadev,
mst, ram.n.pai, aik, cai, tglx, bauerman, linux-kernel
On Thu, Nov 07, 2019 at 09:26:28PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > This enables IOMMU support for pseries Secure VMs.
>
> Can you give us some more explanation please?
Yes. Will do.
The simple explanation is -- it was a mistake. We should
not have disabled IOMMU ops for secure guests. Though it enabled
us to use virtio devices, with the help of some additional patches to
the virtio subsystem; in hindsight, we should not have disabled IOMMU
ops for secure VMs :-(.
RP
>
> This is basically a revert of commit:
> edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")
>
> But neglects to remove the now unnecessary include of svm.h.
>
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 07f0847..189717b 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
> > of_reconfig_notifier_register(&iommu_reconfig_nb);
> > register_memory_notifier(&iommu_mem_nb);
> >
> > - /*
> > - * Secure guest memory is inacessible to devices so regular DMA isn't
> > - * possible.
> > - *
> > - * In that case keep devices' dma_map_ops as NULL so that the generic
> > - * DMA code path will use SWIOTLB to bounce buffers for DMA.
>
> Please explain what has changed to make this no longer necessary.
>
> cheers
>
> > - */
> > - if (!is_secure_guest())
> > - set_pci_dma_ops(&dma_iommu_ops);
> > + set_pci_dma_ops(&dma_iommu_ops);
> > }
> >
> > static int __init disable_multitce(char *str)
> > --
> > 1.8.3.1
--
Ram Pai
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
@ 2019-11-08 5:49 ` Ram Pai
0 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-08 5:49 UTC (permalink / raw)
To: Michael Ellerman
Cc: andmike, mst, aik, mdroth, linux-kernel, ram.n.pai, cai, tglx,
sukadev, linuxppc-dev, hch, bauerman, david
On Thu, Nov 07, 2019 at 09:26:28PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > This enables IOMMU support for pseries Secure VMs.
>
> Can you give us some more explanation please?
Yes. Will do.
The simple explanation is -- it was a mistake. We should
not have disabled IOMMU ops for secure guests. Though it enabled
us to use virtio devices, with the help of some additional patches to
the virtio subsystem; in hindsight, we should not have disabled IOMMU
ops for secure VMs :-(.
RP
>
> This is basically a revert of commit:
> edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")
>
> But neglects to remove the now unnecessary include of svm.h.
>
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 07f0847..189717b 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
> > of_reconfig_notifier_register(&iommu_reconfig_nb);
> > register_memory_notifier(&iommu_mem_nb);
> >
> > - /*
> > - * Secure guest memory is inacessible to devices so regular DMA isn't
> > - * possible.
> > - *
> > - * In that case keep devices' dma_map_ops as NULL so that the generic
> > - * DMA code path will use SWIOTLB to bounce buffers for DMA.
>
> Please explain what has changed to make this no longer necessary.
>
> cheers
>
> > - */
> > - if (!is_secure_guest())
> > - set_pci_dma_ops(&dma_iommu_ops);
> > + set_pci_dma_ops(&dma_iommu_ops);
> > }
> >
> > static int __init disable_multitce(char *str)
> > --
> > 1.8.3.1
--
Ram Pai
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
2019-11-04 21:28 ` Ram Pai
@ 2019-11-06 1:58 ` Alexey Kardashevskiy
-1 siblings, 0 replies; 32+ messages in thread
From: Alexey Kardashevskiy @ 2019-11-06 1:58 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: benh, david, mpe, paulus, mdroth, hch, andmike, sukadev, mst,
ram.n.pai, cai, tglx, bauerman, linux-kernel
On 05/11/2019 08:28, Ram Pai wrote:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
> #include <asm/mmzone.h>
> #include <asm/plpar_wrappers.h>
> #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>
> #include "pseries.h"
>
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>
> static DEFINE_PER_CPU(__be64 *, tce_page);
>
> +/*
> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> + if (tcep && is_secure_guest())
> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
There is no matching unshare in this patch.
> +
> + return tcep;
> +}
> +
> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long npages, unsigned long uaddr,
> enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> * from iommu_alloc{,_sg}()
> */
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> - /* If allocation fails, fall back to the loop implementation */
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> return rc;
> }
>
> +
Unrelated.
> static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> tcep = __this_cpu_read(tce_page);
>
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_enable();
> return -ENOMEM;
>
--
Alexey
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-06 1:58 ` Alexey Kardashevskiy
0 siblings, 0 replies; 32+ messages in thread
From: Alexey Kardashevskiy @ 2019-11-06 1:58 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: andmike, mst, mdroth, linux-kernel, ram.n.pai, cai, tglx,
sukadev, hch, bauerman, david
On 05/11/2019 08:28, Ram Pai wrote:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
> #include <asm/mmzone.h>
> #include <asm/plpar_wrappers.h>
> #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>
> #include "pseries.h"
>
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>
> static DEFINE_PER_CPU(__be64 *, tce_page);
>
> +/*
> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> + if (tcep && is_secure_guest())
> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
There is no matching unshare in this patch.
> +
> + return tcep;
> +}
> +
> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long npages, unsigned long uaddr,
> enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> * from iommu_alloc{,_sg}()
> */
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> - /* If allocation fails, fall back to the loop implementation */
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> return rc;
> }
>
> +
Unrelated.
> static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> tcep = __this_cpu_read(tce_page);
>
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_enable();
> return -ENOMEM;
>
--
Alexey
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
2019-11-06 1:58 ` Alexey Kardashevskiy
@ 2019-11-06 17:01 ` Ram Pai
-1 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-06 17:01 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linuxppc-dev, benh, david, mpe, paulus, mdroth, hch, andmike,
sukadev, mst, ram.n.pai, cai, tglx, bauerman, linux-kernel
On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 05/11/2019 08:28, Ram Pai wrote:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 8d9c2b1..07f0847 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -37,6 +37,7 @@
> > #include <asm/mmzone.h>
> > #include <asm/plpar_wrappers.h>
> > #include <asm/svm.h>
> > +#include <asm/ultravisor.h>
> >
> > #include "pseries.h"
> >
> > @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
> >
> > static DEFINE_PER_CPU(__be64 *, tce_page);
> >
> > +/*
> > + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> > + */
> > +static __be64 *alloc_tce_page(void)
> > +{
> > + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > +
> > + if (tcep && is_secure_guest())
> > + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
>
>
> There is no matching unshare in this patch.
The page is allocated and shared, and stays that way for the life of the
kernel. It is not explicitly unshared or freed. It is however
implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
when the guest kernel reboots. And it also gets implicitly unshared by
the Ultravisor/Hypervisor, if the SVM abruptly terminates.
>
>
> > +
> > + return tcep;
> > +}
> > +
> > static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> > long npages, unsigned long uaddr,
> > enum dma_data_direction direction,
> > @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> > * from iommu_alloc{,_sg}()
> > */
> > if (!tcep) {
> > - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > - /* If allocation fails, fall back to the loop implementation */
> > + tcep = alloc_tce_page();
> > if (!tcep) {
> > local_irq_restore(flags);
> > return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> > @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> > return rc;
> > }
> >
> > +
>
> Unrelated.
yes. will fix it.
Thanks,
RP
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-06 17:01 ` Ram Pai
0 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-06 17:01 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: andmike, mst, mdroth, linux-kernel, ram.n.pai, cai, tglx,
sukadev, linuxppc-dev, hch, bauerman, david
On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 05/11/2019 08:28, Ram Pai wrote:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 8d9c2b1..07f0847 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -37,6 +37,7 @@
> > #include <asm/mmzone.h>
> > #include <asm/plpar_wrappers.h>
> > #include <asm/svm.h>
> > +#include <asm/ultravisor.h>
> >
> > #include "pseries.h"
> >
> > @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
> >
> > static DEFINE_PER_CPU(__be64 *, tce_page);
> >
> > +/*
> > + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> > + */
> > +static __be64 *alloc_tce_page(void)
> > +{
> > + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > +
> > + if (tcep && is_secure_guest())
> > + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
>
>
> There is no matching unshare in this patch.
The page is allocated and shared, and stays that way for the life of the
kernel. It is not explicitly unshared or freed. It is however
implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
when the guest kernel reboots. And it also gets implicitly unshared by
the Ultravisor/Hypervisor, if the SVM abruptly terminates.
>
>
> > +
> > + return tcep;
> > +}
> > +
> > static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> > long npages, unsigned long uaddr,
> > enum dma_data_direction direction,
> > @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> > * from iommu_alloc{,_sg}()
> > */
> > if (!tcep) {
> > - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > - /* If allocation fails, fall back to the loop implementation */
> > + tcep = alloc_tce_page();
> > if (!tcep) {
> > local_irq_restore(flags);
> > return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> > @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> > return rc;
> > }
> >
> > +
>
> Unrelated.
yes. will fix it.
Thanks,
RP
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
2019-11-06 17:01 ` Ram Pai
@ 2019-11-07 5:58 ` Alexey Kardashevskiy
-1 siblings, 0 replies; 32+ messages in thread
From: Alexey Kardashevskiy @ 2019-11-07 5:58 UTC (permalink / raw)
To: Ram Pai
Cc: linuxppc-dev, benh, david, mpe, paulus, mdroth, hch, andmike,
sukadev, mst, ram.n.pai, cai, tglx, bauerman, linux-kernel
On 07/11/2019 04:01, Ram Pai wrote:
> On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 05/11/2019 08:28, Ram Pai wrote:
>>> The hypervisor needs to access the contents of the page holding the TCE
>>> entries while setting up the TCE entries in the IOMMU's TCE table. For
>>> SecureVMs, since this page is encrypted, the hypervisor cannot access
>>> valid entries. Share the page with the hypervisor. This ensures that the
>>> hypervisor sees the valid entries.
>>>
>>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
>>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 8d9c2b1..07f0847 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -37,6 +37,7 @@
>>> #include <asm/mmzone.h>
>>> #include <asm/plpar_wrappers.h>
>>> #include <asm/svm.h>
>>> +#include <asm/ultravisor.h>
>>>
>>> #include "pseries.h"
>>>
>>> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>>
>>> static DEFINE_PER_CPU(__be64 *, tce_page);
>>>
>>> +/*
>>> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
>>> + */
>>> +static __be64 *alloc_tce_page(void)
>>> +{
>>> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> +
>>> + if (tcep && is_secure_guest())
>>> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
>>
>>
>> There is no matching unshare in this patch.
>
> The page is allocated and shared, and stays that way for the life of the
> kernel. It is not explicitly unshared or freed.
Ah, fair enough, I missed that, strange that we do not free it but ok. Thanks,
> It is however
> implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
> when the guest kernel reboots. And it also gets implicitly unshared by
> the Ultravisor/Hypervisor, if the SVM abruptly terminates.
>
>>
>>
>>> +
>>> + return tcep;
>>> +}
>>> +
>>> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>> long npages, unsigned long uaddr,
>>> enum dma_data_direction direction,
>>> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>> * from iommu_alloc{,_sg}()
>>> */
>>> if (!tcep) {
>>> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> - /* If allocation fails, fall back to the loop implementation */
>>> + tcep = alloc_tce_page();
>>> if (!tcep) {
>>> local_irq_restore(flags);
>>> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
>>> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>>> return rc;
>>> }
>>>
>>> +
>>
>> Unrelated.
>
> yes. will fix it.
>
> Thanks,
> RP
>
--
Alexey
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-07 5:58 ` Alexey Kardashevskiy
0 siblings, 0 replies; 32+ messages in thread
From: Alexey Kardashevskiy @ 2019-11-07 5:58 UTC (permalink / raw)
To: Ram Pai
Cc: andmike, mst, mdroth, linux-kernel, ram.n.pai, cai, tglx,
sukadev, linuxppc-dev, hch, bauerman, david
On 07/11/2019 04:01, Ram Pai wrote:
> On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 05/11/2019 08:28, Ram Pai wrote:
>>> The hypervisor needs to access the contents of the page holding the TCE
>>> entries while setting up the TCE entries in the IOMMU's TCE table. For
>>> SecureVMs, since this page is encrypted, the hypervisor cannot access
>>> valid entries. Share the page with the hypervisor. This ensures that the
>>> hypervisor sees the valid entries.
>>>
>>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>> ---
>>> arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
>>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 8d9c2b1..07f0847 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -37,6 +37,7 @@
>>> #include <asm/mmzone.h>
>>> #include <asm/plpar_wrappers.h>
>>> #include <asm/svm.h>
>>> +#include <asm/ultravisor.h>
>>>
>>> #include "pseries.h"
>>>
>>> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>>
>>> static DEFINE_PER_CPU(__be64 *, tce_page);
>>>
>>> +/*
>>> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
>>> + */
>>> +static __be64 *alloc_tce_page(void)
>>> +{
>>> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> +
>>> + if (tcep && is_secure_guest())
>>> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
>>
>>
>> There is no matching unshare in this patch.
>
> The page is allocated and shared, and stays that way for the life of the
> kernel. It is not explicitly unshared or freed.
Ah, fair enough, I missed that, strange that we do not free it but ok. Thanks,
> It is however
> implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
> when the guest kernel reboots. And it also gets implicitly unshared by
> the Ultravisor/Hypervisor, if the SVM abruptly terminates.
>
>>
>>
>>> +
>>> + return tcep;
>>> +}
>>> +
>>> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>> long npages, unsigned long uaddr,
>>> enum dma_data_direction direction,
>>> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>> * from iommu_alloc{,_sg}()
>>> */
>>> if (!tcep) {
>>> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> - /* If allocation fails, fall back to the loop implementation */
>>> + tcep = alloc_tce_page();
>>> if (!tcep) {
>>> local_irq_restore(flags);
>>> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
>>> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>>> return rc;
>>> }
>>>
>>> +
>>
>> Unrelated.
>
> yes. will fix it.
>
> Thanks,
> RP
>
--
Alexey
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
2019-11-04 21:28 ` Ram Pai
@ 2019-11-07 10:29 ` Michael Ellerman
-1 siblings, 0 replies; 32+ messages in thread
From: Michael Ellerman @ 2019-11-07 10:29 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: benh, david, paulus, mdroth, hch, linuxram, andmike, sukadev,
mst, ram.n.pai, aik, cai, tglx, bauerman, linux-kernel
Ram Pai <linuxram@us.ibm.com> writes:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.
Can you please give people some explanation of why this is safe. After
all the point of the Ultravisor is to protect the guest from a malicious
hypervisor. Giving the hypervisor access to a page of TCEs sounds
dangerous, so please explain why it's not.
cheers
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
> #include <asm/mmzone.h>
> #include <asm/plpar_wrappers.h>
> #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>
> #include "pseries.h"
>
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>
> static DEFINE_PER_CPU(__be64 *, tce_page);
>
> +/*
> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> + if (tcep && is_secure_guest())
> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
> +
> + return tcep;
> +}
> +
> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long npages, unsigned long uaddr,
> enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> * from iommu_alloc{,_sg}()
> */
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> - /* If allocation fails, fall back to the loop implementation */
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> return rc;
> }
>
> +
> static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> tcep = __this_cpu_read(tce_page);
>
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_enable();
> return -ENOMEM;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-07 10:29 ` Michael Ellerman
0 siblings, 0 replies; 32+ messages in thread
From: Michael Ellerman @ 2019-11-07 10:29 UTC (permalink / raw)
To: Ram Pai, linuxppc-dev
Cc: andmike, mst, aik, linuxram, mdroth, linux-kernel, ram.n.pai,
cai, tglx, sukadev, hch, bauerman, david
Ram Pai <linuxram@us.ibm.com> writes:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.
Can you please give people some explanation of why this is safe. After
all the point of the Ultravisor is to protect the guest from a malicious
hypervisor. Giving the hypervisor access to a page of TCEs sounds
dangerous, so please explain why it's not.
cheers
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
> #include <asm/mmzone.h>
> #include <asm/plpar_wrappers.h>
> #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>
> #include "pseries.h"
>
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>
> static DEFINE_PER_CPU(__be64 *, tce_page);
>
> +/*
> + * Allocate a tce page. If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> + __be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> + if (tcep && is_secure_guest())
> + uv_share_page(PHYS_PFN(__pa(tcep)), 1);
> +
> + return tcep;
> +}
> +
> static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long npages, unsigned long uaddr,
> enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> * from iommu_alloc{,_sg}()
> */
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> - /* If allocation fails, fall back to the loop implementation */
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> return rc;
> }
>
> +
> static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> unsigned long num_pfn, const void *arg)
> {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
> tcep = __this_cpu_read(tce_page);
>
> if (!tcep) {
> - tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> + tcep = alloc_tce_page();
> if (!tcep) {
> local_irq_enable();
> return -ENOMEM;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
2019-11-07 10:29 ` Michael Ellerman
@ 2019-11-08 6:05 ` Ram Pai
-1 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-08 6:05 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, benh, david, paulus, mdroth, hch, andmike, sukadev,
mst, ram.n.pai, aik, cai, tglx, bauerman, linux-kernel
On Thu, Nov 07, 2019 at 09:29:54PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
>
> Can you please give people some explanation of why this is safe. After
> all the point of the Ultravisor is to protect the guest from a malicious
> hypervisor. Giving the hypervisor access to a page of TCEs sounds
> dangerous, so please explain why it's not.
Yes. will do, in my next version of the patch.
BTW: this page, which is shareed with the hypervisor contains nothing
but TCE entries. The hypervisor has a need to see those entries, so that it
can update the TCE table with correct entires.
Yes, a malicious hypervisor may try to update the TCE table with entries
that point to incorrect memory location. But doing so will not help the
hypervisor to steal any data from those memory location, because those
memory location; if accessed by the hypervisor, will only fetch
encrypted data.
At most it can lead to denial of service, but not stolen data.
RP
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
@ 2019-11-08 6:05 ` Ram Pai
0 siblings, 0 replies; 32+ messages in thread
From: Ram Pai @ 2019-11-08 6:05 UTC (permalink / raw)
To: Michael Ellerman
Cc: andmike, mst, aik, mdroth, linux-kernel, ram.n.pai, cai, tglx,
sukadev, linuxppc-dev, hch, bauerman, david
On Thu, Nov 07, 2019 at 09:29:54PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
>
> Can you please give people some explanation of why this is safe. After
> all the point of the Ultravisor is to protect the guest from a malicious
> hypervisor. Giving the hypervisor access to a page of TCEs sounds
> dangerous, so please explain why it's not.
Yes. will do, in my next version of the patch.
BTW: this page, which is shareed with the hypervisor contains nothing
but TCE entries. The hypervisor has a need to see those entries, so that it
can update the TCE table with correct entires.
Yes, a malicious hypervisor may try to update the TCE table with entries
that point to incorrect memory location. But doing so will not help the
hypervisor to steal any data from those memory location, because those
memory location; if accessed by the hypervisor, will only fetch
encrypted data.
At most it can lead to denial of service, but not stolen data.
RP
^ permalink raw reply [flat|nested] 32+ messages in thread