Hi Nicolin,

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, February 5, 2025 12:09 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Eric Auger
> <eric.auger@redhat.com>
> Cc: ddutile@redhat.com; Peter Maydell <peter.maydell@linaro.org>; Jason
> Gunthorpe <jgg@nvidia.com>; Daniel P. Berrangé <berrange@redhat.com>;
> qemu-arm@nongnu.org; qemu-devel@nongnu.org; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [RFC PATCH 0/5] hw/arm/virt: Add support for user-creatable
> nested SMMUv3
> 
> On Tue, Feb 04, 2025 at 06:49:15PM +0100, Eric Auger wrote:
> > > In summary, we will have the following series:
> > > 1) HWPT uAPI patches in backends/iommufd.c (Zhenzhong or Shameer)
> > >    https://lore.kernel.org/qemu-
> devel/SJ0PR11MB6744943702EB5798EC9B3B9992E02@SJ0PR11MB6744.nam
> prd11.prod.outlook.com/
> > > 2) vIOMMU uAPI patches in backends/iommufd.c (I will rebase/send)
> 
> > for 1 and 2, are you taking about the "Add VIOMMU infrastructure
> support
> > " series in Shameer's branch: private-smmuv3-nested-dev-rfc-v1.
> > Sorry I may instead refer to NVidia or Intel's branch but I am not sure
> > about the last ones.
> 
> That "vIOMMU infrastructure" is for 2, yes.
> 
> For 1, it's inside the Intel's series:
> "cover-letter: intel_iommu: Enable stage-1 translation for passthrough
> device"
> 
> So, we need to extract them out and make it separately..
> 
> > > 3) vSMMUv3 patches for HW-acc/nesting (Hoping Don/you could take
> over)
> > We can start sending it upstream assuming we have a decent test
> environment.
> >
> > However in
> >
> https://lore.kernel.org/all/329445b2f68a47269292aefb34584375@huawei.c
> om/
> >
> > Shameer suggested he may include it in his SMMU multi instance series.
> > What do you both prefer?
> 
> Sure, I think it's good to include those patches, 

One of the feedback I received on my series was to rename "arm-smmuv3-nested"
to "arm-smmuv3-accel" and possibly rename function names to include "accel' as well
and move those functions to a separate "smmuv3-accel.c" file. I suppose that applies to 
the " Add HW accelerated nesting support for arm SMMUv3" series as well. 

Is that fine with you?

Thanks,
Shameer

On Thu, Feb 06, 2025 at 10:34:15AM +0000, Shameerali Kolothum Thodi wrote:
> > -----Original Message-----
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > On Tue, Feb 04, 2025 at 06:49:15PM +0100, Eric Auger wrote:
> > > However in
> > >
> > > Shameer suggested he may include it in his SMMU multi instance series.
> > > What do you both prefer?
> > 
> > Sure, I think it's good to include those patches, 
> 
> One of the feedback I received on my series was to rename "arm-smmuv3-nested"
> to "arm-smmuv3-accel" and possibly rename function names to include "accel' as well
> and move those functions to a separate "smmuv3-accel.c" file. I suppose that applies to 
> the " Add HW accelerated nesting support for arm SMMUv3" series as well. 
> 
> Is that fine with you?

Oh, no problem. If you want to rename the whole thing, please feel
free. I do see the naming conflict between the "nested" stage and
the "nested" HW feature, which are both supported by the vSMMU now.

Thanks
Nicolin

Hi Nicolin,

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, February 6, 2025 6:58 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Eric Auger <eric.auger@redhat.com>; ddutile@redhat.com; Peter
> Maydell <peter.maydell@linaro.org>; Jason Gunthorpe <jgg@nvidia.com>;
> Daniel P. Berrangé <berrange@redhat.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>; Wangzhou
> (B) <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org
> Subject: Re: [RFC PATCH 0/5] hw/arm/virt: Add support for user-creatable
> nested SMMUv3
> 
[..]

> > One of the feedback I received on my series was to rename "arm-smmuv3-
> nested"
> > to "arm-smmuv3-accel" and possibly rename function names to include
> "accel' as well
> > and move those functions to a separate "smmuv3-accel.c" file. I suppose
> that applies to
> > the " Add HW accelerated nesting support for arm SMMUv3" series as
> well.
> >
> > Is that fine with you?
> 
> Oh, no problem. If you want to rename the whole thing, please feel
> free. I do see the naming conflict between the "nested" stage and
> the "nested" HW feature, which are both supported by the vSMMU now.

I am working on the above now and have quick question to you😊.

Looking at the smmu_dev_attach_viommu() fn here[0],
it appears to do the following:

1. Alloc a s2_hwpt if not allocated already and attach it.
2. Allocate abort and bypass hwpt
3. Attach bypass hwpt.

I didn't get why we are doing the step 3 here. To me it looks like,
when we attach the s2_hwpt(ie, the nested parent domain attach), 
the kernel will do,

arm_smmu_attach_dev()
  arm_smmu_make_s2_domain_ste()

It appears through step 3, we achieve the same thing again.

Or it is possible I missed something obvious here.

Please let me know.

Thanks,
Shameer

[0] https://github.com/nicolinc/qemu/blob/wip/for_shameer_02042025/hw/arm/smmu-common.c#L910C13-L910C35

On Mon, Mar 03, 2025 at 03:21:57PM +0000, Shameerali Kolothum Thodi wrote:
> I am working on the above now and have quick question to you😊.
> 
> Looking at the smmu_dev_attach_viommu() fn here[0],
> it appears to do the following:
> 
> 1. Alloc a s2_hwpt if not allocated already and attach it.
> 2. Allocate abort and bypass hwpt
> 3. Attach bypass hwpt.
> 
> I didn't get why we are doing the step 3 here. To me it looks like,
> when we attach the s2_hwpt(ie, the nested parent domain attach), 
> the kernel will do,
> 
> arm_smmu_attach_dev()
>   arm_smmu_make_s2_domain_ste()
> 
> It appears through step 3, we achieve the same thing again.
> 
> Or it is possible I missed something obvious here.

Because a device cannot attach to a vIOMMU object directly, but
only via a proxy hwpt_nested. So, this bypass hwpt gives us the
port to associate the device to the vIOMMU, before a vDEVICE or
a "translate" hwpt_nested is allocated.

Currently it's the same because an S2 parent hwpt holds a VMID,
so we could just attach the device to the S2 hwpt for the same
STE configuration as attaching the device to the proxy bypass
hwpt. Yet, this will change in the future after letting vIOMMU
objects hold their own VMIDs to share a common S2 parent hwpt
that won't have a VMID, i.e. arm_smmu_make_s2_domain_ste() will
need the vIOMMU object to get the VMID for STE.

I should have added a few lines of comments there :)

Thanks
Nicolin

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Monday, March 3, 2025 5:05 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Eric Auger <eric.auger@redhat.com>; ddutile@redhat.com; Peter
> Maydell <peter.maydell@linaro.org>; Jason Gunthorpe <jgg@nvidia.com>;
> Daniel P. Berrangé <berrange@redhat.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>; Wangzhou
> (B) <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org
> Subject: Re: [RFC PATCH 0/5] hw/arm/virt: Add support for user-creatable
> nested SMMUv3
> 
> On Mon, Mar 03, 2025 at 03:21:57PM +0000, Shameerali Kolothum Thodi
> wrote:
> > I am working on the above now and have quick question to you😊.
> >
> > Looking at the smmu_dev_attach_viommu() fn here[0],
> > it appears to do the following:
> >
> > 1. Alloc a s2_hwpt if not allocated already and attach it.
> > 2. Allocate abort and bypass hwpt
> > 3. Attach bypass hwpt.
> >
> > I didn't get why we are doing the step 3 here. To me it looks like,
> > when we attach the s2_hwpt(ie, the nested parent domain attach),
> > the kernel will do,
> >
> > arm_smmu_attach_dev()
> >   arm_smmu_make_s2_domain_ste()
> >
> > It appears through step 3, we achieve the same thing again.
> >
> > Or it is possible I missed something obvious here.
> 
> Because a device cannot attach to a vIOMMU object directly, but
> only via a proxy hwpt_nested. So, this bypass hwpt gives us the
> port to associate the device to the vIOMMU, before a vDEVICE or
> a "translate" hwpt_nested is allocated.
> 
> Currently it's the same because an S2 parent hwpt holds a VMID,
> so we could just attach the device to the S2 hwpt for the same
> STE configuration as attaching the device to the proxy bypass
> hwpt. Yet, this will change in the future after letting vIOMMU
> objects hold their own VMIDs to share a common S2 parent hwpt
> that won't have a VMID, i.e. arm_smmu_make_s2_domain_ste() will
> need the vIOMMU object to get the VMID for STE.
> 
> I should have added a few lines of comments there :)

Ok. Thanks for the explanation. I will keep it then and add few comments
to make it clear.

Do you have an initial implementation of the above with vIOMMU object
holding the VMIDs to share? Actually I do have a dependency on that for
my KVM pinned VMID series[0] where it was suggested that the VMID
should associated with a vIOMMU object rather than the IOMMUFD
context I used in there.

And Jason mentioned about the work involved to do that here[1]. Appreciate
if you could share if any progress is made on that so that I can try to rebase
that KVM  Pinned series on top of that and give it a try.

Thanks,
Shameer
[0] https://lore.kernel.org/linux-iommu/20240208151837.35068-1-shameerali.kolothum.thodi@huawei.com/
[1] https://lore.kernel.org/linux-arm-kernel/20241129150628.GG1253388@nvidia.com/