Re: [PATCH v6 02/15] iommu: Report domain nesting info

From: Jacob Pan
Date: Tue Aug 18 2020 - 00:15:10 EST

Next message: Martin KaFai Lau: "Re: [PATCH bpf-next v8 6/7] bpf: Allow local storage to be used from LSM programs"
Previous message: Brooke Basile: "[PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning"
In reply to: Auger Eric: "Re: [PATCH v6 02/15] iommu: Report domain nesting info"
Next in thread: Auger Eric: "Re: [PATCH v6 02/15] iommu: Report domain nesting info"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, 16 Aug 2020 14:40:57 +0200
Auger Eric <eric.auger@xxxxxxxxxx> wrote:

> Hi Yi,
>
> On 8/14/20 9:15 AM, Liu, Yi L wrote:
> > Hi Eric,
> >
> >> From: Auger Eric <eric.auger@xxxxxxxxxx>
> >> Sent: Thursday, August 13, 2020 8:53 PM
> >>
> >> Yi,
> >> On 7/28/20 8:27 AM, Liu Yi L wrote:
> >>> IOMMUs that support nesting translation needs report the
> >>> capability info
> >> s/needs/need to
> >>> to userspace. It gives information about requirements the
> >>> userspace needs to implement plus other features characterizing
> >>> the physical implementation.
> >>>
> >>> This patch reports nesting info by DOMAIN_ATTR_NESTING. Caller
> >>> can get nesting info after setting DOMAIN_ATTR_NESTING. For VFIO,
> >>> it is after selecting VFIO_TYPE1_NESTING_IOMMU.
> >> This is not what this patch does ;-) It introduces a new IOMMU UAPI
> >> struct that gives information about the nesting capabilities and
> >> features. This struct is supposed to be returned by
> >> iommu_domain_get_attr() with DOMAIN_ATTR_NESTING attribute
> >> parameter, one a domain whose type has been set to
> >> DOMAIN_ATTR_NESTING.
> >
> > got it. let me apply your suggestion. thanks. :-)
> >
> >>>
> >>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> >>> CC: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>> Cc: Eric Auger <eric.auger@xxxxxxxxxx>
> >>> Cc: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> >>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> >>> Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >>> ---
> >>> v5 -> v6:
> >>> *) rephrase the feature notes per comments from Eric Auger.
> >>> *) rename @size of struct iommu_nesting_info to @argsz.
> >>>
> >>> v4 -> v5:
> >>> *) address comments from Eric Auger.
> >>>
> >>> v3 -> v4:
> >>> *) split the SMMU driver changes to be a separate patch
> >>> *) move the @addr_width and @pasid_bits from vendor specific
> >>> part to generic part.
> >>> *) tweak the description for the @features field of struct
> >>> iommu_nesting_info.
> >>> *) add description on the @data[] field of struct
> >>> iommu_nesting_info
> >>>
> >>> v2 -> v3:
> >>> *) remvoe cap/ecap_mask in iommu_nesting_info.
> >>> *) reuse DOMAIN_ATTR_NESTING to get nesting info.
> >>> *) return an empty iommu_nesting_info for SMMU drivers per Jean'
> >>> suggestion.
> >>> ---
> >>> include/uapi/linux/iommu.h | 74
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 74 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/iommu.h
> >>> b/include/uapi/linux/iommu.h index 7c8e075..5e4745a 100644
> >>> --- a/include/uapi/linux/iommu.h
> >>> +++ b/include/uapi/linux/iommu.h
> >>> @@ -332,4 +332,78 @@ struct iommu_gpasid_bind_data {
> >>> } vendor;
> >>> };
> >>>
> >>> +/*
> >>> + * struct iommu_nesting_info - Information for nesting-capable
> >>> IOMMU.
> >>> + * userspace should check it
> >>> before using
> >>> + * nesting capability.
> >>> + *
> >>> + * @argsz: size of the whole structure.
> >>> + * @flags: currently reserved for future extension. must
> >>> set to 0.
> >>> + * @format: PASID table entry format, the same definition
> >>> as struct
> >>> + * iommu_gpasid_bind_data @format.
> >>> + * @features: supported nesting features.
> >>> + * @addr_width: The output addr width of first
> >>> level/stage translation
> >>> + * @pasid_bits: Maximum supported PASID bits, 0
> >>> represents no PASID
> >>> + * support.
> >>> + * @data: vendor specific cap info. data[] structure type
> >>> can be deduced
> >>> + * from @format field.
> >>> + *
> >>> + *
> >> +===============+===================================================
> >> ===+
> >>> + * | feature |
> >>> Notes |
> >>> + *
> >> +===============+===================================================
> >> ===+
> >>> + * | SYSWIDE_PASID | IOMMU vendor driver sets it to mandate
> >>> userspace |
> >>> + * | | to allocate PASID from kernel. All PASID
> >>> allocation |
> >>> + * | | free must be mediated through the TBD
> >>> API. |
> >> s/TBD/IOMMU
> >
> > got it.
> >
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + * | BIND_PGTBL | IOMMU vendor driver sets it to mandate
> >>> userspace |
> >>> + * | | bind the first level/stage page table to
> >>> associated |
> >> s/bind/to bind
> >
> > got it.
> >
> >>> + * | | PASID (either the one specified in bind
> >>> request or |
> >>> + * | | the default PASID of iommu domain),
> >>> through IOMMU |
> >>> + * | |
> >>> UAPI. |
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + * | CACHE_INVLD | IOMMU vendor driver sets it to mandate
> >>> userspace |
> >>
> >>> + * | | explicitly invalidate the IOMMU cache
> >>> through IOMMU |
> >> to explicitly
> >
> > I see.
> >
> >>> + * | | U
> >>> API according to vendor-specific requirement when |
> >>> + * | | changing the 1st level/stage page
> >>> table. |
> >>> + *
> >>> +---------------+------------------------------------------------------+
> >>> + *
> >>> + * @data[] types defined for @format:
> >>> + *
> >> +================================+==================================
> >> ===+
> >>> + * | @format |
> >>> @data[] |
> >>> + *
> >> +================================+==================================
> >> ===+
> >>> + * | IOMMU_PASID_FORMAT_INTEL_VTD | struct
> >>> iommu_nesting_info_vtd |
> >>> + *
> >>> +--------------------------------+-------------------------------------+
> >>> + *
> >>> + */
> >>> +struct iommu_nesting_info {
> >>> + __u32 argsz;
> >>> + __u32 flags;
> >>> + __u32 format;
> >>> +#define IOMMU_NESTING_FEAT_SYSWIDE_PASID (1 << 0)
> >>> +#define IOMMU_NESTING_FEAT_BIND_PGTBL (1 << 1)
> >>> +#define IOMMU_NESTING_FEAT_CACHE_INVLD (1 << 2)
> >>> + __u32 features;
> >>> + __u16 addr_width;
> >>> + __u16 pasid_bits;
> >>> + __u32 padding;
> >>> + __u8 data[];
> >>> +};
> >> As opposed to other IOMMU UAPI structs there is no union member at
> >> the end.
> >
> > nice catch. do you think it would be better to adding a union and
> > put the struct iommu_nesting_info_vtd in it?
> Yes I think so. At least it would be consistent with the rest of the
> API and with the guidelines.
> >
> >> Also this struct is not documented in [PATCH v7 1/7] docs: IOMMU
> >> user API. Shouldn't we align.
> >> You may also consider to move this patch in Jacob's series for
> >> consistency, thoughts?
> >
> > this was talked one time between Jacob and me. It was put in this
> > series as the major user of nesting_info is in this series. e.g.
> > vfio checks the SYSWIDE_PASID. but I'm open to merge it with Jacob's
> > series if it would make the merge easier.
> Yep I think it would make sense to move in Jacob's series to have a
> general understanding of the uapi
>
I a little reluctant to include this in my UAPI set, the reason is that
there are two dimensions IOMMU UAPI are extended:
1. Define the protocols in interaction with VFIO, sanity checking, and
backward compatibility.
2. Adding more UAPI data structures that are parallel to the existing
ones.

My patchset is to address #1, this patch is for #2. My thinking is that
once we have reached consensus on #1, new UAPI structures such as this
patch can just follow the suit.

If that is OK with you, I would like to keep them separate to avoid
diverging conversations.

Thanks,

Jacob

> Thanks
>
> Eric
> >
> > Thanks,
> > Yi Liu
> >
> >>> +
> >>> +/*
> >>> + * struct iommu_nesting_info_vtd - Intel VT-d specific nesting
> >>> info.
> >>> + *
> >>> + * @flags: VT-d specific flags. Currently reserved for
> >>> future
> >>> + * extension. must be set to 0.
> >>> + * @cap_reg: Describe basic capabilities as defined in
> >>> VT-d capability
> >>> + * register.
> >>> + * @ecap_reg: Describe the extended capabilities as
> >>> defined in VT-d
> >>> + * extended capability register.
> >>> + */
> >>> +struct iommu_nesting_info_vtd {
> >>> + __u32 flags;
> >>> + __u32 padding;
> >>> + __u64 cap_reg;
> >>> + __u64 ecap_reg;
> >>> +};
> >>> +
> >>> #endif /* _UAPI_IOMMU_H */
> >>>
> >>
> >> Thanks
> >>
> >> Eric
> >>
> >
>

[Jacob Pan]

Next message: Martin KaFai Lau: "Re: [PATCH bpf-next v8 6/7] bpf: Allow local storage to be used from LSM programs"
Previous message: Brooke Basile: "[PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning"
In reply to: Auger Eric: "Re: [PATCH v6 02/15] iommu: Report domain nesting info"
Next in thread: Auger Eric: "Re: [PATCH v6 02/15] iommu: Report domain nesting info"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]