This patch is added as instead of returning a boolean for DOMAIN_ATTR_NESTING,
iommu_domain_get_attr() should return an iommu_nesting_info handle.
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Eric Auger <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>
Suggested-by: Jean-Philippe Brucker <[email protected]>
Signed-off-by: Liu Yi L <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
v4 -> v5:
*) address comments from Eric Auger.
---
drivers/iommu/arm-smmu-v3.c | 29 +++++++++++++++++++++++++++--
drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++--
2 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677..ec815d7 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -3019,6 +3019,32 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
return group;
}
+static int arm_smmu_domain_nesting_info(struct arm_smmu_domain *smmu_domain,
+ void *data)
+{
+ struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
+ unsigned int size;
+
+ if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
+ return -ENODEV;
+
+ size = sizeof(struct iommu_nesting_info);
+
+ /*
+ * if provided buffer size is smaller than expected, should
+ * return 0 and also the expected buffer size to caller.
+ */
+ if (info->size < size) {
+ info->size = size;
+ return 0;
+ }
+
+ /* report an empty iommu_nesting_info for now */
+ memset(info, 0x0, size);
+ info->size = size;
+ return 0;
+}
+
static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
enum iommu_attr attr, void *data)
{
@@ -3028,8 +3054,7 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
case IOMMU_DOMAIN_UNMANAGED:
switch (attr) {
case DOMAIN_ATTR_NESTING:
- *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
- return 0;
+ return arm_smmu_domain_nesting_info(smmu_domain, data);
default:
return -ENODEV;
}
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4c..09e2f1b 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1506,6 +1506,32 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
return group;
}
+static int arm_smmu_domain_nesting_info(struct arm_smmu_domain *smmu_domain,
+ void *data)
+{
+ struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
+ unsigned int size;
+
+ if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
+ return -ENODEV;
+
+ size = sizeof(struct iommu_nesting_info);
+
+ /*
+ * if provided buffer size is smaller than expected, should
+ * return 0 and also the expected buffer size to caller.
+ */
+ if (info->size < size) {
+ info->size = size;
+ return 0;
+ }
+
+ /* report an empty iommu_nesting_info for now */
+ memset(info, 0x0, size);
+ info->size = size;
+ return 0;
+}
+
static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
enum iommu_attr attr, void *data)
{
@@ -1515,8 +1541,7 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
case IOMMU_DOMAIN_UNMANAGED:
switch (attr) {
case DOMAIN_ATTR_NESTING:
- *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
- return 0;
+ return arm_smmu_domain_nesting_info(smmu_domain, data);
default:
return -ENODEV;
}
--
2.7.4
On Sun, Jul 12, 2020 at 04:20:58AM -0700, Liu Yi L wrote:
> This patch is added as instead of returning a boolean for DOMAIN_ATTR_NESTING,
> iommu_domain_get_attr() should return an iommu_nesting_info handle.
>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Eric Auger <[email protected]>
> Cc: Jean-Philippe Brucker <[email protected]>
> Suggested-by: Jean-Philippe Brucker <[email protected]>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> v4 -> v5:
> *) address comments from Eric Auger.
> ---
> drivers/iommu/arm-smmu-v3.c | 29 +++++++++++++++++++++++++++--
> drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++--
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index f578677..ec815d7 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -3019,6 +3019,32 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> return group;
> }
>
> +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain *smmu_domain,
> + void *data)
> +{
> + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> + unsigned int size;
> +
> + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> + return -ENODEV;
> +
> + size = sizeof(struct iommu_nesting_info);
> +
> + /*
> + * if provided buffer size is smaller than expected, should
> + * return 0 and also the expected buffer size to caller.
> + */
> + if (info->size < size) {
> + info->size = size;
> + return 0;
> + }
> +
> + /* report an empty iommu_nesting_info for now */
> + memset(info, 0x0, size);
> + info->size = size;
> + return 0;
> +}
Have you verified that this doesn't break the existing usage of
DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
Will
Hi Will,
> From: Will Deacon <[email protected]>
> Sent: Monday, July 13, 2020 9:15 PM
>
> On Sun, Jul 12, 2020 at 04:20:58AM -0700, Liu Yi L wrote:
> > This patch is added as instead of returning a boolean for DOMAIN_ATTR_NESTING,
> > iommu_domain_get_attr() should return an iommu_nesting_info handle.
> >
> > Cc: Will Deacon <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Eric Auger <[email protected]>
> > Cc: Jean-Philippe Brucker <[email protected]>
> > Suggested-by: Jean-Philippe Brucker <[email protected]>
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > v4 -> v5:
> > *) address comments from Eric Auger.
> > ---
> > drivers/iommu/arm-smmu-v3.c | 29 +++++++++++++++++++++++++++--
> > drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++--
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index f578677..ec815d7 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -3019,6 +3019,32 @@ static struct iommu_group
> *arm_smmu_device_group(struct device *dev)
> > return group;
> > }
> >
> > +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain
> *smmu_domain,
> > + void *data)
> > +{
> > + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> > + unsigned int size;
> > +
> > + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> > + return -ENODEV;
> > +
> > + size = sizeof(struct iommu_nesting_info);
> > +
> > + /*
> > + * if provided buffer size is smaller than expected, should
> > + * return 0 and also the expected buffer size to caller.
> > + */
> > + if (info->size < size) {
> > + info->size = size;
> > + return 0;
> > + }
> > +
> > + /* report an empty iommu_nesting_info for now */
> > + memset(info, 0x0, size);
> > + info->size = size;
> > + return 0;
> > +}
>
> Have you verified that this doesn't break the existing usage of
> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
I didn't have ARM machine on my hand. But I contacted with Jean
Philippe, he confirmed no compiling issue. I didn't see any code
getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
and won't fail if the iommu_domai_get_attr() returns 0. This patch
returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
value is 0 if no error. So I guess it won't fail nesting for ARM.
@Eric, how about your opinion? your dual-stage vSMMU support may
also share the vfio_iommu_type1.c code.
Regards,
Yi Liu
> Will
On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
> > Have you verified that this doesn't break the existing usage of
> > DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
>
> I didn't have ARM machine on my hand. But I contacted with Jean
> Philippe, he confirmed no compiling issue. I didn't see any code
> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> and won't fail if the iommu_domai_get_attr() returns 0. This patch
> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> value is 0 if no error. So I guess it won't fail nesting for ARM.
I confirm that this series doesn't break the current support for
VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
If the SMMU does not support stage-2 then there is a change in behavior
(untested): after the domain is silently switched to stage-1 by the SMMU
driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
succeding as before, the VFIO ioctl will now fail. I believe that's a fix
rather than a regression, it should have been like this since the
beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
far, so I don't think it should be a concern.
And if userspace queries the nesting properties using the new ABI
introduced in this patchset, it will obtain an empty struct. I think
that's acceptable, but it may be better to avoid adding the nesting cap if
@format is 0?
Thanks,
Jean
>
> @Eric, how about your opinion? your dual-stage vSMMU support may
> also share the vfio_iommu_type1.c code.
>
> Regards,
> Yi Liu
>
> > Will
Hi Jean,
On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
> On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
>>> Have you verified that this doesn't break the existing usage of
>>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
>>
>> I didn't have ARM machine on my hand. But I contacted with Jean
>> Philippe, he confirmed no compiling issue. I didn't see any code
>> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
>> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
>> and won't fail if the iommu_domai_get_attr() returns 0. This patch
>> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
>> value is 0 if no error. So I guess it won't fail nesting for ARM.
>
> I confirm that this series doesn't break the current support for
> VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
>
> If the SMMU does not support stage-2 then there is a change in behavior
> (untested): after the domain is silently switched to stage-1 by the SMMU
> driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> rather than a regression, it should have been like this since the
> beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
> far, so I don't think it should be a concern.
But as Yi mentioned ealier, in the current vfio code there is no
DOMAIN_ATTR_NESTING query yet. In my SMMUV3 nested stage series, I added
such a query in vfio-pci.c to detect if I need to expose a fault region
but I already test both the returned value and the output arg. So to me
there is no issue with that change.
>
> And if userspace queries the nesting properties using the new ABI
> introduced in this patchset, it will obtain an empty struct. I think
> that's acceptable, but it may be better to avoid adding the nesting cap if
> @format is 0?
agreed
Thanks
Eric
>
> Thanks,
> Jean
>
>>
>> @Eric, how about your opinion? your dual-stage vSMMU support may
>> also share the vfio_iommu_type1.c code.
>>
>> Regards,
>> Yi Liu
>>
>>> Will
>
On Thu, Jul 16, 2020 at 10:38:17PM +0200, Auger Eric wrote:
> Hi Jean,
>
> On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
> > On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
> >>> Have you verified that this doesn't break the existing usage of
> >>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
> >>
> >> I didn't have ARM machine on my hand. But I contacted with Jean
> >> Philippe, he confirmed no compiling issue. I didn't see any code
> >> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
> >> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> >> and won't fail if the iommu_domai_get_attr() returns 0. This patch
> >> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> >> value is 0 if no error. So I guess it won't fail nesting for ARM.
> >
> > I confirm that this series doesn't break the current support for
> > VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
> >
> > If the SMMU does not support stage-2 then there is a change in behavior
> > (untested): after the domain is silently switched to stage-1 by the SMMU
> > driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> > succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> > rather than a regression, it should have been like this since the
> > beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
> > far, so I don't think it should be a concern.
> But as Yi mentioned ealier, in the current vfio code there is no
> DOMAIN_ATTR_NESTING query yet.
That's why something that would have succeeded before will now fail:
Before this series, if user asked for a VFIO_IOMMU_TYPE1_NESTING, it would
have succeeded even if the SMMU didn't support stage-2, as the driver
would have silently fallen back on stage-1 mappings (which work exactly
the same as stage-2-only since there was no nesting supported). After the
series, we do check for DOMAIN_ATTR_NESTING so if user asks for
VFIO_IOMMU_TYPE1_NESTING and the SMMU doesn't support stage-2, the ioctl
fails.
I believe it's a good fix and completely harmless, but wanted to make sure
no one objects because it's an ABI change.
Thanks,
Jean
> In my SMMUV3 nested stage series, I added
> such a query in vfio-pci.c to detect if I need to expose a fault region
> but I already test both the returned value and the output arg. So to me
> there is no issue with that change.
> >
> > And if userspace queries the nesting properties using the new ABI
> > introduced in this patchset, it will obtain an empty struct. I think
> > that's acceptable, but it may be better to avoid adding the nesting cap if
> > @format is 0?
> agreed
>
> Thanks
>
> Eric
> >
> > Thanks,
> > Jean
> >
> >>
> >> @Eric, how about your opinion? your dual-stage vSMMU support may
> >> also share the vfio_iommu_type1.c code.
> >>
> >> Regards,
> >> Yi Liu
> >>
> >>> Will
> >
>
Hi Jean,
On 7/17/20 11:09 AM, Jean-Philippe Brucker wrote:
> On Thu, Jul 16, 2020 at 10:38:17PM +0200, Auger Eric wrote:
>> Hi Jean,
>>
>> On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
>>> On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
>>>>> Have you verified that this doesn't break the existing usage of
>>>>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
>>>>
>>>> I didn't have ARM machine on my hand. But I contacted with Jean
>>>> Philippe, he confirmed no compiling issue. I didn't see any code
>>>> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
>>>> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
>>>> and won't fail if the iommu_domai_get_attr() returns 0. This patch
>>>> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
>>>> value is 0 if no error. So I guess it won't fail nesting for ARM.
>>>
>>> I confirm that this series doesn't break the current support for
>>> VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
>>>
>>> If the SMMU does not support stage-2 then there is a change in behavior
>>> (untested): after the domain is silently switched to stage-1 by the SMMU
>>> driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
>>> succeding as before, the VFIO ioctl will now fail. I believe that's a fix
>>> rather than a regression, it should have been like this since the
>>> beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
>>> far, so I don't think it should be a concern.
>> But as Yi mentioned ealier, in the current vfio code there is no
>> DOMAIN_ATTR_NESTING query yet.
>
> That's why something that would have succeeded before will now fail:
> Before this series, if user asked for a VFIO_IOMMU_TYPE1_NESTING, it would
> have succeeded even if the SMMU didn't support stage-2, as the driver
> would have silently fallen back on stage-1 mappings (which work exactly
> the same as stage-2-only since there was no nesting supported). After the
> series, we do check for DOMAIN_ATTR_NESTING so if user asks for
> VFIO_IOMMU_TYPE1_NESTING and the SMMU doesn't support stage-2, the ioctl
> fails.
OK I now understand what you meant. Yes this actual change is brought by
the next patch, ie.
[PATCH v5 04/15] vfio/type1: Report iommu nesting info to userspace
Thanks
Eric
>
> I believe it's a good fix and completely harmless, but wanted to make sure
> no one objects because it's an ABI change.
>
> Thanks,
> Jean
>
>> In my SMMUV3 nested stage series, I added
>> such a query in vfio-pci.c to detect if I need to expose a fault region
>> but I already test both the returned value and the output arg. So to me
>> there is no issue with that change.
>>>
>>> And if userspace queries the nesting properties using the new ABI
>>> introduced in this patchset, it will obtain an empty struct. I think
>>> that's acceptable, but it may be better to avoid adding the nesting cap if
>>> @format is 0?
>> agreed
>>
>> Thanks
>>
>> Eric
>>>
>>> Thanks,
>>> Jean
>>>
>>>>
>>>> @Eric, how about your opinion? your dual-stage vSMMU support may
>>>> also share the vfio_iommu_type1.c code.
>>>>
>>>> Regards,
>>>> Yi Liu
>>>>
>>>>> Will
>>>
>>
>
Yi,
On 7/12/20 1:20 PM, Liu Yi L wrote:
> This patch is added as instead of returning a boolean for DOMAIN_ATTR_NESTING,
> iommu_domain_get_attr() should return an iommu_nesting_info handle.
you may add in the commit message you return an empty nesting info
struct for now as true nesting is not yet supported by the SMMUs.
Besides:
Reviewed-by: Eric Auger <[email protected]>
Thanks
Eric
>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Eric Auger <[email protected]>
> Cc: Jean-Philippe Brucker <[email protected]>
> Suggested-by: Jean-Philippe Brucker <[email protected]>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> v4 -> v5:
> *) address comments from Eric Auger.
> ---
> drivers/iommu/arm-smmu-v3.c | 29 +++++++++++++++++++++++++++--
> drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++--
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index f578677..ec815d7 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -3019,6 +3019,32 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> return group;
> }
>
> +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain *smmu_domain,
> + void *data)
> +{
> + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> + unsigned int size;
> +
> + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> + return -ENODEV;
> +
> + size = sizeof(struct iommu_nesting_info);
> +
> + /*
> + * if provided buffer size is smaller than expected, should
> + * return 0 and also the expected buffer size to caller.
> + */
> + if (info->size < size) {
> + info->size = size;
> + return 0;
> + }
> +
> + /* report an empty iommu_nesting_info for now */
> + memset(info, 0x0, size);
> + info->size = size;
> + return 0;
> +}
> +
> static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> enum iommu_attr attr, void *data)
> {
> @@ -3028,8 +3054,7 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> case IOMMU_DOMAIN_UNMANAGED:
> switch (attr) {
> case DOMAIN_ATTR_NESTING:
> - *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> - return 0;
> + return arm_smmu_domain_nesting_info(smmu_domain, data);
> default:
> return -ENODEV;
> }
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 243bc4c..09e2f1b 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1506,6 +1506,32 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> return group;
> }
>
> +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain *smmu_domain,
> + void *data)
> +{
> + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> + unsigned int size;
> +
> + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> + return -ENODEV;
> +
> + size = sizeof(struct iommu_nesting_info);
> +
> + /*
> + * if provided buffer size is smaller than expected, should
> + * return 0 and also the expected buffer size to caller.
> + */
> + if (info->size < size) {
> + info->size = size;
> + return 0;
> + }
> +
> + /* report an empty iommu_nesting_info for now */
> + memset(info, 0x0, size);
> + info->size = size;
> + return 0;
> +}
> +
> static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> enum iommu_attr attr, void *data)
> {
> @@ -1515,8 +1541,7 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> case IOMMU_DOMAIN_UNMANAGED:
> switch (attr) {
> case DOMAIN_ATTR_NESTING:
> - *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> - return 0;
> + return arm_smmu_domain_nesting_info(smmu_domain, data);
> default:
> return -ENODEV;
> }
>
Hi Eric,
> From: Auger Eric <[email protected]>
>
> Yi,
>
> On 7/12/20 1:20 PM, Liu Yi L wrote:
> > This patch is added as instead of returning a boolean for
> > DOMAIN_ATTR_NESTING,
> > iommu_domain_get_attr() should return an iommu_nesting_info handle.
>
> you may add in the commit message you return an empty nesting info struct for now
> as true nesting is not yet supported by the SMMUs.
will do.
> Besides:
> Reviewed-by: Eric Auger <[email protected]>
thanks.
Regards,
Yi Liu
> Thanks
>
> Eric
> >
> > Cc: Will Deacon <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Eric Auger <[email protected]>
> > Cc: Jean-Philippe Brucker <[email protected]>
> > Suggested-by: Jean-Philippe Brucker <[email protected]>
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > v4 -> v5:
> > *) address comments from Eric Auger.
> > ---
> > drivers/iommu/arm-smmu-v3.c | 29 +++++++++++++++++++++++++++--
> > drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++--
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index f578677..ec815d7 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -3019,6 +3019,32 @@ static struct iommu_group
> *arm_smmu_device_group(struct device *dev)
> > return group;
> > }
> >
> > +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain
> *smmu_domain,
> > + void *data)
> > +{
> > + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> > + unsigned int size;
> > +
> > + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> > + return -ENODEV;
> > +
> > + size = sizeof(struct iommu_nesting_info);
> > +
> > + /*
> > + * if provided buffer size is smaller than expected, should
> > + * return 0 and also the expected buffer size to caller.
> > + */
> > + if (info->size < size) {
> > + info->size = size;
> > + return 0;
> > + }
> > +
> > + /* report an empty iommu_nesting_info for now */
> > + memset(info, 0x0, size);
> > + info->size = size;
> > + return 0;
> > +}
> > +
> > static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > enum iommu_attr attr, void *data) { @@ -
> 3028,8 +3054,7 @@
> > static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > case IOMMU_DOMAIN_UNMANAGED:
> > switch (attr) {
> > case DOMAIN_ATTR_NESTING:
> > - *(int *)data = (smmu_domain->stage ==
> ARM_SMMU_DOMAIN_NESTED);
> > - return 0;
> > + return arm_smmu_domain_nesting_info(smmu_domain,
> data);
> > default:
> > return -ENODEV;
> > }
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> > 243bc4c..09e2f1b 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1506,6 +1506,32 @@ static struct iommu_group
> *arm_smmu_device_group(struct device *dev)
> > return group;
> > }
> >
> > +static int arm_smmu_domain_nesting_info(struct arm_smmu_domain
> *smmu_domain,
> > + void *data)
> > +{
> > + struct iommu_nesting_info *info = (struct iommu_nesting_info *)data;
> > + unsigned int size;
> > +
> > + if (!info || smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED)
> > + return -ENODEV;
> > +
> > + size = sizeof(struct iommu_nesting_info);
> > +
> > + /*
> > + * if provided buffer size is smaller than expected, should
> > + * return 0 and also the expected buffer size to caller.
> > + */
> > + if (info->size < size) {
> > + info->size = size;
> > + return 0;
> > + }
> > +
> > + /* report an empty iommu_nesting_info for now */
> > + memset(info, 0x0, size);
> > + info->size = size;
> > + return 0;
> > +}
> > +
> > static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > enum iommu_attr attr, void *data) { @@ -
> 1515,8 +1541,7 @@
> > static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> > case IOMMU_DOMAIN_UNMANAGED:
> > switch (attr) {
> > case DOMAIN_ATTR_NESTING:
> > - *(int *)data = (smmu_domain->stage ==
> ARM_SMMU_DOMAIN_NESTED);
> > - return 0;
> > + return arm_smmu_domain_nesting_info(smmu_domain,
> data);
> > default:
> > return -ENODEV;
> > }
> >
Hi Jean,
> From: Jean-Philippe Brucker <[email protected]>
> Sent: Thursday, July 16, 2020 11:40 PM
>
> On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
> > > Have you verified that this doesn't break the existing usage of
> > > DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
> >
> > I didn't have ARM machine on my hand. But I contacted with Jean
> > Philippe, he confirmed no compiling issue. I didn't see any code
> > getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
> > What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> > and won't fail if the iommu_domai_get_attr() returns 0. This patch
> > returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> > value is 0 if no error. So I guess it won't fail nesting for ARM.
>
> I confirm that this series doesn't break the current support for
> VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
thanks.
> If the SMMU does not support stage-2 then there is a change in behavior
> (untested): after the domain is silently switched to stage-1 by the SMMU
> driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> rather than a regression, it should have been like this since the
> beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
> far, so I don't think it should be a concern.
>
> And if userspace queries the nesting properties using the new ABI
> introduced in this patchset, it will obtain an empty struct.
yes.
> I think
> that's acceptable, but it may be better to avoid adding the nesting cap if
> @format is 0?
right. will add it in patch 4/15.
Regards,
Yi Liu
>
> Thanks,
> Jean
>
> >
> > @Eric, how about your opinion? your dual-stage vSMMU support may
> > also share the vfio_iommu_type1.c code.
> >
> > Regards,
> > Yi Liu
> >
> > > Will
Hi Jean,
> From: Jean-Philippe Brucker <[email protected]>
> Sent: Friday, July 17, 2020 5:09 PM
>
> On Thu, Jul 16, 2020 at 10:38:17PM +0200, Auger Eric wrote:
> > Hi Jean,
> >
> > On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
> > > On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
> > >>> Have you verified that this doesn't break the existing usage of
> > >>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
> > >>
> > >> I didn't have ARM machine on my hand. But I contacted with Jean
> > >> Philippe, he confirmed no compiling issue. I didn't see any code
> > >> getting DOMAIN_ATTR_NESTING attr in current
> drivers/vfio/vfio_iommu_type1.c.
> > >> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> > >> and won't fail if the iommu_domai_get_attr() returns 0. This patch
> > >> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> > >> value is 0 if no error. So I guess it won't fail nesting for ARM.
> > >
> > > I confirm that this series doesn't break the current support for
> > > VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
> > >
> > > If the SMMU does not support stage-2 then there is a change in behavior
> > > (untested): after the domain is silently switched to stage-1 by the SMMU
> > > driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> > > succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> > > rather than a regression, it should have been like this since the
> > > beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING
> so
> > > far, so I don't think it should be a concern.
> > But as Yi mentioned ealier, in the current vfio code there is no
> > DOMAIN_ATTR_NESTING query yet.
>
> That's why something that would have succeeded before will now fail:
> Before this series, if user asked for a VFIO_IOMMU_TYPE1_NESTING, it would
> have succeeded even if the SMMU didn't support stage-2, as the driver
> would have silently fallen back on stage-1 mappings (which work exactly
> the same as stage-2-only since there was no nesting supported). After the
> series, we do check for DOMAIN_ATTR_NESTING so if user asks for
> VFIO_IOMMU_TYPE1_NESTING and the SMMU doesn't support stage-2, the ioctl
> fails.
I think this depends on iommu driver. I noticed current SMMU driver
doesn't check physical IOMMU about nesting capability. So I guess
the SET_IOMMU would still work for SMMU. but it will fail as you
mentioned in prior email, userspace will check the nesting info and
would fail as it gets an empty struct from host.
https://lore.kernel.org/kvm/20200716153959.GA447208@myrica/
>
> I believe it's a good fix and completely harmless, but wanted to make sure
> no one objects because it's an ABI change.
yes.
Regards,
Yi Liu
> Thanks,
> Jean
>
> > In my SMMUV3 nested stage series, I added
> > such a query in vfio-pci.c to detect if I need to expose a fault region
> > but I already test both the returned value and the output arg. So to me
> > there is no issue with that change.
> > >
> > > And if userspace queries the nesting properties using the new ABI
> > > introduced in this patchset, it will obtain an empty struct. I think
> > > that's acceptable, but it may be better to avoid adding the nesting cap if
> > > @format is 0?
> > agreed
> >
> > Thanks
> >
> > Eric
> > >
> > > Thanks,
> > > Jean
> > >
> > >>
> > >> @Eric, how about your opinion? your dual-stage vSMMU support may
> > >> also share the vfio_iommu_type1.c code.
> > >>
> > >> Regards,
> > >> Yi Liu
> > >>
> > >>> Will
> > >
> >