Message-ID: <be36117d-204e-bf59-287a-371103186e16@ozlabs.ru>
Date:   Wed, 11 Dec 2019 19:36:29 +1100
From:   Alexey Kardashevskiy <aik@...abs.ru>
To:     Michael Roth <mdroth@...ux.vnet.ibm.com>,
        Ram Pai <linuxram@...ibm.com>, mpe@...erman.id.au
Cc:     linuxppc-dev@...ts.ozlabs.org, benh@...nel.crashing.org,
        david@...son.dropbear.id.au, paulus@...abs.org, hch@....de,
        andmike@...ibm.com, sukadev@...ux.vnet.ibm.com, mst@...hat.com,
        ram.n.pai@...il.com, cai@....pw, tglx@...utronix.de,
        bauerman@...ux.ibm.com, linux-kernel@...r.kernel.org,
        leonardo@...ux.ibm.com
Subject: Re: [PATCH v5 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for
 Secure VM.



On 11/12/2019 12:43, Michael Roth wrote:
> Quoting Ram Pai (2019-12-06 19:12:39)
>> Commit edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on
>>                 secure guests")
>> disabled dma_iommu_ops path, for secure VMs. Disabling dma_iommu_ops
>> path for secure VMs, helped enable dma_direct path.  This enabled
>> support for bounce-buffering through SWIOTLB.  However it fails to
>> operate when IOMMU is enabled, since I/O pages are not TCE mapped.
>>
>> Renable dma_iommu_ops path for pseries Secure VMs.  It handles all
>> cases including, TCE mapping I/O pages, in the presence of a
>> IOMMU.
> 
> Wasn't clear to me at first, but I guess the main gist of this series is
> that we want to continue to use SWIOTLB, but also need to create mappings
> of it's bounce buffers in the IOMMU, so we revert to using dma_iommu_ops
> and rely on the various dma_iommu_{map,alloc}_bypass() hooks throughout
> to call into dma_direct_* ops rather than relying on the dma_is_direct(ops)
> checks in DMA API functions to do the same.


Correct. Took me a bit of time to realize what we got here :) We only
rely on  dma_iommu_ops::.dma_supported to write the DMA offset to a
device (when creating a huge window), and after that we know it is
mapped directly and swiotlb gets this 1<<59 offset via __phys_to_dma().


> That makes sense, but one issue I see with that is that
> dma_iommu_map_bypass() only tests true if all the following are true:
> 
> 1) the device requests a 64-bit DMA mask via
>    dma_set_mask/dma_set_coherent_mask
> 2) DDW is enabled (i.e. we don't pass disable_ddw on command-line)
> 
> dma_is_direct() checks don't have this limitation, so I think for
> anything cases, such as devices that use a smaller DMA mask, we'll
> end up falling back to the non-bypass functions in dma_iommu_ops, which
> will likely break for things like dma_alloc_coherent/dma_map_single
> since they won't use SWIOTLB pages and won't do the necessary calls to
> set_memory_unencrypted() to share those non-SWIOTLB buffers with
> hypervisor.
> 
> Maybe that's ok, but I think we should be clearer about how to
> fail/handle these cases.
> 
> Though I also agree with some concerns Alexey stated earlier: it seems
> wasteful to map the entire DDW window just so these bounce buffers can be
> mapped.  Especially if you consider the lack of a mapping to be an additional
> safe-guard against things like buggy device implementations on the QEMU
> side. E.g. if we leaked pages to the hypervisor on accident, those pages
> wouldn't be immediately accessible to a device, and would still require
> additional work get past the IOMMU.
> 
> What would it look like if we try to make all this work with disable_ddw passed
> to kernel command-line (or forced for is_secure_guest())?
> 
>   1) dma_iommu_{alloc,map}_bypass() would no longer get us to dma_direct_* ops,
>      but an additional case or hook that considers is_secure_guest() might do
>      it.
>      
>   2) We'd also need to set up an IOMMU mapping for the bounce buffers via
>      io_tlb_start/io_tlb_end. We could do it once, on-demand via
>      dma_iommu_bypass_supported() like we do for the 64-bit DDW window, or
>      maybe in some init function.


io_tlb_start/io_tlb_end are only guaranteed to stay within 4GB and our
default DMA window is 1GB (KVM) or 2GB (PowerVM), ok, we can define
ARCH_LOW_ADDRESS_LIMIT as 1GB.

But it has also been mentioned that we are likely to be having swiotlb
buffers outside of the first 4GB as they are not just for crippled
devices any more. So we are likely to have 64bit window, I'd just ditch
the default window then, I have patches for this but every time I
thought I have a use case, turned out that I did not.


> That also has the benefit of not requiring devices to support 64-bit DMA.
> 
> Alternatively, we could continue to rely on the 64-bit DDW window, but
> modify call to enable_ddw() to only map the io_tlb_start/end range in
> the case of is_secure_guest(). This is a little cleaner implementation-wise
> since we can rely on the existing dma_iommu_{alloc,map}_bypass() hooks, but
> devices that don't support 64-bit will fail back to not using dma_direct_* ops
> and fail miserably. We'd probably want to handle that more gracefully.
> 
> Or we handle both cases gracefully. To me it makes more sense to enable
> non-DDW case, then consider adding DDW case later if there's some reason
> why 64-bit DMA is needed. But would be good to hear if there are other
> opinions.


For now we need to do something with the H_PUT_TCE_INDIRECT's page -
either disable multitce (but boot time increases) or share the page. The
patch does the latter. Thanks,


> 
>>
>> Signed-off-by: Ram Pai <linuxram@...ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/iommu.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>> index 67b5009..4e27d66 100644
>> --- a/arch/powerpc/platforms/pseries/iommu.c
>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>> @@ -36,7 +36,6 @@
>>  #include <asm/udbg.h>
>>  #include <asm/mmzone.h>
>>  #include <asm/plpar_wrappers.h>
>> -#include <asm/svm.h>
>>  #include <asm/ultravisor.h>
>>
>>  #include "pseries.h"
>> @@ -1346,15 +1345,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
>>

-- 
Alexey

Powered by blists - more mailing lists