On Thu, Sep 21, 2023 at 08:12:03PM +0800, Baolu Lu wrote:
> On 2023/9/21 15:51, Yi Liu wrote:
> > diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> > index 4a7c5c8fdbb4..3c8660fe9bb1 100644
> > --- a/include/uapi/linux/iommufd.h
> > +++ b/include/uapi/linux/iommufd.h
> > @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
> > IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
> > };
> > +/**
> > + * enum iommu_hwpt_type - IOMMU HWPT Type
> > + * @IOMMU_HWPT_TYPE_DEFAULT: default
>
> How about s/default/vendor agnostic/ ?
Please don't use the word vendor :)
IOMMU_HWPT_TYPE_GENERIC perhaps if we don't like default
Jason
On 2023-09-21 17:44, Jason Gunthorpe wrote:
> On Thu, Sep 21, 2023 at 08:12:03PM +0800, Baolu Lu wrote:
>> On 2023/9/21 15:51, Yi Liu wrote:
>>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>>> index 4a7c5c8fdbb4..3c8660fe9bb1 100644
>>> --- a/include/uapi/linux/iommufd.h
>>> +++ b/include/uapi/linux/iommufd.h
>>> @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
>>> IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>> };
>>> +/**
>>> + * enum iommu_hwpt_type - IOMMU HWPT Type
>>> + * @IOMMU_HWPT_TYPE_DEFAULT: default
>>
>> How about s/default/vendor agnostic/ ?
>
> Please don't use the word vendor :)
>
> IOMMU_HWPT_TYPE_GENERIC perhaps if we don't like default
Ah yes, a default domain type, not to be confused with any default
domain type, including the default default domain type. Just in case
anyone had forgotten how gleefully fun this is :D
I particularly like the bit where we end up with this construct later:
switch (hwpt_type) {
case IOMMU_HWPT_TYPE_DEFAULT:
/* allocate a domain */
default:
/* allocate a different domain */
}
But of course neither case allocates a *default* domain, because it's
quite obviously the wrong place to be doing that.
I could go on enjoying myself, but basically yeah, "default" can't be a
type in itself (at best it would be a meta-type which could be
requested, such that it resolves to some real type to actually
allocate), so a good name should reflect what the type functionally
*means* to the user. IIUC the important distinction is that it's an
abstract kernel-owned pagetable for the user to indirectly control via
the API, rather than one it owns and writes directly (and thus has to be
in a specific agreed format).
Thanks,
Robin.
> From: Robin Murphy <[email protected]>
> Sent: Friday, September 22, 2023 5:48 PM
>
> I could go on enjoying myself, but basically yeah, "default" can't be a
> type in itself (at best it would be a meta-type which could be
> requested, such that it resolves to some real type to actually
> allocate), so a good name should reflect what the type functionally
> *means* to the user. IIUC the important distinction is that it's an
> abstract kernel-owned pagetable for the user to indirectly control via
> the API, rather than one it owns and writes directly (and thus has to be
> in a specific agreed format).
>
IOMMU_HWPT_TYPE_KERNEL then?
IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.
On 2023/9/22 17:47, Robin Murphy wrote:
> On 2023-09-21 17:44, Jason Gunthorpe wrote:
>> On Thu, Sep 21, 2023 at 08:12:03PM +0800, Baolu Lu wrote:
>>> On 2023/9/21 15:51, Yi Liu wrote:
>>>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>>>> index 4a7c5c8fdbb4..3c8660fe9bb1 100644
>>>> --- a/include/uapi/linux/iommufd.h
>>>> +++ b/include/uapi/linux/iommufd.h
>>>> @@ -357,6 +357,14 @@ enum iommufd_hwpt_alloc_flags {
>>>> IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>>> };
>>>> +/**
>>>> + * enum iommu_hwpt_type - IOMMU HWPT Type
>>>> + * @IOMMU_HWPT_TYPE_DEFAULT: default
>>>
>>> How about s/default/vendor agnostic/ ?
>>
>> Please don't use the word vendor :)
>>
>> IOMMU_HWPT_TYPE_GENERIC perhaps if we don't like default
>
> Ah yes, a default domain type, not to be confused with any default domain
> type, including the default default domain type. Just in case anyone had
> forgotten how gleefully fun this is :D
>
> I particularly like the bit where we end up with this construct later:
>
> switch (hwpt_type) {
> case IOMMU_HWPT_TYPE_DEFAULT:
> /* allocate a domain */
> default:
> /* allocate a different domain */
> }
>
> But of course neither case allocates a *default* domain, because it's quite
> obviously the wrong place to be doing that.
>
> I could go on enjoying myself, but basically yeah, "default" can't be a
> type in itself (at best it would be a meta-type which could be requested,
> such that it resolves to some real type to actually allocate), so a good
> name should reflect what the type functionally *means* to the user. IIUC
> the important distinction is that it's an abstract kernel-owned pagetable
> for the user to indirectly control via the API, rather than one it owns and
> writes directly (and thus has to be in a specific agreed format).
yes. It is just what the existing domain_alloc_user op does. Here we add
a hwpt_type as the type can be given by user, so we need to define a
specific type for it.
Perhaps we can also name it as IOMMU_HWPT_TYPE_UNMANAGED to be aligned with
the domain type naming. IOMMU_HWPT_TYPE_GENERIC is also a good choice.
Please feel free let me know your preference.
--
Regards,
Yi Liu
On Tue, Sep 26, 2023 at 06:37:55AM +0000, Tian, Kevin wrote:
> > From: Robin Murphy <[email protected]>
> > Sent: Friday, September 22, 2023 5:48 PM
> >
> > I could go on enjoying myself, but basically yeah, "default" can't be a
> > type in itself (at best it would be a meta-type which could be
> > requested, such that it resolves to some real type to actually
> > allocate), so a good name should reflect what the type functionally
> > *means* to the user. IIUC the important distinction is that it's an
> > abstract kernel-owned pagetable for the user to indirectly control via
> > the API, rather than one it owns and writes directly (and thus has to be
> > in a specific agreed format).
> >
>
> IOMMU_HWPT_TYPE_KERNEL then?
>
> IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.
At the end of the day this enum is the type tag for:
struct iommu_hwpt_alloc {
__u32 size;
__u32 pt_id;
__u32 out_hwpt_id;
__u32 __reserved;
+ __u32 hwpt_type;
+ __u32 data_len;
+ __aligned_u64 data_uptr;
};
That pointer.
IOMMU_HWPT_ALLOC_DATA_NONE = 0
IOMMU_HWPT_ALLOC_DATA_INTEL_VTD
IOMMU_HWPT_ALLOC_DATA_ARM_SMMUV3
etc?
DATA_NONE requires data_len == 0
Jason
> From: Jason Gunthorpe <[email protected]>
> Sent: Wednesday, September 27, 2023 12:29 AM
>
> On Tue, Sep 26, 2023 at 06:37:55AM +0000, Tian, Kevin wrote:
> > > From: Robin Murphy <[email protected]>
> > > Sent: Friday, September 22, 2023 5:48 PM
> > >
> > > I could go on enjoying myself, but basically yeah, "default" can't be a
> > > type in itself (at best it would be a meta-type which could be
> > > requested, such that it resolves to some real type to actually
> > > allocate), so a good name should reflect what the type functionally
> > > *means* to the user. IIUC the important distinction is that it's an
> > > abstract kernel-owned pagetable for the user to indirectly control via
> > > the API, rather than one it owns and writes directly (and thus has to be
> > > in a specific agreed format).
> > >
> >
> > IOMMU_HWPT_TYPE_KERNEL then?
> >
> > IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.
>
> At the end of the day this enum is the type tag for:
>
> struct iommu_hwpt_alloc {
> __u32 size;
> __u32 pt_id;
> __u32 out_hwpt_id;
> __u32 __reserved;
> + __u32 hwpt_type;
> + __u32 data_len;
> + __aligned_u64 data_uptr;
> };
>
> That pointer.
>
> IOMMU_HWPT_ALLOC_DATA_NONE = 0
> IOMMU_HWPT_ALLOC_DATA_INTEL_VTD
> IOMMU_HWPT_ALLOC_DATA_ARM_SMMUV3
>
> etc?
>
> DATA_NONE requires data_len == 0
>
Looks good. Probably hwpt_type can be also renamed to data_type
to better match this interpretation.
On 2023/9/27 09:08, Tian, Kevin wrote:
>> From: Jason Gunthorpe <[email protected]>
>> Sent: Wednesday, September 27, 2023 12:29 AM
>>
>> On Tue, Sep 26, 2023 at 06:37:55AM +0000, Tian, Kevin wrote:
>>>> From: Robin Murphy <[email protected]>
>>>> Sent: Friday, September 22, 2023 5:48 PM
>>>>
>>>> I could go on enjoying myself, but basically yeah, "default" can't be a
>>>> type in itself (at best it would be a meta-type which could be
>>>> requested, such that it resolves to some real type to actually
>>>> allocate), so a good name should reflect what the type functionally
>>>> *means* to the user. IIUC the important distinction is that it's an
>>>> abstract kernel-owned pagetable for the user to indirectly control via
>>>> the API, rather than one it owns and writes directly (and thus has to be
>>>> in a specific agreed format).
>>>>
>>>
>>> IOMMU_HWPT_TYPE_KERNEL then?
>>>
>>> IOMMU_HWPT_TYPE_GENERIC also doesn't sound a straight word.
>>
>> At the end of the day this enum is the type tag for:
>>
>> struct iommu_hwpt_alloc {
>> __u32 size;
>> __u32 pt_id;
>> __u32 out_hwpt_id;
>> __u32 __reserved;
>> + __u32 hwpt_type;
>> + __u32 data_len;
>> + __aligned_u64 data_uptr;
>> };
>>
>> That pointer.
>>
>> IOMMU_HWPT_ALLOC_DATA_NONE = 0
>> IOMMU_HWPT_ALLOC_DATA_INTEL_VTD
>> IOMMU_HWPT_ALLOC_DATA_ARM_SMMUV3
>>
>> etc?
>>
>> DATA_NONE requires data_len == 0
>>
>
> Looks good. Probably hwpt_type can be also renamed to data_type
> to better match this interpretation.
ack.
--
Regards,
Yi Liu