Hi, On 5/27/20 7:30 PM, Robin Murphy wrote: > On 2020-05-27 17:03, Srinath Mannam wrote: >> This patch gives the provision to change default value of MSI IOVA base >> to platform's suitable IOVA using module parameter. The present >> hardcoded MSI IOVA base may not be the accessible IOVA ranges of >> platform. > > That in itself doesn't seem entirely unreasonable; IIRC the current > address is just an arbitrary choice to fit nicely into Qemu's memory > map, correct and there was always the possibility that it wouldn't suit everything.
Indeed I also recently had this case of PCI host bridge collision with the SW MSI reserved window - maybe that's the same ;-) -. > >> Since commit aadad097cd46 ("iommu/dma: Reserve IOVA for PCIe inaccessible >> DMA address"), inaccessible IOVA address ranges parsed from dma-ranges >> property are reserved. > > That, however, doesn't seem to fit here; iommu-dma maps MSI doorbells > dynamically, so they aren't affected by reserved regions any more than > regular DMA pages are. In fact, it explicitly ignores the software MSI > region, since as the comment says, it *is* the software that manages those. > > The MSI_IOVA_BASE region exists for VFIO, precisely because in that case > the kernel *doesn't* control the address space, but still needs some way > to steal a bit of it for MSIs that the guest doesn't necessarily know > about, and give userspace a fighting chance of knowing what it's taken. > I think at the time we discussed the idea of adding something to the > VFIO uapi such that userspace could move this around if it wanted or > needed to, but decided we could live without that initially. Yes indeed ;-) Perhaps now > the time has come? Do you mean you would welcome a VFIO based approach or would a driver parameter be sufficient? Thanks Eric > > Robin. > >> If any platform has the limitaion to access default MSI IOVA, then it can >> be changed using "arm-smmu.msi_iova_base=0xa0000000" command line >> argument. >> >> Signed-off-by: Srinath Mannam <srinath.man...@broadcom.com> >> --- >> drivers/iommu/arm-smmu.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 4f1a350..5e59c9d 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -72,6 +72,9 @@ static bool disable_bypass = >> module_param(disable_bypass, bool, S_IRUGO); >> MODULE_PARM_DESC(disable_bypass, >> "Disable bypass streams such that incoming transactions from >> devices that are not attached to an iommu domain will report an abort >> back to the device and will not be allowed to pass through the SMMU."); >> +static unsigned long msi_iova_base = MSI_IOVA_BASE; >> +module_param(msi_iova_base, ulong, S_IRUGO); >> +MODULE_PARM_DESC(msi_iova_base, "msi iova base address."); >> struct arm_smmu_s2cr { >> struct iommu_group *group; >> @@ -1566,7 +1569,7 @@ static void arm_smmu_get_resv_regions(struct >> device *dev, >> struct iommu_resv_region *region; >> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; >> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, >> + region = iommu_alloc_resv_region(msi_iova_base, MSI_IOVA_LENGTH, >> prot, IOMMU_RESV_SW_MSI); >> if (!region) >> return; >> >