RE: [PATCH V2 4/4] intel-iommu: PASID support

[Tian, Kevin]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, March 28, 2022 10:31 AM
> 
> On Thu, Mar 24, 2022 at 4:54 PM Tian, Kevin <kevin.tian@intel.com> wrote:
> >
> > > From: Jason Wang
> > > Sent: Monday, March 21, 2022 1:54 PM
> > >
> > > This patch introduce ECAP_PASID via "x-pasid-mode". Based on the
> > > existing support for scalable mode, we need to implement the following
> > > missing parts:
> > >
> > > 1) tag VTDAddressSpace with PASID and support IOMMU/DMA
> translation
> > >    with PASID
> > > 2) tag IOTLB with PASID
> >
> > and invalidate desc to flush PASID iotlb, which seems missing in this patch.
> 
> It existed in the previous version, but it looks like it will be used
> only for the first level page table which is not supported right now.
> So I deleted the codes.

You are right. But there is also PASID-based device TLB invalidate descriptor
which is orthogonal to 1st vs. 2nd level thing. If we don't want to break the
spec with this series then there will need a way to prevent the user from
setting both "device-iotlb" and "x-pasid-mode" together.

> 
> >
> > > 3) PASID cache and its flush
> > > 4) Fault recording with PASID
> > >
> > > For simplicity:
> > >
> > > 1) PASID cache is not implemented so we can simply implement the PASID
> > > cache flush as a nop.
> > > 2) Fault recording with PASID is not supported, NFR is not changed.
> > >
> > > All of the above is not mandatory and could be implemented in the
> > > future.
> >
> > PASID cache is optional, but fault recording with PASID is required.
> 
> Any pointer in the spec to say something like this? I think sticking
> to the NFR would be sufficient.

I didn't remember any place in spec saying that fault recording with PASID is
not required when PASID capability is exposed. If there is certain fault
triggered by a request with PASID, we do want to report this information
upward. 

btw can you elaborate why NFR matters to PASID? It is just about the
number of fault recording register...

> 
> > I'm fine with adding it incrementally but want to clarify the concept first.
> 
> Yes, that's the plan.
> 

I have one open which requires your input.

While incrementally enabling things does be a common practice, one worry
is whether we want to create too many control knobs in the staging process
to cause confusion to the end user.

Earlier when Yi proposed Qemu changes for guest SVA [1] he aimed for a
coarse-grained knob design:
--
  Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
  related to scalable mode translation, thus there are multiple combinations.
  While this vIOMMU implementation wants simplify it for user by providing
  typical combinations. User could config it by "x-scalable-mode" option. The
  usage is as below:
    "-device intel-iommu,x-scalable-mode=["legacy"|"modern"]"

    - "legacy": gives support for SL page table
    - "modern": gives support for FL page table, pasid, virtual command
    -  if not configured, means no scalable mode support, if not proper
       configured, will throw error
--

Which way do you prefer to?

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg02805.html

Thanks
Kevin