On 28/02/2019 01:33, Greg Kurz wrote:
> On Wed, 27 Feb 2019 19:51:47 +1100
> Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
>
>> On sPAPR vfio_listener_region_add() is called in 2 situations:
>> 1. a new listener is registered from vfio_connect_container();
>> 2. a new IOMMU Memory Region is added from rtas_ibm_create_pe_dma_window().
>>
>> In both cases vfio_listener_region_add() calls
>> memory_region_iommu_replay() to notify newly registered IOMMU notifiers
>> about existing mappings which is totally desirable for case 1.
>>
>> However for case 2 it is nothing but noop as the window has just been
>> created and has no valid mappings so replaying those does not do anything.
>> It is barely noticeable with usual guests but if the window happens to be
>> really big, such no-op replay might take minutes and trigger RCU stall
>> warnings in the guest.
>>
>> For example, a upcoming GPU RAM memory region mapped at 64TiB (right
>> after SPAPR_PCI_LIMIT) causes a 64bit DMA window to be at least 128TiB
>> which is (128<<40)/0x10000=2.147.483.648 TCEs to replay.
>>
>> This mitigates the problem by adding an "skipping_replay" flag to
>> sPAPRTCETable and defining sPAPR own IOMMU MR replay() hook which does
>> exactly the same thing as the generic one except it returns early if
>> @skipping_replay==true.
>>
>> When "ibm,create-pe-dma-window" is complete, the guest will map only
>> required regions of the huge DMA window.
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> ---
>> include/hw/ppc/spapr.h | 1 +
>> hw/ppc/spapr_iommu.c | 31 +++++++++++++++++++++++++++++++
>> hw/ppc/spapr_rtas_ddw.c | 7 +++++++
>> 3 files changed, 39 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 86b0488..358bb38 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -727,6 +727,7 @@ struct sPAPRTCETable {
>> uint64_t *mig_table;
>> bool bypass;
>> bool need_vfio;
>> + bool skipping_replay;
>> int fd;
>> MemoryRegion root;
>> IOMMUMemoryRegion iommu;
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index 37e98f9..8f23179 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -141,6 +141,36 @@ static IOMMUTLBEntry
>> spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
>> return ret;
>> }
>>
>> +static void spapr_tce_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>> +{
>> + MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>> + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
>> + hwaddr addr, granularity;
>> + IOMMUTLBEntry iotlb;
>> + sPAPRTCETable *tcet = container_of(iommu_mr, sPAPRTCETable, iommu);
>> +
>> + if (tcet->skipping_replay) {
>> + return;
>> + }
>> +
>> + granularity = memory_region_iommu_get_min_page_size(iommu_mr);
>> +
>> + for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
>> + iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
>> + if (iotlb.perm != IOMMU_NONE) {
>> + n->notify(n, &iotlb);
>> + }
>> +
>> + /*
>> + * if (2^64 - MR size) < granularity, it's possible to get an
>> + * infinite loop here. This should catch such a wraparound.
>> + */
>> + if ((addr + granularity) < addr) {
>> + break;
>> + }
>> + }
>> +}
>
> It is a bit unfortunate to duplicate all that code. What about making
> a memory_region_iommu_replay_generic() helper out of it and call it
> from spapr_tce_replay() and memory_region_iommu_replay() ?
I really do not want to mess with generic code to solve our local sPAPR
problem, especially when there is a way not to do so.
And as a next step, I was thinking of removing (i.e. making this replay
a no-op) from QEMU later and do replay in KVM instead when an IOMMU
group is attaching to KVM as this is the only case when we need replay
and KVM has a lot better idea what TCEs are actually valid and can skip
most of them. This is a bit bigger thing as it requires a KVM capability
"KVM replays mappings" but when we get it, spapr_tce_replay() will
become no-op.
> Apart from that, LGTM.
Well. It is a hack, I just do not have taste to tell how nasty it is :)
>
>> +
>> static int spapr_tce_table_pre_save(void *opaque)
>> {
>> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
>> @@ -659,6 +689,7 @@ static void
>> spapr_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>> IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>>
>> imrc->translate = spapr_tce_translate_iommu;
>> + imrc->replay = spapr_tce_replay;
>> imrc->get_min_page_size = spapr_tce_get_min_page_size;
>> imrc->notify_flag_changed = spapr_tce_notify_flag_changed;
>> imrc->get_attr = spapr_tce_get_attr;
>> diff --git a/hw/ppc/spapr_rtas_ddw.c b/hw/ppc/spapr_rtas_ddw.c
>> index cb8a410..9cc020d 100644
>> --- a/hw/ppc/spapr_rtas_ddw.c
>> +++ b/hw/ppc/spapr_rtas_ddw.c
>> @@ -171,8 +171,15 @@ static void rtas_ibm_create_pe_dma_window(PowerPCCPU
>> *cpu,
>> }
>>
>> win_addr = (windows == 0) ? sphb->dma_win_addr : sphb->dma64_win_addr;
>> + /*
>> + * We have just created a window, we know for the fact that it is empty,
>> + * use a hack to avoid iterating over the table as it is quite possible
>> + * to have billions of TCEs, all empty.
>> + */
>> + tcet->skipping_replay = true;
>> spapr_tce_table_enable(tcet, page_shift, win_addr,
>> 1ULL << (window_shift - page_shift));
>> + tcet->skipping_replay = false;
>> if (!tcet->nb_table) {
>> goto hw_error_exit;
>> }
>
--
Alexey
