This patch series enables IOMMU support for pseries Secure VMs.
Tested using QEMU command line option:
"-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
iommu_platform=on,disable-modern=off,disable-legacy=on"
and
"-device virtio-blk-pci,scsi=off,bus=pci.0,
addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
iommu_platform=on,disable-modern=off,disable-legacy=on"
Ram Pai (2):
powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
--
1.8.3.1
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On 05/11/2019 08:28, Ram Pai wrote:
> This patch series enables IOMMU support for pseries Secure VMs.
>
>
> Tested using QEMU command line option:
>
> "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> iommu_platform=on,disable-modern=off,disable-legacy=on"
> and
>
> "-device virtio-blk-pci,scsi=off,bus=pci.0,
> addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> iommu_platform=on,disable-modern=off,disable-legacy=on"
Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
>
> Ram Pai (2):
> powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
> powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
>
> arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
--
Alexey
On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 05/11/2019 08:28, Ram Pai wrote:
> > This patch series enables IOMMU support for pseries Secure VMs.
> >
> >
> > Tested using QEMU command line option:
> >
> > "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > iommu_platform=on,disable-modern=off,disable-legacy=on"
> > and
> >
> > "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > iommu_platform=on,disable-modern=off,disable-legacy=on"
>
>
> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
I did see SLOF complaining, but that did not stop it from booting the guest.
My boot device was a virtio device, with iommu platform flag
enabled.
So I am not sure, under what conditions SLOF will fail to boot. Any
idea?
RP
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 <[email protected]>
> > ---
> > 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
On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 05/11/2019 08:28, Ram Pai wrote:
> > This patch series enables IOMMU support for pseries Secure VMs.
> >
> >
> > Tested using QEMU command line option:
> >
> > "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > iommu_platform=on,disable-modern=off,disable-legacy=on"
> > and
> >
> > "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > iommu_platform=on,disable-modern=off,disable-legacy=on"
>
>
> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
Shouldn't be hard to support: set up the iommu to allow everything
and ack the feature. Right?
> >
> > Ram Pai (2):
> > powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
> > powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
> >
> > arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
> > 1 file changed, 18 insertions(+), 12 deletions(-)
> >
>
> --
> Alexey
Quoting Michael S. Tsirkin (2019-11-06 12:06:37)
> On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> >
> >
> > On 05/11/2019 08:28, Ram Pai wrote:
> > > This patch series enables IOMMU support for pseries Secure VMs.
> > >
> > >
> > > Tested using QEMU command line option:
> > >
> > > "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > > iommu_platform=on,disable-modern=off,disable-legacy=on"
> > > and
> > >
> > > "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > > addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > > iommu_platform=on,disable-modern=off,disable-legacy=on"
> >
> >
> > Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
>
> Shouldn't be hard to support: set up the iommu to allow everything
> and ack the feature. Right?
It's not a static/linear mapping in this case so we need calls to map DMA
buffers as-needed. I've gotten it to boot with virtio-blk, but the patches
have some hacks and need cleanup, hoping to post them soon.
I'm a bit perplexed how we would manage to boot a guest without those
changes though, this is what I get with qemu 4.1.0:
qemu-system-ppc64 -M pseries,ic-mode=xics -m 512M -bios /home/mdroth/w/build/qemu-4.1.0-build/pc-bios/slof.bin -device virtio-blk-pci,drive=drive0,id=blk0,disable-modern=off,disable-legacy=on,iommu_platform=on -drive file=/home/mdroth/vm/bionic-server-cloudimg-ppc64el.img,if=none,id=drive0 -trace enable=spapr_iommu\*,file=trace.out -monitor unix:/tmp/mon.sock,server,nowait -vga none -nographic
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
SLOF **********************************************************************
QEMU Starting
Build Date = Jul 3 2019 12:26:14
FW Version = git-ba1ab360eebe6338
Press "s" to enter Open Firmware.
Populating /vdevice methods
Populating /vdevice/vty@71000000
Populating /vdevice/nvram@71000001
Populating /vdevice/l-lan@71000002
Populating /vdevice/v-scsi@71000003
SCSI: Looking for devices
8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+"
Populating /pci@800000020000000
00 0000 (D) : 1af4 1042 virtio [ block ]
No NVRAM common partition, re-initializing...
Scanning USB
Using default console: /vdevice/vty@71000000
Welcome to Open Firmware
Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
This program and the accompanying materials are made available
under the terms of the BSD License available at
http://www.opensource.org/licenses/bsd-license.php
Trying to load: from: /pci@800000020000000/scsi@0 ... virtioblk_init: failed
virtioblk_transfer: Access beyond end of device!
And then it hangs. This is with TCG so maybe it behaves differently with
KVM, but that's the result I would expect with the current SLOF code.
>
> > >
> > > Ram Pai (2):
> > > powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
> > > powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
> > >
> > > arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
> > > 1 file changed, 18 insertions(+), 12 deletions(-)
> > >
> >
> > --
> > Alexey
>
On Wed, Nov 06, 2019 at 12:52:02PM -0600, Michael Roth wrote:
> Quoting Michael S. Tsirkin (2019-11-06 12:06:37)
> > On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> > >
> > >
> > > On 05/11/2019 08:28, Ram Pai wrote:
> > > > This patch series enables IOMMU support for pseries Secure VMs.
> > > >
> > > >
> > > > Tested using QEMU command line option:
> > > >
> > > > "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > > > iommu_platform=on,disable-modern=off,disable-legacy=on"
> > > > and
> > > >
> > > > "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > > > addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > > > iommu_platform=on,disable-modern=off,disable-legacy=on"
> > >
> > >
> > > Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
> >
> > Shouldn't be hard to support: set up the iommu to allow everything
> > and ack the feature. Right?
>
> It's not a static/linear mapping in this case so we need calls to map DMA
> buffers as-needed. I've gotten it to boot with virtio-blk, but the patches
> have some hacks and need cleanup, hoping to post them soon.
>
> I'm a bit perplexed how we would manage to boot a guest without those
> changes though, this is what I get with qemu 4.1.0:
>
> qemu-system-ppc64 -M pseries,ic-mode=xics -m 512M -bios /home/mdroth/w/build/qemu-4.1.0-build/pc-bios/slof.bin -device virtio-blk-pci,drive=drive0,id=blk0,disable-modern=off,disable-legacy=on,iommu_platform=on -drive file=/home/mdroth/vm/bionic-server-cloudimg-ppc64el.img,if=none,id=drive0 -trace enable=spapr_iommu\*,file=trace.out -monitor unix:/tmp/mon.sock,server,nowait -vga none -nographic
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
>
>
> SLOF **********************************************************************
> QEMU Starting
> Build Date = Jul 3 2019 12:26:14
> FW Version = git-ba1ab360eebe6338
> Press "s" to enter Open Firmware.
>
> Populating /vdevice methods
> Populating /vdevice/vty@71000000
> Populating /vdevice/nvram@71000001
> Populating /vdevice/l-lan@71000002
> Populating /vdevice/v-scsi@71000003
> SCSI: Looking for devices
> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+"
> Populating /pci@800000020000000
> 00 0000 (D) : 1af4 1042 virtio [ block ]
> No NVRAM common partition, re-initializing...
> Scanning USB
> Using default console: /vdevice/vty@71000000
>
> Welcome to Open Firmware
>
> Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
> This program and the accompanying materials are made available
> under the terms of the BSD License available at
> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.opensource.org_licenses_bsd-2Dlicense.php&d=DwIFaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=m-UrKChQVkZtnPpjbF6YY99NbT8FBByQ-E-ygV8luxw&m=-xG4gaWE7BANKGOFx0wmF5ZgZVd8A1r-tsN44n4JUW4&s=QcpPgRqeZAk1pICsA-kk2gNKKiMJLASiPVi-hPinur0&e=
>
>
> Trying to load: from: /pci@800000020000000/scsi@0 ... virtioblk_init: failed
> virtioblk_transfer: Access beyond end of device!
>
> And then it hangs. This is with TCG so maybe it behaves differently with
> KVM, but that's the result I would expect with the current SLOF code.
you are right. In my case, I was providing the kernel image
on the qemu command line. So there was no need for SLOF to read the
virtio disk. Hence it continued without hanging. SLOF needs to be enhanced
to support IOMMU.
RP
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 <[email protected]>
>>> ---
>>> 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
On 07/11/2019 05:06, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 05/11/2019 08:28, Ram Pai wrote:
>>> This patch series enables IOMMU support for pseries Secure VMs.
>>>
>>>
>>> Tested using QEMU command line option:
>>>
>>> "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
>>> iommu_platform=on,disable-modern=off,disable-legacy=on"
>>> and
>>>
>>> "-device virtio-blk-pci,scsi=off,bus=pci.0,
>>> addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
>>> iommu_platform=on,disable-modern=off,disable-legacy=on"
>>
>>
>> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
>
> Shouldn't be hard to support: set up the iommu to allow everything
> and ack the feature. Right?
With the defaults we have (32bit window limited by 1GB and SLOF residing close to the end of the second GB), it is not
straight forward; also SLOF's XHCI and E1000 already use IOMMU so we should just do this for virtio as well, I am
hacking it now. Thanks,
>
>>>
>>> Ram Pai (2):
>>> powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
>>> powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
>>>
>>> arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
>>> 1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>
>> --
>> Alexey
>
--
Alexey
Ram Pai <[email protected]> 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
On Thu, Nov 07, 2019 at 09:29:54PM +1100, Michael Ellerman wrote:
> Ram Pai <[email protected]> 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