| Message ID | 1500456838-18405-4-git-send-email-anup.patel@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/07/17 10:33, Anup Patel wrote: > The ARM SMMUv3 support bypassing transactions for which domain > is not configured. The patch adds corresponding IOMMU capability > to advertise this fact. > > Signed-off-by: Anup Patel <anup.patel@broadcom.com> > --- > drivers/iommu/arm-smmu-v3.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 568c400..a6c7f66 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) > return true; > case IOMMU_CAP_NOEXEC: > return true; > + case IOMMU_CAP_BYPASS: > + return true; And this is never true. If Linux knows a device masters through the SMMU, it will always have a default domain of some sort (either identity or DMA ops). If Linux doesn't know, then it won't have been able to initialise the stream table for the relevant stream IDs, thus any 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only bypass unknown stream IDs if disabled entirely. Robin. > default: > return false; > } >
On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 19/07/17 10:33, Anup Patel wrote: >> The ARM SMMUv3 support bypassing transactions for which domain >> is not configured. The patch adds corresponding IOMMU capability >> to advertise this fact. >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> >> --- >> drivers/iommu/arm-smmu-v3.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 568c400..a6c7f66 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) >> return true; >> case IOMMU_CAP_NOEXEC: >> return true; >> + case IOMMU_CAP_BYPASS: >> + return true; > > And this is never true. If Linux knows a device masters through the > SMMU, it will always have a default domain of some sort (either identity > or DMA ops). If Linux doesn't know, then it won't have been able to > initialise the stream table for the relevant stream IDs, thus any > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only > bypass unknown stream IDs if disabled entirely. What if we don't want to use IOMMU for certain device and due to this we never provide "iommus" DT attribute in the device DT node. Further, we want to access device without "iommus" DT attribute from user-space using VFIO no-IOMMU. Regards, Anup
On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: > On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: > > On 19/07/17 10:33, Anup Patel wrote: > >> The ARM SMMUv3 support bypassing transactions for which domain > >> is not configured. The patch adds corresponding IOMMU capability > >> to advertise this fact. > >> > >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> > >> --- > >> drivers/iommu/arm-smmu-v3.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >> index 568c400..a6c7f66 100644 > >> --- a/drivers/iommu/arm-smmu-v3.c > >> +++ b/drivers/iommu/arm-smmu-v3.c > >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) > >> return true; > >> case IOMMU_CAP_NOEXEC: > >> return true; > >> + case IOMMU_CAP_BYPASS: > >> + return true; > > > > And this is never true. If Linux knows a device masters through the > > SMMU, it will always have a default domain of some sort (either identity > > or DMA ops). If Linux doesn't know, then it won't have been able to > > initialise the stream table for the relevant stream IDs, thus any > > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only > > bypass unknown stream IDs if disabled entirely. > > What if we don't want to use IOMMU for certain device and > due to this we never provide "iommus" DT attribute in the > device DT node. Further, we want to access device without > "iommus" DT attribute from user-space using VFIO no-IOMMU. Wait, you want to pass a device through to userspace but you don't want to use the IOMMU? Why not? If you describe the SMMU in firmware with only a partial topology description, then you will run into problems with unknown masters trying to perform DMA. That's the IOMMU doing its job! Will
On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> > On 19/07/17 10:33, Anup Patel wrote: >> >> The ARM SMMUv3 support bypassing transactions for which domain >> >> is not configured. The patch adds corresponding IOMMU capability >> >> to advertise this fact. >> >> >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> >> >> --- >> >> drivers/iommu/arm-smmu-v3.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> >> index 568c400..a6c7f66 100644 >> >> --- a/drivers/iommu/arm-smmu-v3.c >> >> +++ b/drivers/iommu/arm-smmu-v3.c >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) >> >> return true; >> >> case IOMMU_CAP_NOEXEC: >> >> return true; >> >> + case IOMMU_CAP_BYPASS: >> >> + return true; >> > >> > And this is never true. If Linux knows a device masters through the >> > SMMU, it will always have a default domain of some sort (either identity >> > or DMA ops). If Linux doesn't know, then it won't have been able to >> > initialise the stream table for the relevant stream IDs, thus any >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only >> > bypass unknown stream IDs if disabled entirely. >> >> What if we don't want to use IOMMU for certain device and >> due to this we never provide "iommus" DT attribute in the >> device DT node. Further, we want to access device without >> "iommus" DT attribute from user-space using VFIO no-IOMMU. > > Wait, you want to pass a device through to userspace but you don't want to > use the IOMMU? Why not? > > If you describe the SMMU in firmware with only a partial topology > description, then you will run into problems with unknown masters trying to > perform DMA. That's the IOMMU doing its job! We are keeping disable_bypass = false. In other words, we are using bypass mode for unmatched streams. The real reason is limited number of SMRs due to which we choose not to provide "iommus" DT attribute for certain devices. Regards, Anup
On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote: > On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <will.deacon@arm.com> wrote: > > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: > >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: > >> > On 19/07/17 10:33, Anup Patel wrote: > >> >> The ARM SMMUv3 support bypassing transactions for which domain > >> >> is not configured. The patch adds corresponding IOMMU capability > >> >> to advertise this fact. > >> >> > >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> > >> >> --- > >> >> drivers/iommu/arm-smmu-v3.c | 2 ++ > >> >> 1 file changed, 2 insertions(+) > >> >> > >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >> >> index 568c400..a6c7f66 100644 > >> >> --- a/drivers/iommu/arm-smmu-v3.c > >> >> +++ b/drivers/iommu/arm-smmu-v3.c > >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) > >> >> return true; > >> >> case IOMMU_CAP_NOEXEC: > >> >> return true; > >> >> + case IOMMU_CAP_BYPASS: > >> >> + return true; > >> > > >> > And this is never true. If Linux knows a device masters through the > >> > SMMU, it will always have a default domain of some sort (either identity > >> > or DMA ops). If Linux doesn't know, then it won't have been able to > >> > initialise the stream table for the relevant stream IDs, thus any > >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only > >> > bypass unknown stream IDs if disabled entirely. > >> > >> What if we don't want to use IOMMU for certain device and > >> due to this we never provide "iommus" DT attribute in the > >> device DT node. Further, we want to access device without > >> "iommus" DT attribute from user-space using VFIO no-IOMMU. > > > > Wait, you want to pass a device through to userspace but you don't want to > > use the IOMMU? Why not? > > > > If you describe the SMMU in firmware with only a partial topology > > description, then you will run into problems with unknown masters trying to > > perform DMA. That's the IOMMU doing its job! > > We are keeping disable_bypass = false. In other words, we > are using bypass mode for unmatched streams. The real > reason is limited number of SMRs due to which we choose > not to provide "iommus" DT attribute for certain devices. Understood, but that's not robust for SMMUv3 and we *really* shouldn't have a user ABI that changes behaviour based on a cmdline option. VFIO should be requesting its own identity mappings, if that's what you need. Will
On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote: >> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <will.deacon@arm.com> wrote: >> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: >> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> >> > On 19/07/17 10:33, Anup Patel wrote: >> >> >> The ARM SMMUv3 support bypassing transactions for which domain >> >> >> is not configured. The patch adds corresponding IOMMU capability >> >> >> to advertise this fact. >> >> >> >> >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> >> >> >> --- >> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++ >> >> >> 1 file changed, 2 insertions(+) >> >> >> >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> >> >> index 568c400..a6c7f66 100644 >> >> >> --- a/drivers/iommu/arm-smmu-v3.c >> >> >> +++ b/drivers/iommu/arm-smmu-v3.c >> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) >> >> >> return true; >> >> >> case IOMMU_CAP_NOEXEC: >> >> >> return true; >> >> >> + case IOMMU_CAP_BYPASS: >> >> >> + return true; >> >> > >> >> > And this is never true. If Linux knows a device masters through the >> >> > SMMU, it will always have a default domain of some sort (either identity >> >> > or DMA ops). If Linux doesn't know, then it won't have been able to >> >> > initialise the stream table for the relevant stream IDs, thus any >> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only >> >> > bypass unknown stream IDs if disabled entirely. >> >> >> >> What if we don't want to use IOMMU for certain device and >> >> due to this we never provide "iommus" DT attribute in the >> >> device DT node. Further, we want to access device without >> >> "iommus" DT attribute from user-space using VFIO no-IOMMU. >> > >> > Wait, you want to pass a device through to userspace but you don't want to >> > use the IOMMU? Why not? >> > >> > If you describe the SMMU in firmware with only a partial topology >> > description, then you will run into problems with unknown masters trying to >> > perform DMA. That's the IOMMU doing its job! >> >> We are keeping disable_bypass = false. In other words, we >> are using bypass mode for unmatched streams. The real >> reason is limited number of SMRs due to which we choose >> not to provide "iommus" DT attribute for certain devices. > > Understood, but that's not robust for SMMUv3 and we *really* shouldn't have > a user ABI that changes behaviour based on a cmdline option. VFIO should be > requesting its own identity mappings, if that's what you need. Currently, the iommu_present() check in vfio_iommu_group_get() is preventing us allow no-IOMMU mode for certain devices. Is there a better replacement of iommu_present() check in vfio_iommu_group_get()? Regards, Anup
On Wed, Jul 19, 2017 at 05:09:05PM +0530, Anup Patel wrote: > On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <will.deacon@arm.com> wrote: > > On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote: > >> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <will.deacon@arm.com> wrote: > >> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: > >> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: > >> >> > On 19/07/17 10:33, Anup Patel wrote: > >> >> >> The ARM SMMUv3 support bypassing transactions for which domain > >> >> >> is not configured. The patch adds corresponding IOMMU capability > >> >> >> to advertise this fact. > >> >> >> > >> >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> > >> >> >> --- > >> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++ > >> >> >> 1 file changed, 2 insertions(+) > >> >> >> > >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >> >> >> index 568c400..a6c7f66 100644 > >> >> >> --- a/drivers/iommu/arm-smmu-v3.c > >> >> >> +++ b/drivers/iommu/arm-smmu-v3.c > >> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) > >> >> >> return true; > >> >> >> case IOMMU_CAP_NOEXEC: > >> >> >> return true; > >> >> >> + case IOMMU_CAP_BYPASS: > >> >> >> + return true; > >> >> > > >> >> > And this is never true. If Linux knows a device masters through the > >> >> > SMMU, it will always have a default domain of some sort (either identity > >> >> > or DMA ops). If Linux doesn't know, then it won't have been able to > >> >> > initialise the stream table for the relevant stream IDs, thus any > >> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only > >> >> > bypass unknown stream IDs if disabled entirely. > >> >> > >> >> What if we don't want to use IOMMU for certain device and > >> >> due to this we never provide "iommus" DT attribute in the > >> >> device DT node. Further, we want to access device without > >> >> "iommus" DT attribute from user-space using VFIO no-IOMMU. > >> > > >> > Wait, you want to pass a device through to userspace but you don't want to > >> > use the IOMMU? Why not? > >> > > >> > If you describe the SMMU in firmware with only a partial topology > >> > description, then you will run into problems with unknown masters trying to > >> > perform DMA. That's the IOMMU doing its job! > >> > >> We are keeping disable_bypass = false. In other words, we > >> are using bypass mode for unmatched streams. The real > >> reason is limited number of SMRs due to which we choose > >> not to provide "iommus" DT attribute for certain devices. > > > > Understood, but that's not robust for SMMUv3 and we *really* shouldn't have > > a user ABI that changes behaviour based on a cmdline option. VFIO should be > > requesting its own identity mappings, if that's what you need. > > Currently, the iommu_present() check in vfio_iommu_group_get() > is preventing us allow no-IOMMU mode for certain devices. > > Is there a better replacement of iommu_present() check in > vfio_iommu_group_get()? There are two things here: 1. iommu_present() is pretty useless, because it applies to a "bus" which doesn't actually tell you what you need to know for things like the platform_bus, where some masters might be upstream of an SMMU and others might not be. 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, then the VFIO no-IOMMU code needs to be extended so that it creates an IDENTITY domain on that IOMMU. I think you need to solve both of those to make this robust. That way, VFIO can accurately tell whether a given master has an IOMMU or not and then figure out whether or not it needs to create an IDENTITY domain, or whether it can do like it does at the moment and ignore the IOMMU entirely (for the no-IOMMU case). Will
On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jul 19, 2017 at 05:09:05PM +0530, Anup Patel wrote: >> On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <will.deacon@arm.com> wrote: >> > On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote: >> >> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <will.deacon@arm.com> wrote: >> >> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote: >> >> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> >> >> > On 19/07/17 10:33, Anup Patel wrote: >> >> >> >> The ARM SMMUv3 support bypassing transactions for which domain >> >> >> >> is not configured. The patch adds corresponding IOMMU capability >> >> >> >> to advertise this fact. >> >> >> >> >> >> >> >> Signed-off-by: Anup Patel <anup.patel@broadcom.com> >> >> >> >> --- >> >> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++ >> >> >> >> 1 file changed, 2 insertions(+) >> >> >> >> >> >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> >> >> >> index 568c400..a6c7f66 100644 >> >> >> >> --- a/drivers/iommu/arm-smmu-v3.c >> >> >> >> +++ b/drivers/iommu/arm-smmu-v3.c >> >> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) >> >> >> >> return true; >> >> >> >> case IOMMU_CAP_NOEXEC: >> >> >> >> return true; >> >> >> >> + case IOMMU_CAP_BYPASS: >> >> >> >> + return true; >> >> >> > >> >> >> > And this is never true. If Linux knows a device masters through the >> >> >> > SMMU, it will always have a default domain of some sort (either identity >> >> >> > or DMA ops). If Linux doesn't know, then it won't have been able to >> >> >> > initialise the stream table for the relevant stream IDs, thus any >> >> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only >> >> >> > bypass unknown stream IDs if disabled entirely. >> >> >> >> >> >> What if we don't want to use IOMMU for certain device and >> >> >> due to this we never provide "iommus" DT attribute in the >> >> >> device DT node. Further, we want to access device without >> >> >> "iommus" DT attribute from user-space using VFIO no-IOMMU. >> >> > >> >> > Wait, you want to pass a device through to userspace but you don't want to >> >> > use the IOMMU? Why not? >> >> > >> >> > If you describe the SMMU in firmware with only a partial topology >> >> > description, then you will run into problems with unknown masters trying to >> >> > perform DMA. That's the IOMMU doing its job! >> >> >> >> We are keeping disable_bypass = false. In other words, we >> >> are using bypass mode for unmatched streams. The real >> >> reason is limited number of SMRs due to which we choose >> >> not to provide "iommus" DT attribute for certain devices. >> > >> > Understood, but that's not robust for SMMUv3 and we *really* shouldn't have >> > a user ABI that changes behaviour based on a cmdline option. VFIO should be >> > requesting its own identity mappings, if that's what you need. >> >> Currently, the iommu_present() check in vfio_iommu_group_get() >> is preventing us allow no-IOMMU mode for certain devices. >> >> Is there a better replacement of iommu_present() check in >> vfio_iommu_group_get()? > > There are two things here: > > 1. iommu_present() is pretty useless, because it applies to a "bus" which > doesn't actually tell you what you need to know for things like the > platform_bus, where some masters might be upstream of an SMMU and > others might not be. I agree with you. The iommu_present() check in vfio_iommu_group_get() is not much useful. We only reach line which checks iommu_present() when iommu_group_get() returns NULL for given "struct device *". If there is no IOMMU group for a "struct device *" then it means there is no IOMMU HW doing translations for such device. If we drop the iommu_present() check (due to above reasons) in vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS and we can happily drop PATCH1, PATCH2, and PATCH3. I will remove the iommu_present() check in vfio_iommu_group_get() because it is only comes into actions when VFIO_NOIOMMU is enabled. This will also help us drop PATCH1-to-PATCH3. > > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, > then the VFIO no-IOMMU code needs to be extended so that it creates > an IDENTITY domain on that IOMMU. The VFIO no-IOMMU mode is equivalent to Linux UIO hence having IDENTITY domain for VFIO no-IOMMU is not appropriate here. In fact, going forward it has been suggested to use VFIO no-IOMMU instead of Linux UIO for user-space poll-mode drivers so that we have one Linux interface for user-space poll-mode drivers. Regards, Anup
On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: > On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: > > There are two things here: > > > > 1. iommu_present() is pretty useless, because it applies to a "bus" which > > doesn't actually tell you what you need to know for things like the > > platform_bus, where some masters might be upstream of an SMMU and > > others might not be. > > I agree with you. The iommu_present() check in vfio_iommu_group_get() > is not much useful. We only reach line which checks iommu_present() > when iommu_group_get() returns NULL for given "struct device *". If there > is no IOMMU group for a "struct device *" then it means there is no IOMMU > HW doing translations for such device. > > If we drop the iommu_present() check (due to above reasons) in > vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS > and we can happily drop PATCH1, PATCH2, and PATCH3. > > I will remove the iommu_present() check in vfio_iommu_group_get() > because it is only comes into actions when VFIO_NOIOMMU is > enabled. This will also help us drop PATCH1-to-PATCH3. I don't think that's the right answer. Whilst iommu_present has obvious shortcomings, its intention is clear: it should tell you whether a given *device* is upstream of an IOMMU. So the right fix is to make this per-device, instead of per-bus. Removing it altogether is worse than leaving it like it is. > > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, > > then the VFIO no-IOMMU code needs to be extended so that it creates > > an IDENTITY domain on that IOMMU. > > The VFIO no-IOMMU mode is equivalent to Linux UIO hence having > IDENTITY domain for VFIO no-IOMMU is not appropriate here. Can you elaborate on this please? I don't understand the argument you're making. It's like saying "I don't like eggs, therefore I don't drive a car". Will
On Thu, Jul 20, 2017 at 2:40 PM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: >> > There are two things here: >> > >> > 1. iommu_present() is pretty useless, because it applies to a "bus" which >> > doesn't actually tell you what you need to know for things like the >> > platform_bus, where some masters might be upstream of an SMMU and >> > others might not be. >> >> I agree with you. The iommu_present() check in vfio_iommu_group_get() >> is not much useful. We only reach line which checks iommu_present() >> when iommu_group_get() returns NULL for given "struct device *". If there >> is no IOMMU group for a "struct device *" then it means there is no IOMMU >> HW doing translations for such device. >> >> If we drop the iommu_present() check (due to above reasons) in >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS >> and we can happily drop PATCH1, PATCH2, and PATCH3. >> >> I will remove the iommu_present() check in vfio_iommu_group_get() >> because it is only comes into actions when VFIO_NOIOMMU is >> enabled. This will also help us drop PATCH1-to-PATCH3. > > I don't think that's the right answer. Whilst iommu_present has obvious > shortcomings, its intention is clear: it should tell you whether a given > *device* is upstream of an IOMMU. So the right fix is to make this > per-device, instead of per-bus. Removing it altogether is worse than leaving > it like it is. > >> > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, >> > then the VFIO no-IOMMU code needs to be extended so that it creates >> > an IDENTITY domain on that IOMMU. >> >> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having >> IDENTITY domain for VFIO no-IOMMU is not appropriate here. > > Can you elaborate on this please? I don't understand the argument you're > making. It's like saying "I don't like eggs, therefore I don't drive a > car". > Like I said, VFIO no-IOMMU mode for a device means device transactions will not go through any IOMMU. That's why having IDENTITY domain for device using VFIO no-IOMMU is not semantically correct. The analogy you proposed does not apply here. Anyways, this patch has nothing to do with FlexRM support for VFIO platform hence I will drop it. Fixing VFIO no-IOMMU mode can be a separate patchset. Regards, Anup
On Thu, Jul 20, 2017 at 04:38:09PM +0530, Anup Patel wrote: > On Thu, Jul 20, 2017 at 2:40 PM, Will Deacon <will.deacon@arm.com> wrote: > > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: > >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: > >> > There are two things here: > >> > > >> > 1. iommu_present() is pretty useless, because it applies to a "bus" which > >> > doesn't actually tell you what you need to know for things like the > >> > platform_bus, where some masters might be upstream of an SMMU and > >> > others might not be. > >> > >> I agree with you. The iommu_present() check in vfio_iommu_group_get() > >> is not much useful. We only reach line which checks iommu_present() > >> when iommu_group_get() returns NULL for given "struct device *". If there > >> is no IOMMU group for a "struct device *" then it means there is no IOMMU > >> HW doing translations for such device. > >> > >> If we drop the iommu_present() check (due to above reasons) in > >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS > >> and we can happily drop PATCH1, PATCH2, and PATCH3. > >> > >> I will remove the iommu_present() check in vfio_iommu_group_get() > >> because it is only comes into actions when VFIO_NOIOMMU is > >> enabled. This will also help us drop PATCH1-to-PATCH3. > > > > I don't think that's the right answer. Whilst iommu_present has obvious > > shortcomings, its intention is clear: it should tell you whether a given > > *device* is upstream of an IOMMU. So the right fix is to make this > > per-device, instead of per-bus. Removing it altogether is worse than leaving > > it like it is. > > > >> > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, > >> > then the VFIO no-IOMMU code needs to be extended so that it creates > >> > an IDENTITY domain on that IOMMU. > >> > >> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having > >> IDENTITY domain for VFIO no-IOMMU is not appropriate here. > > > > Can you elaborate on this please? I don't understand the argument you're > > making. It's like saying "I don't like eggs, therefore I don't drive a > > car". > > > > Like I said, VFIO no-IOMMU mode for a device means device transactions > will not go through any IOMMU. That's why having IDENTITY domain for > device using VFIO no-IOMMU is not semantically correct. The analogy you > proposed does not apply here. If you have a master that is wired through an IOMMU, then its transactions go through that IOMMU and you can't avoid that. What you want is a way for the IOMMU driver to make the IOMMU appear transparent to the device. That is achieved by creating an IDENTITY domain, which sounds perfect for what you're trying to do. Will
On 20/07/17 10:10, Will Deacon wrote: > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: >>> There are two things here: >>> >>> 1. iommu_present() is pretty useless, because it applies to a "bus" which >>> doesn't actually tell you what you need to know for things like the >>> platform_bus, where some masters might be upstream of an SMMU and >>> others might not be. >> >> I agree with you. The iommu_present() check in vfio_iommu_group_get() >> is not much useful. We only reach line which checks iommu_present() >> when iommu_group_get() returns NULL for given "struct device *". If there >> is no IOMMU group for a "struct device *" then it means there is no IOMMU >> HW doing translations for such device. >> >> If we drop the iommu_present() check (due to above reasons) in >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS >> and we can happily drop PATCH1, PATCH2, and PATCH3. >> >> I will remove the iommu_present() check in vfio_iommu_group_get() >> because it is only comes into actions when VFIO_NOIOMMU is >> enabled. This will also help us drop PATCH1-to-PATCH3. > > I don't think that's the right answer. Whilst iommu_present has obvious > shortcomings, its intention is clear: it should tell you whether a given > *device* is upstream of an IOMMU. So the right fix is to make this > per-device, instead of per-bus. Removing it altogether is worse than leaving > it like it is. Not really - if there is an IOMMU up and running to the point of setting bus ops, every device it cares about can be expected to have a group already (there are only a couple of drivers left that don't use groups, and they're hardly relevant to VFIO). Thus iommu_group_get() already is the de-facto per-device IOMMU check. And having looked into it, I'm now spinning a couple of patches to finish off making groups truly mandatory so that that can be less de-facto ;) Robin. >>> 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU, >>> then the VFIO no-IOMMU code needs to be extended so that it creates >>> an IDENTITY domain on that IOMMU. >> >> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having >> IDENTITY domain for VFIO no-IOMMU is not appropriate here. > > Can you elaborate on this please? I don't understand the argument you're > making. It's like saying "I don't like eggs, therefore I don't drive a > car". > > Will >
On Thu, 20 Jul 2017 12:17:12 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > On 20/07/17 10:10, Will Deacon wrote: > > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: > >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: > >>> There are two things here: > >>> > >>> 1. iommu_present() is pretty useless, because it applies to a "bus" which > >>> doesn't actually tell you what you need to know for things like the > >>> platform_bus, where some masters might be upstream of an SMMU and > >>> others might not be. > >> > >> I agree with you. The iommu_present() check in vfio_iommu_group_get() > >> is not much useful. We only reach line which checks iommu_present() > >> when iommu_group_get() returns NULL for given "struct device *". If there > >> is no IOMMU group for a "struct device *" then it means there is no IOMMU > >> HW doing translations for such device. > >> > >> If we drop the iommu_present() check (due to above reasons) in > >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS > >> and we can happily drop PATCH1, PATCH2, and PATCH3. > >> > >> I will remove the iommu_present() check in vfio_iommu_group_get() > >> because it is only comes into actions when VFIO_NOIOMMU is > >> enabled. This will also help us drop PATCH1-to-PATCH3. > > > > I don't think that's the right answer. Whilst iommu_present has obvious > > shortcomings, its intention is clear: it should tell you whether a given > > *device* is upstream of an IOMMU. So the right fix is to make this > > per-device, instead of per-bus. Removing it altogether is worse than leaving > > it like it is. > > Not really - if there is an IOMMU up and running to the point of setting > bus ops, every device it cares about can be expected to have a group > already (there are only a couple of drivers left that don't use groups, > and they're hardly relevant to VFIO). Thus iommu_group_get() already is > the de-facto per-device IOMMU check. > > And having looked into it, I'm now spinning a couple of patches to > finish off making groups truly mandatory so that that can be less > de-facto ;) No, look at vfio-noiommu and even vfio-mdev devices for devices which have an iommu group but there is no physical iommu supporting them. iommu_present() is how we can distinguish these groups and therefore not generate a segfault in trying to use the full IOMMU API on them. Thanks, Alex
On 24/07/17 18:16, Alex Williamson wrote: > On Thu, 20 Jul 2017 12:17:12 +0100 > Robin Murphy <robin.murphy@arm.com> wrote: > >> On 20/07/17 10:10, Will Deacon wrote: >>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: >>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: >>>>> There are two things here: >>>>> >>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which >>>>> doesn't actually tell you what you need to know for things like the >>>>> platform_bus, where some masters might be upstream of an SMMU and >>>>> others might not be. >>>> >>>> I agree with you. The iommu_present() check in vfio_iommu_group_get() >>>> is not much useful. We only reach line which checks iommu_present() >>>> when iommu_group_get() returns NULL for given "struct device *". If there >>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU >>>> HW doing translations for such device. >>>> >>>> If we drop the iommu_present() check (due to above reasons) in >>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS >>>> and we can happily drop PATCH1, PATCH2, and PATCH3. >>>> >>>> I will remove the iommu_present() check in vfio_iommu_group_get() >>>> because it is only comes into actions when VFIO_NOIOMMU is >>>> enabled. This will also help us drop PATCH1-to-PATCH3. >>> >>> I don't think that's the right answer. Whilst iommu_present has obvious >>> shortcomings, its intention is clear: it should tell you whether a given >>> *device* is upstream of an IOMMU. So the right fix is to make this >>> per-device, instead of per-bus. Removing it altogether is worse than leaving >>> it like it is. >> >> Not really - if there is an IOMMU up and running to the point of setting >> bus ops, every device it cares about can be expected to have a group >> already (there are only a couple of drivers left that don't use groups, >> and they're hardly relevant to VFIO). Thus iommu_group_get() already is >> the de-facto per-device IOMMU check. >> >> And having looked into it, I'm now spinning a couple of patches to >> finish off making groups truly mandatory so that that can be less >> de-facto ;) > > No, look at vfio-noiommu and even vfio-mdev devices for devices which > have an iommu group but there is no physical iommu supporting them. > iommu_present() is how we can distinguish these groups and therefore > not generate a segfault in trying to use the full IOMMU API on them. OK, so that means that the combination of vfio-noiommu and vfio-platform is simply unusable, because iommu_present(&platform_bus_type) can give such dangerous false positives too. Robin.
On Mon, 24 Jul 2017 18:23:20 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > On 24/07/17 18:16, Alex Williamson wrote: > > On Thu, 20 Jul 2017 12:17:12 +0100 > > Robin Murphy <robin.murphy@arm.com> wrote: > > > >> On 20/07/17 10:10, Will Deacon wrote: > >>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: > >>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: > >>>>> There are two things here: > >>>>> > >>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which > >>>>> doesn't actually tell you what you need to know for things like the > >>>>> platform_bus, where some masters might be upstream of an SMMU and > >>>>> others might not be. > >>>> > >>>> I agree with you. The iommu_present() check in vfio_iommu_group_get() > >>>> is not much useful. We only reach line which checks iommu_present() > >>>> when iommu_group_get() returns NULL for given "struct device *". If there > >>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU > >>>> HW doing translations for such device. > >>>> > >>>> If we drop the iommu_present() check (due to above reasons) in > >>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS > >>>> and we can happily drop PATCH1, PATCH2, and PATCH3. > >>>> > >>>> I will remove the iommu_present() check in vfio_iommu_group_get() > >>>> because it is only comes into actions when VFIO_NOIOMMU is > >>>> enabled. This will also help us drop PATCH1-to-PATCH3. > >>> > >>> I don't think that's the right answer. Whilst iommu_present has obvious > >>> shortcomings, its intention is clear: it should tell you whether a given > >>> *device* is upstream of an IOMMU. So the right fix is to make this > >>> per-device, instead of per-bus. Removing it altogether is worse than leaving > >>> it like it is. > >> > >> Not really - if there is an IOMMU up and running to the point of setting > >> bus ops, every device it cares about can be expected to have a group > >> already (there are only a couple of drivers left that don't use groups, > >> and they're hardly relevant to VFIO). Thus iommu_group_get() already is > >> the de-facto per-device IOMMU check. > >> > >> And having looked into it, I'm now spinning a couple of patches to > >> finish off making groups truly mandatory so that that can be less > >> de-facto ;) > > > > No, look at vfio-noiommu and even vfio-mdev devices for devices which > > have an iommu group but there is no physical iommu supporting them. > > iommu_present() is how we can distinguish these groups and therefore > > not generate a segfault in trying to use the full IOMMU API on them. > > OK, so that means that the combination of vfio-noiommu and vfio-platform > is simply unusable, because iommu_present(&platform_bus_type) can give > such dangerous false positives too. Yep, this kinda falls apart since platform_bus_type doesn't really map to a physical bus, nor does the presence of a group canonically demonstrate that an iommu is present since anyone can create a group for a device. We really do need to migrate to per-device iommu_ops. Thanks, Alex
On Tue, Jul 25, 2017 at 12:36 AM, Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 24 Jul 2017 18:23:20 +0100 > Robin Murphy <robin.murphy@arm.com> wrote: > >> On 24/07/17 18:16, Alex Williamson wrote: >> > On Thu, 20 Jul 2017 12:17:12 +0100 >> > Robin Murphy <robin.murphy@arm.com> wrote: >> > >> >> On 20/07/17 10:10, Will Deacon wrote: >> >>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote: >> >>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <will.deacon@arm.com> wrote: >> >>>>> There are two things here: >> >>>>> >> >>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which >> >>>>> doesn't actually tell you what you need to know for things like the >> >>>>> platform_bus, where some masters might be upstream of an SMMU and >> >>>>> others might not be. >> >>>> >> >>>> I agree with you. The iommu_present() check in vfio_iommu_group_get() >> >>>> is not much useful. We only reach line which checks iommu_present() >> >>>> when iommu_group_get() returns NULL for given "struct device *". If there >> >>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU >> >>>> HW doing translations for such device. >> >>>> >> >>>> If we drop the iommu_present() check (due to above reasons) in >> >>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS >> >>>> and we can happily drop PATCH1, PATCH2, and PATCH3. >> >>>> >> >>>> I will remove the iommu_present() check in vfio_iommu_group_get() >> >>>> because it is only comes into actions when VFIO_NOIOMMU is >> >>>> enabled. This will also help us drop PATCH1-to-PATCH3. >> >>> >> >>> I don't think that's the right answer. Whilst iommu_present has obvious >> >>> shortcomings, its intention is clear: it should tell you whether a given >> >>> *device* is upstream of an IOMMU. So the right fix is to make this >> >>> per-device, instead of per-bus. Removing it altogether is worse than leaving >> >>> it like it is. >> >> >> >> Not really - if there is an IOMMU up and running to the point of setting >> >> bus ops, every device it cares about can be expected to have a group >> >> already (there are only a couple of drivers left that don't use groups, >> >> and they're hardly relevant to VFIO). Thus iommu_group_get() already is >> >> the de-facto per-device IOMMU check. >> >> >> >> And having looked into it, I'm now spinning a couple of patches to >> >> finish off making groups truly mandatory so that that can be less >> >> de-facto ;) >> > >> > No, look at vfio-noiommu and even vfio-mdev devices for devices which >> > have an iommu group but there is no physical iommu supporting them. >> > iommu_present() is how we can distinguish these groups and therefore >> > not generate a segfault in trying to use the full IOMMU API on them. >> >> OK, so that means that the combination of vfio-noiommu and vfio-platform >> is simply unusable, because iommu_present(&platform_bus_type) can give >> such dangerous false positives too. > > Yep, this kinda falls apart since platform_bus_type doesn't really map > to a physical bus, nor does the presence of a group canonically > demonstrate that an iommu is present since anyone can create a group > for a device. We really do need to migrate to per-device iommu_ops. > Thanks, Yes, per-device iommu_ops will make things much cleaner. That's why I have dropped VFIO no-IOMMU and IOMMU_CAP_BYPASS related patches. Can you please have a look at FlexRM platform reset driver? Regards, Anup
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 568c400..a6c7f66 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) return true; case IOMMU_CAP_NOEXEC: return true; + case IOMMU_CAP_BYPASS: + return true; default: return false; }
The ARM SMMUv3 support bypassing transactions for which domain is not configured. The patch adds corresponding IOMMU capability to advertise this fact. Signed-off-by: Anup Patel <anup.patel@broadcom.com> --- drivers/iommu/arm-smmu-v3.c | 2 ++ 1 file changed, 2 insertions(+)