| Message ID | 20240429032403.74910-14-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
Hi Mostafa, On 4/29/24 05:23, Mostafa Saleh wrote: > IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs keep > the implementation, while only notify for stage-1 invalidation > in case of nesting. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmuv3.c | 23 +++++++++++++++-------- > hw/arm/trace-events | 2 +- > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index e0fd494646..96d07234fe 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -1051,7 +1051,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > IOMMUNotifier *n, > int asid, int vmid, > dma_addr_t iova, uint8_t tg, > - uint64_t num_pages) > + uint64_t num_pages, int stage) add the new param in the doc comment above > { > SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > IOMMUTLBEvent event; > @@ -1075,14 +1075,21 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > return; > } > > - if (STAGE1_SUPPORTED(s)) { > + /* > + * IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs > + * keep the implementation, while only notify for stage-1 > + * invalidation in case of nesting. > + */ > + if (stage == SMMU_STAGE_1) { > tt = select_tt(cfg, iova); > if (!tt) { > return; > } > granule = tt->granule_sz; > - } else { > + } else if (!STAGE1_SUPPORTED(s)) { I don't get why you don't test stage == SMMU_STAGE_2 instead in each block shouldn't you test if the corresponding state of supported? > granule = cfg->s2cfg.granule_sz; > + } else { I don't really understand the logic here. Please can you comment each case? Thanks Eric > + return; > } > > } else { > @@ -1101,7 +1108,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > /* invalidate an asid/vmid/iova range tuple in all mr's */ > static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, > dma_addr_t iova, uint8_t tg, > - uint64_t num_pages) > + uint64_t num_pages, int stage) > { > SMMUDevice *sdev; > > @@ -1110,10 +1117,10 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, > IOMMUNotifier *n; > > trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid, > - iova, tg, num_pages); > + iova, tg, num_pages, stage); > > IOMMU_NOTIFIER_FOREACH(n, mr) { > - smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages); > + smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages, stage); > } > } > } > @@ -1144,7 +1151,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) > > if (!tg) { > trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage); > - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1); > + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1, stage); > if (stage == SMMU_STAGE_1) { > smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl); > } else { > @@ -1167,7 +1174,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) > num_pages = (mask + 1) >> granule; > trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, > ttl, leaf, stage); > - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages); > + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages, stage); > if (stage == SMMU_STAGE_1) { > smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl); > } else { > diff --git a/hw/arm/trace-events b/hw/arm/trace-events > index 593cc571da..be6c8f720b 100644 > --- a/hw/arm/trace-events > +++ b/hw/arm/trace-events > @@ -55,7 +55,7 @@ smmuv3_cmdq_tlbi_s12_vmid(int vmid) "vmid=%d" > smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x" > smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s" > smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s" > -smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64 > +smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages, int stage) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" stage=%d" > > # strongarm.c > strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
Hi Eric, On Mon, May 20, 2024 at 12:37:55PM +0200, Eric Auger wrote: > Hi Mostafa, > > On 4/29/24 05:23, Mostafa Saleh wrote: > > IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs keep > > the implementation, while only notify for stage-1 invalidation > > in case of nesting. > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmuv3.c | 23 +++++++++++++++-------- > > hw/arm/trace-events | 2 +- > > 2 files changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index e0fd494646..96d07234fe 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -1051,7 +1051,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > IOMMUNotifier *n, > > int asid, int vmid, > > dma_addr_t iova, uint8_t tg, > > - uint64_t num_pages) > > + uint64_t num_pages, int stage) > add the new param in the doc comment above > > { > > SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > > IOMMUTLBEvent event; > > @@ -1075,14 +1075,21 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > return; > > } > > > > - if (STAGE1_SUPPORTED(s)) { > > + /* > > + * IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs > > + * keep the implementation, while only notify for stage-1 > > + * invalidation in case of nesting. > > + */ > > + if (stage == SMMU_STAGE_1) { > > tt = select_tt(cfg, iova); > > if (!tt) { > > return; > > } > > granule = tt->granule_sz; > > - } else { > > + } else if (!STAGE1_SUPPORTED(s)) { > I don't get why you don't test stage == SMMU_STAGE_2 instead > in each block shouldn't you test if the corresponding state of supported? > > granule = cfg->s2cfg.granule_sz; > > + } else { > I don't really understand the logic here. Please can you comment each case? The current implementation will call memory_region_notify_iommu_one() from smmuv3_notify_iova() for stage-1 or stage-2 based on which one is supported and in each case this address is considered an “IOVA”. However, with nested translation memory_region_notify_iommu_one() doesn’t distinguish between stage-1 and stage-2, so only stage-1 is considered “IOVA”. And the implementation basically as follows: 1) If the translation was stage-1, it’s an IOVA and memory_region_notify_iommu_one() is called. 2) If stage-1 is not supported (this is an stage-2 only instance) maintain the old behaviour by calling memory_region_notify_iommu_one() 3) This leaves us with stage-1 being supported and this is a stage-2 translation, where the notification would be ignored, I think in this case if the SW configured only for stage-2 it would expect it to behave as 2) :/ Not sure how to fix that, maybe only ignore stage-2 if it was in a nested STE, or just or always ignore stage-2? Thanks, Mostafa > > Thanks > > Eric > > + return; > > } > > > > } else { > > @@ -1101,7 +1108,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > /* invalidate an asid/vmid/iova range tuple in all mr's */ > > static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, > > dma_addr_t iova, uint8_t tg, > > - uint64_t num_pages) > > + uint64_t num_pages, int stage) > > { > > SMMUDevice *sdev; > > > > @@ -1110,10 +1117,10 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, > > IOMMUNotifier *n; > > > > trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid, > > - iova, tg, num_pages); > > + iova, tg, num_pages, stage); > > > > IOMMU_NOTIFIER_FOREACH(n, mr) { > > - smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages); > > + smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages, stage); > > } > > } > > } > > @@ -1144,7 +1151,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) > > > > if (!tg) { > > trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage); > > - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1); > > + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1, stage); > > if (stage == SMMU_STAGE_1) { > > smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl); > > } else { > > @@ -1167,7 +1174,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) > > num_pages = (mask + 1) >> granule; > > trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, > > ttl, leaf, stage); > > - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages); > > + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages, stage); > > if (stage == SMMU_STAGE_1) { > > smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl); > > } else { > > diff --git a/hw/arm/trace-events b/hw/arm/trace-events > > index 593cc571da..be6c8f720b 100644 > > --- a/hw/arm/trace-events > > +++ b/hw/arm/trace-events > > @@ -55,7 +55,7 @@ smmuv3_cmdq_tlbi_s12_vmid(int vmid) "vmid=%d" > > smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x" > > smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s" > > smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s" > > -smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64 > > +smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages, int stage) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" stage=%d" > > > > # strongarm.c > > strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d" >
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index e0fd494646..96d07234fe 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -1051,7 +1051,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, IOMMUNotifier *n, int asid, int vmid, dma_addr_t iova, uint8_t tg, - uint64_t num_pages) + uint64_t num_pages, int stage) { SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); IOMMUTLBEvent event; @@ -1075,14 +1075,21 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, return; } - if (STAGE1_SUPPORTED(s)) { + /* + * IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs + * keep the implementation, while only notify for stage-1 + * invalidation in case of nesting. + */ + if (stage == SMMU_STAGE_1) { tt = select_tt(cfg, iova); if (!tt) { return; } granule = tt->granule_sz; - } else { + } else if (!STAGE1_SUPPORTED(s)) { granule = cfg->s2cfg.granule_sz; + } else { + return; } } else { @@ -1101,7 +1108,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, /* invalidate an asid/vmid/iova range tuple in all mr's */ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova, uint8_t tg, - uint64_t num_pages) + uint64_t num_pages, int stage) { SMMUDevice *sdev; @@ -1110,10 +1117,10 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid, IOMMUNotifier *n; trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid, - iova, tg, num_pages); + iova, tg, num_pages, stage); IOMMU_NOTIFIER_FOREACH(n, mr) { - smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages); + smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages, stage); } } } @@ -1144,7 +1151,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) if (!tg) { trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf, stage); - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1); + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1, stage); if (stage == SMMU_STAGE_1) { smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl); } else { @@ -1167,7 +1174,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd, SMMUStage stage) num_pages = (mask + 1) >> granule; trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf, stage); - smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages); + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages, stage); if (stage == SMMU_STAGE_1) { smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl); } else { diff --git a/hw/arm/trace-events b/hw/arm/trace-events index 593cc571da..be6c8f720b 100644 --- a/hw/arm/trace-events +++ b/hw/arm/trace-events @@ -55,7 +55,7 @@ smmuv3_cmdq_tlbi_s12_vmid(int vmid) "vmid=%d" smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x" smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s" smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s" -smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64 +smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t iova, uint8_t tg, uint64_t num_pages, int stage) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" stage=%d" # strongarm.c strongarm_uart_update_parameters(const char *label, int speed, char parity, int data_bits, int stop_bits) "%s speed=%d parity=%c data=%d stop=%d"
IOMMUTLBEvent only understands IOVA, for stage-2 only SMMUs keep the implementation, while only notify for stage-1 invalidation in case of nesting. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmuv3.c | 23 +++++++++++++++-------- hw/arm/trace-events | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-)