RE: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Eric,
> From: Auger Eric
> Sent: Friday, February 12, 2021 5:58 PM
>
> Hi Vivek, Yi,
>
> On 2/12/21 8:14 AM, Vivek Gautam wrote:
> > Hi Yi,
> >
> >
> > On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L wrote:
> >>
> >> Hi Eric,
> >>
> >>> From: Auger Eric
> >>> Sent: Tuesday, January 19, 2021 6:03 PM
> >>>
> >>> Hi Yi, Vivek,
> >>>
> >> [...]
> I see. I think there needs a change in the code there. Should also expect
> a nesting_info returned instead of an int anymore. @Eric, how about your
> opinion?
>
> domain = iommu_get_domain_for_dev(>pdev->dev);
> ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
> >>> );
> if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
> /*
> * No need go futher as no page request service support.
> */
> return 0;
> }
> >>> Sure I think it is "just" a matter of synchro between the 2 series. Yi,
> >>
> >> exactly.
> >>
> >>> do you have plans to respin part of
> >>> [PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
> >>> or would you allow me to embed this patch in my series.
> >>
> >> My v7 hasn’t touch the prq change yet. So I think it's better for you to
> >> embed it to your series. ^_^>>
> >
> > Can you please let me know if you have an updated series of these
> > patches? It will help me to work with virtio-iommu/arm side changes.
>
> As per the previous discussion, I plan to take those 2 patches in my
> SMMUv3 nested stage series:
>
> [PATCH v7 01/16] iommu: Report domain nesting info
> [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
>
> we need to upgrade both since we do not want to report an empty nesting
> info anymore, for arm.
sorry for the late response. I've sent out the updated version. Also,
yeah, please feel free to take the patch in your series.
https://lore.kernel.org/linux-iommu/20210302203545.436623-2-yi.l@intel.com/
Regards,
Yi Liu
> Thanks
>
> Eric
> >
> > Thanks & regards
> > Vivek
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Eric,
On 2/12/21 3:48 PM, Vivek Kumar Gautam wrote:
Hi Eric,
On 2/12/21 3:27 PM, Auger Eric wrote:
Hi Vivek, Yi,
On 2/12/21 8:14 AM, Vivek Gautam wrote:
Hi Yi,
On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L wrote:
Hi Eric,
From: Auger Eric
Sent: Tuesday, January 19, 2021 6:03 PM
Hi Yi, Vivek,
[...]
I see. I think there needs a change in the code there. Should also
expect
a nesting_info returned instead of an int anymore. @Eric, how
about your
opinion?
domain = iommu_get_domain_for_dev(>pdev->dev);
ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
);
if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
/*
* No need go futher as no page request service support.
*/
return 0;
}
Sure I think it is "just" a matter of synchro between the 2 series.
Yi,
exactly.
do you have plans to respin part of
[PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
or would you allow me to embed this patch in my series.
My v7 hasn’t touch the prq change yet. So I think it's better for
you to
embed it to your series. ^_^>>
Can you please let me know if you have an updated series of these
patches? It will help me to work with virtio-iommu/arm side changes.
As per the previous discussion, I plan to take those 2 patches in my
SMMUv3 nested stage series:
[PATCH v7 01/16] iommu: Report domain nesting info
[PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
we need to upgrade both since we do not want to report an empty nesting
info anymore, for arm.
Absolutely. Let me send the couple of patches that I have been using,
that add arm configuration.
Posted the couple of patches that I have been using -
https://lore.kernel.org/linux-iommu/20210212105859.8445-1-vivek.gau...@arm.com/T/#t
Thanks & regards
Vivek
Best regards
Vivek
Thanks
Eric
Thanks & regards
Vivek
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Eric,
On 2/12/21 3:27 PM, Auger Eric wrote:
Hi Vivek, Yi,
On 2/12/21 8:14 AM, Vivek Gautam wrote:
Hi Yi,
On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L wrote:
Hi Eric,
From: Auger Eric
Sent: Tuesday, January 19, 2021 6:03 PM
Hi Yi, Vivek,
[...]
I see. I think there needs a change in the code there. Should also expect
a nesting_info returned instead of an int anymore. @Eric, how about your
opinion?
domain = iommu_get_domain_for_dev(>pdev->dev);
ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
);
if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
/*
* No need go futher as no page request service support.
*/
return 0;
}
Sure I think it is "just" a matter of synchro between the 2 series. Yi,
exactly.
do you have plans to respin part of
[PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
or would you allow me to embed this patch in my series.
My v7 hasn’t touch the prq change yet. So I think it's better for you to
embed it to your series. ^_^>>
Can you please let me know if you have an updated series of these
patches? It will help me to work with virtio-iommu/arm side changes.
As per the previous discussion, I plan to take those 2 patches in my
SMMUv3 nested stage series:
[PATCH v7 01/16] iommu: Report domain nesting info
[PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
we need to upgrade both since we do not want to report an empty nesting
info anymore, for arm.
Absolutely. Let me send the couple of patches that I have been using,
that add arm configuration.
Best regards
Vivek
Thanks
Eric
Thanks & regards
Vivek
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Vivek, Yi,
On 2/12/21 8:14 AM, Vivek Gautam wrote:
> Hi Yi,
>
>
> On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L wrote:
>>
>> Hi Eric,
>>
>>> From: Auger Eric
>>> Sent: Tuesday, January 19, 2021 6:03 PM
>>>
>>> Hi Yi, Vivek,
>>>
>> [...]
I see. I think there needs a change in the code there. Should also expect
a nesting_info returned instead of an int anymore. @Eric, how about your
opinion?
domain = iommu_get_domain_for_dev(>pdev->dev);
ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
>>> );
if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
/*
* No need go futher as no page request service support.
*/
return 0;
}
>>> Sure I think it is "just" a matter of synchro between the 2 series. Yi,
>>
>> exactly.
>>
>>> do you have plans to respin part of
>>> [PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
>>> or would you allow me to embed this patch in my series.
>>
>> My v7 hasn’t touch the prq change yet. So I think it's better for you to
>> embed it to your series. ^_^>>
>
> Can you please let me know if you have an updated series of these
> patches? It will help me to work with virtio-iommu/arm side changes.
As per the previous discussion, I plan to take those 2 patches in my
SMMUv3 nested stage series:
[PATCH v7 01/16] iommu: Report domain nesting info
[PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
we need to upgrade both since we do not want to report an empty nesting
info anymore, for arm.
Thanks
Eric
>
> Thanks & regards
> Vivek
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Yi,
On Sat, Jan 23, 2021 at 2:29 PM Liu, Yi L wrote:
>
> Hi Eric,
>
> > From: Auger Eric
> > Sent: Tuesday, January 19, 2021 6:03 PM
> >
> > Hi Yi, Vivek,
> >
> [...]
> > > I see. I think there needs a change in the code there. Should also expect
> > > a nesting_info returned instead of an int anymore. @Eric, how about your
> > > opinion?
> > >
> > > domain = iommu_get_domain_for_dev(>pdev->dev);
> > > ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
> > );
> > > if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
> > > /*
> > > * No need go futher as no page request service support.
> > > */
> > > return 0;
> > > }
> > Sure I think it is "just" a matter of synchro between the 2 series. Yi,
>
> exactly.
>
> > do you have plans to respin part of
> > [PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
> > or would you allow me to embed this patch in my series.
>
> My v7 hasn’t touch the prq change yet. So I think it's better for you to
> embed it to your series. ^_^
>
Can you please let me know if you have an updated series of these
patches? It will help me to work with virtio-iommu/arm side changes.
Thanks & regards
Vivek
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Eric,
> From: Auger Eric
> Sent: Tuesday, January 19, 2021 6:03 PM
>
> Hi Yi, Vivek,
>
[...]
> > I see. I think there needs a change in the code there. Should also expect
> > a nesting_info returned instead of an int anymore. @Eric, how about your
> > opinion?
> >
> > domain = iommu_get_domain_for_dev(>pdev->dev);
> > ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING,
> );
> > if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
> > /*
> > * No need go futher as no page request service support.
> > */
> > return 0;
> > }
> Sure I think it is "just" a matter of synchro between the 2 series. Yi,
exactly.
> do you have plans to respin part of
> [PATCH v7 00/16] vfio: expose virtual Shared Virtual Addressing to VMs
> or would you allow me to embed this patch in my series.
My v7 hasn’t touch the prq change yet. So I think it's better for you to
embed it to your series. ^_^
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Yi, Vivek,
On 1/13/21 6:56 AM, Liu, Yi L wrote:
> Hi Vivek,
>
>> From: Vivek Gautam
>> Sent: Tuesday, January 12, 2021 7:06 PM
>>
>> Hi Yi,
>>
>>
>> On Tue, Jan 12, 2021 at 2:51 PM Liu, Yi L wrote:
>>>
>>> Hi Vivek,
>>>
From: Vivek Gautam
Sent: Tuesday, January 12, 2021 2:50 PM
Hi Yi,
On Thu, Sep 10, 2020 at 4:13 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.
For
> now, return an empty nesting info struct for now as true nesting is not
> yet supported by the SMMUs.
>
> Cc: Will Deacon
> Cc: Robin Murphy
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Suggested-by: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Jacob Pan
> Reviewed-by: Eric Auger
> ---
> v5 -> v6:
> *) add review-by from Eric Auger.
>
> v4 -> v5:
> *) address comments from Eric Auger.
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29
+++--
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29
+++--
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 7196207..016e2e5 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/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->argsz < size) {
> + info->argsz = size;
> + return 0;
> + }
> +
> + /* report an empty iommu_nesting_info for now */
> + memset(info, 0x0, size);
> + info->argsz = 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);
Thanks for the patch.
This would unnecessarily overflow 'data' for any caller that's expecting
>> only
an int data. Dump from one such issue that I was seeing when testing
this change along with local kvmtool changes is pasted below [1].
I could get around with the issue by adding another (iommu_attr) -
DOMAIN_ATTR_NESTING_INFO that returns (iommu_nesting_info).
>>>
>>> nice to hear from you. At first, we planned to have a separate iommu_attr
>>> for getting nesting_info. However, we considered there is no existing user
>>> which gets DOMAIN_ATTR_NESTING, so we decided to reuse it for iommu
>> nesting
>>> info. Could you share me the code base you are using? If the error you
>>> encountered is due to this change, so there should be a place which gets
>>> DOMAIN_ATTR_NESTING.
>>
>> I am currently working on top of Eric's tree for nested stage support [1].
>> My best guess was that the vfio_pci_dma_fault_init() method [2] that is
>> requesting DOMAIN_ATTR_NESTING causes stack overflow, and corruption.
>> That's when I added a new attribute.
>
> I see. I think there needs a change in the code there. Should also expect
> a nesting_info returned instead of an int anymore. @Eric, how about your
> opinion?
>
> domain = iommu_get_domain_for_dev(>pdev->dev);
> ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING, );
> if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
> /*
>* No need go futher as no page request service support.
>*/
> return 0;
> }
Sure I think it is "just" a matter of synchro between the 2 series. Yi,
do you have plans to
RE: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Vivek,
> From: Vivek Gautam
> Sent: Tuesday, January 12, 2021 7:06 PM
>
> Hi Yi,
>
>
> On Tue, Jan 12, 2021 at 2:51 PM Liu, Yi L wrote:
> >
> > Hi Vivek,
> >
> > > From: Vivek Gautam
> > > Sent: Tuesday, January 12, 2021 2:50 PM
> > >
> > > Hi Yi,
> > >
> > >
> > > On Thu, Sep 10, 2020 at 4:13 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.
> > > For
> > > > now, return an empty nesting info struct for now as true nesting is not
> > > > yet supported by the SMMUs.
> > > >
> > > > Cc: Will Deacon
> > > > Cc: Robin Murphy
> > > > Cc: Eric Auger
> > > > Cc: Jean-Philippe Brucker
> > > > Suggested-by: Jean-Philippe Brucker
> > > > Signed-off-by: Liu Yi L
> > > > Signed-off-by: Jacob Pan
> > > > Reviewed-by: Eric Auger
> > > > ---
> > > > v5 -> v6:
> > > > *) add review-by from Eric Auger.
> > > >
> > > > v4 -> v5:
> > > > *) address comments from Eric Auger.
> > > > ---
> > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29
> > > +++--
> > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 29
> > > +++--
> > > > 2 files changed, 54 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > index 7196207..016e2e5 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/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->argsz < size) {
> > > > + info->argsz = size;
> > > > + return 0;
> > > > + }
> > > > +
> > > > + /* report an empty iommu_nesting_info for now */
> > > > + memset(info, 0x0, size);
> > > > + info->argsz = 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);
> > >
> > > Thanks for the patch.
> > > This would unnecessarily overflow 'data' for any caller that's expecting
> only
> > > an int data. Dump from one such issue that I was seeing when testing
> > > this change along with local kvmtool changes is pasted below [1].
> > >
> > > I could get around with the issue by adding another (iommu_attr) -
> > > DOMAIN_ATTR_NESTING_INFO that returns (iommu_nesting_info).
> >
> > nice to hear from you. At first, we planned to have a separate iommu_attr
> > for getting nesting_info. However, we considered there is no existing user
> > which gets DOMAIN_ATTR_NESTING, so we decided to reuse it for iommu
> nesting
> > info. Could you share me the code base you are using? If the error you
> > encountered is due to this change, so there should be a place which gets
> > DOMAIN_ATTR_NESTING.
>
> I am currently working on top of Eric's tree for nested stage support [1].
> My best guess was that the vfio_pci_dma_fault_init() method [2] that is
> requesting DOMAIN_ATTR_NESTING causes stack overflow, and corruption.
> That's when I added a new attribute.
I see. I think there needs a change in the code there. Should also expect
a nesting_info returned instead of an int anymore. @Eric, how about your
opinion?
domain = iommu_get_domain_for_dev(>pdev->dev);
ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_NESTING, );
if (ret || !(info.features & IOMMU_NESTING_FEAT_PAGE_RESP)) {
/*
* No need go futher as no page request service support.
*/
return 0;
}
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Yi,
On Tue, Jan 12, 2021 at 2:51 PM Liu, Yi L wrote:
>
> Hi Vivek,
>
> > From: Vivek Gautam
> > Sent: Tuesday, January 12, 2021 2:50 PM
> >
> > Hi Yi,
> >
> >
> > On Thu, Sep 10, 2020 at 4:13 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.
> > For
> > > now, return an empty nesting info struct for now as true nesting is not
> > > yet supported by the SMMUs.
> > >
> > > Cc: Will Deacon
> > > Cc: Robin Murphy
> > > Cc: Eric Auger
> > > Cc: Jean-Philippe Brucker
> > > Suggested-by: Jean-Philippe Brucker
> > > Signed-off-by: Liu Yi L
> > > Signed-off-by: Jacob Pan
> > > Reviewed-by: Eric Auger
> > > ---
> > > v5 -> v6:
> > > *) add review-by from Eric Auger.
> > >
> > > v4 -> v5:
> > > *) address comments from Eric Auger.
> > > ---
> > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29
> > +++--
> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 29
> > +++--
> > > 2 files changed, 54 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > index 7196207..016e2e5 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/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->argsz < size) {
> > > + info->argsz = size;
> > > + return 0;
> > > + }
> > > +
> > > + /* report an empty iommu_nesting_info for now */
> > > + memset(info, 0x0, size);
> > > + info->argsz = 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);
> >
> > Thanks for the patch.
> > This would unnecessarily overflow 'data' for any caller that's expecting
> > only
> > an int data. Dump from one such issue that I was seeing when testing
> > this change along with local kvmtool changes is pasted below [1].
> >
> > I could get around with the issue by adding another (iommu_attr) -
> > DOMAIN_ATTR_NESTING_INFO that returns (iommu_nesting_info).
>
> nice to hear from you. At first, we planned to have a separate iommu_attr
> for getting nesting_info. However, we considered there is no existing user
> which gets DOMAIN_ATTR_NESTING, so we decided to reuse it for iommu nesting
> info. Could you share me the code base you are using? If the error you
> encountered is due to this change, so there should be a place which gets
> DOMAIN_ATTR_NESTING.
I am currently working on top of Eric's tree for nested stage support [1].
My best guess was that the vfio_pci_dma_fault_init() method [2] that is
requesting DOMAIN_ATTR_NESTING causes stack overflow, and corruption.
That's when I added a new attribute.
I will soon publish my patches to the list for review. Let me know
your thoughts.
[1] https://github.com/eauger/linux/tree/5.10-rc4-2stage-v13
[2]
https://github.com/eauger/linux/blob/5.10-rc4-2stage-v13/drivers/vfio/pci/vfio_pci.c#L494
Thanks
Vivek
>
> Regards,
> Yi Liu
[snip]
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Vivek,
> From: Vivek Gautam
> Sent: Tuesday, January 12, 2021 2:50 PM
>
> Hi Yi,
>
>
> On Thu, Sep 10, 2020 at 4:13 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.
> For
> > now, return an empty nesting info struct for now as true nesting is not
> > yet supported by the SMMUs.
> >
> > Cc: Will Deacon
> > Cc: Robin Murphy
> > Cc: Eric Auger
> > Cc: Jean-Philippe Brucker
> > Suggested-by: Jean-Philippe Brucker
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > Reviewed-by: Eric Auger
> > ---
> > v5 -> v6:
> > *) add review-by from Eric Auger.
> >
> > v4 -> v5:
> > *) address comments from Eric Auger.
> > ---
> > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29
> +++--
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 29
> +++--
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 7196207..016e2e5 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/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->argsz < size) {
> > + info->argsz = size;
> > + return 0;
> > + }
> > +
> > + /* report an empty iommu_nesting_info for now */
> > + memset(info, 0x0, size);
> > + info->argsz = 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);
>
> Thanks for the patch.
> This would unnecessarily overflow 'data' for any caller that's expecting only
> an int data. Dump from one such issue that I was seeing when testing
> this change along with local kvmtool changes is pasted below [1].
>
> I could get around with the issue by adding another (iommu_attr) -
> DOMAIN_ATTR_NESTING_INFO that returns (iommu_nesting_info).
nice to hear from you. At first, we planned to have a separate iommu_attr
for getting nesting_info. However, we considered there is no existing user
which gets DOMAIN_ATTR_NESTING, so we decided to reuse it for iommu nesting
info. Could you share me the code base you are using? If the error you
encountered is due to this change, so there should be a place which gets
DOMAIN_ATTR_NESTING.
Regards,
Yi Liu
> Thanks & regards
> Vivek
>
> [1]--
> [ 811.756516] vfio-pci :08:00.1: vfio_ecap_init: hiding ecap
> 0x1b@0x108
> [ 811.756516] Kernel panic - not syncing: stack-protector: Kernel
> stack is corrupted in: vfio_pci_open+0x644/0x648
> [ 811.756516] CPU: 0 PID: 175 Comm: lkvm-cleanup-ne Not tainted
> 5.10.0-rc5-00096-gf015061e14cf #43
> [ 811.756516] Call trace:
> [ 811.756516] dump_backtrace+0x0/0x1b0
> [ 811.756516] show_stack+0x18/0x68
> [ 811.756516] dump_stack+0xd8/0x134
> [ 811.756516] panic+0x174/0x33c
> [ 811.756516] __stack_chk_fail+0x3c/0x40
> [ 811.756516] vfio_pci_open+0x644/0x648
> [ 811.756516] vfio_group_fops_unl_ioctl+0x4bc/0x648
> [ 811.756516] 0x0
> [ 811.756516] SMP: stopping secondary CPUs
> [ 811.756597] Kernel Offset: disabled
> [ 811.756597] CPU features: 0x0040006,6a00aa38
> [ 811.756602] Memory Limit: none
> [ 811.768497] ---[ end Kernel panic - not syncing: stack-protector:
> Kernel stack is corrupted in: vfio_pci_open+0x644/0x648 ]
> -
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 02/16] iommu/smmu: Report empty domain nesting info
Hi Yi,
On Thu, Sep 10, 2020 at 4:13 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. For
> now, return an empty nesting info struct for now as true nesting is not
> yet supported by the SMMUs.
>
> Cc: Will Deacon
> Cc: Robin Murphy
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Suggested-by: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Jacob Pan
> Reviewed-by: Eric Auger
> ---
> v5 -> v6:
> *) add review-by from Eric Auger.
>
> v4 -> v5:
> *) address comments from Eric Auger.
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29
> +++--
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29
> +++--
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 7196207..016e2e5 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/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->argsz < size) {
> + info->argsz = size;
> + return 0;
> + }
> +
> + /* report an empty iommu_nesting_info for now */
> + memset(info, 0x0, size);
> + info->argsz = 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);
Thanks for the patch.
This would unnecessarily overflow 'data' for any caller that's expecting only
an int data. Dump from one such issue that I was seeing when testing
this change along with local kvmtool changes is pasted below [1].
I could get around with the issue by adding another (iommu_attr) -
DOMAIN_ATTR_NESTING_INFO that returns (iommu_nesting_info).
Thanks & regards
Vivek
[1]--
[ 811.756516] vfio-pci :08:00.1: vfio_ecap_init: hiding ecap 0x1b@0x108
[ 811.756516] Kernel panic - not syncing: stack-protector: Kernel
stack is corrupted in: vfio_pci_open+0x644/0x648
[ 811.756516] CPU: 0 PID: 175 Comm: lkvm-cleanup-ne Not tainted
5.10.0-rc5-00096-gf015061e14cf #43
[ 811.756516] Call trace:
[ 811.756516] dump_backtrace+0x0/0x1b0
[ 811.756516] show_stack+0x18/0x68
[ 811.756516] dump_stack+0xd8/0x134
[ 811.756516] panic+0x174/0x33c
[ 811.756516] __stack_chk_fail+0x3c/0x40
[ 811.756516] vfio_pci_open+0x644/0x648
[ 811.756516] vfio_group_fops_unl_ioctl+0x4bc/0x648
[ 811.756516] 0x0
[ 811.756516] SMP: stopping secondary CPUs
[ 811.756597] Kernel Offset: disabled
[ 811.756597] CPU features: 0x0040006,6a00aa38
[ 811.756602] Memory Limit: none
[ 811.768497] ---[ end Kernel panic - not syncing: stack-protector:
Kernel stack is corrupted in: vfio_pci_open+0x644/0x648 ]
-
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
