Messages in this thread | | | Subject | Re: [PATCH 3/3] iommu/uapi: Add helper function for size lookup | From | Auger Eric <> | Date | Thu, 6 Feb 2020 11:14:00 +0100 |
| |
Hi Jacob, On 2/3/20 11:41 PM, Jacob Pan wrote: > On Mon, 3 Feb 2020 14:12:36 -0700 > Alex Williamson <alex.williamson@redhat.com> wrote: > >> On Mon, 3 Feb 2020 12:41:43 -0800 >> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote: >> >>> On Mon, 3 Feb 2020 11:27:08 -0700 >>> Alex Williamson <alex.williamson@redhat.com> wrote: >>> >>>> On Fri, 31 Jan 2020 15:51:25 -0800 >>>> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote: >>>> >>>>> Hi Alex, >>>>> Sorry I missed this part in the previous reply. Comments below. >>>>> >>>>> On Wed, 29 Jan 2020 15:19:51 -0700 >>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>> >>>>>> Also, is the 12-bytes of padding in struct >>>>>> iommu_gpasid_bind_data excessive with this new versioning >>>>>> scheme? Per rule #2 I'm not sure if we're allowed to >>>>>> repurpose those padding bytes, >>>>> We can still use the padding bytes as long as there is a new >>>>> flag bit to indicate the validity of the new filed within the >>>>> padding. I should have made it clear in rule #2 when mentioning >>>>> the flags bits. Should define what extension constitutes. >>>>> How about this? >>>>> " >>>>> * 2. Data structures are open to extension but closed to >>>>> modification. >>>>> * Extension should leverage the padding bytes first where a >>>>> new >>>>> * flag bit is required to indicate the validity of each new >>>>> member. >>>>> * The above rule for padding bytes also applies to adding >>>>> new union >>>>> * members. >>>>> * After padding bytes are exhausted, new fields must be >>>>> added at the >>>>> * end of each data structure with 64bit alignment. Flag bits >>>>> can be >>>>> * added without size change but existing ones cannot be >>>>> altered. * >>>>> " >>>>> So if we add new field by doing re-purpose of padding bytes, >>>>> size lookup result will remain the same. New code would >>>>> recognize the new flag, old code stays the same. >>>>> >>>>> VFIO layer checks for UAPI compatibility and size to copy, >>>>> version sanity check and flag usage are done in the IOMMU code. >>>>> >>>>>> but if we add >>>>>> fields to the end of the structure as the scheme suggests, >>>>>> we're stuck with not being able to expand the union for new >>>>>> fields. >>>>> Good point, it does sound contradictory. I hope the rewritten >>>>> rule #2 address that. >>>>> Adding data after the union should be extremely rare. Do you >>>>> see any issues with the example below? >>>>> >>>>> offsetofend() can still find the right size. >>>>> e.g. >>>>> V1 >>>>> struct iommu_gpasid_bind_data { >>>>> __u32 version; >>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD 1 >>>>> __u32 format; >>>>> #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID >>>>> valid */ __u64 flags; >>>>> __u64 gpgd; >>>>> __u64 hpasid; >>>>> __u64 gpasid; >>>>> __u32 addr_width; >>>>> __u8 padding[12]; >>>>> /* Vendor specific data */ >>>>> union { >>>>> struct iommu_gpasid_bind_data_vtd vtd; >>>>> }; >>>>> }; >>>>> >>>>> const static int >>>>> iommu_uapi_data_size[NR_IOMMU_UAPI_TYPE][IOMMU_UAPI_VERSION] = >>>>> { /* IOMMU_UAPI_BIND_GPASID */ {offsetofend(struct >>>>> iommu_gpasid_bind_data, vtd)}, ... >>>>> }; >>>>> >>>>> V2, Add new_member at the end (forget padding for now). >>>>> struct iommu_gpasid_bind_data { >>>>> __u32 version; >>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD 1 >>>>> __u32 format; >>>>> #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID >>>>> valid */ #define IOMMU_NEW_MEMBER_VAL (1 << 1) /* new >>>>> member added */ __u64 flags; >>>>> __u64 gpgd; >>>>> __u64 hpasid; >>>>> __u64 gpasid; >>>>> __u32 addr_width; >>>>> __u8 padding[12]; >>>>> /* Vendor specific data */ >>>>> union { >>>>> struct iommu_gpasid_bind_data_vtd vtd; >>>>> }; >>>>> __u64 new_member; >>>>> }; >>>>> const static int >>>>> iommu_uapi_data_size[NR_IOMMU_UAPI_TYPE][IOMMU_UAPI_VERSION] = >>>>> { /* IOMMU_UAPI_BIND_GPASID */ >>>>> {offsetofend(struct iommu_gpasid_bind_data, >>>>> vtd), offsetofend(struct >>>>> iommu_gpasid_bind_data,new_member)}, >>>>> >>>>> }; >>>>> >>>>> V3, Add smmu to the union,larger than vtd >>>>> >>>>> struct iommu_gpasid_bind_data { >>>>> __u32 version; >>>>> #define IOMMU_PASID_FORMAT_INTEL_VTD 1 >>>>> #define IOMMU_PASID_FORMAT_INTEL_SMMU 2 >>>>> __u32 format; >>>>> #define IOMMU_SVA_GPASID_VAL (1 << 0) /* guest PASID >>>>> valid */ #define IOMMU_NEW_MEMBER_VAL (1 << 1) /* new >>>>> member added */ #define IOMMU_SVA_SMMU_SUPP (1 << 2) /* >>>>> SMMU data supported */ __u64 flags; >>>>> __u64 gpgd; >>>>> __u64 hpasid; >>>>> __u64 gpasid; >>>>> __u32 addr_width; >>>>> __u8 padding[12]; >>>>> /* Vendor specific data */ >>>>> union { >>>>> struct iommu_gpasid_bind_data_vtd vtd; >>>>> struct iommu_gpasid_bind_data_smmu smmu; >>>>> }; >>>>> __u64 new_member; >>>>> }; >>>>> const static int >>>>> iommu_uapi_data_size[NR_IOMMU_UAPI_TYPE][IOMMU_UAPI_VERSION] = { >>>>> /* IOMMU_UAPI_BIND_GPASID */ >>>>> {offsetofend(struct iommu_gpasid_bind_data,vtd), >>>>> offsetofend(struct iommu_gpasid_bind_data, new_member), >>>>> offsetofend(struct iommu_gpasid_bind_data, new_member)}, >>>>> ... >>>>> }; >>>>> >>>> >>>> How are you not breaking rule #3, "Versions are backward >>>> compatible" with this? If the kernel is at version 3 and >>>> userspace is at version 2 then new_member exists at different >>>> offsets of the structure. The kernels iommu_uapi_data_size for >>>> V2 changed between version 2 and 3. Thanks, >>>> >>> You are right. if we want to add new member to the end of the >>> structure as well as expanding union, I think we have to fix the >>> size of the union. Would this work? (just an example for one struct) >>> >>> >>> @@ -344,6 +348,11 @@ struct iommu_gpasid_bind_data_vtd { >>> * @gpasid: Process address space ID used for the guest mm in >>> guest IOMMU >>> * @addr_width: Guest virtual address width >>> * @padding: Reserved for future use (should be zero) >>> + * @dummy Reserve space for vendor specific data in the >>> union. New >>> + * members added to the union cannot exceed the size of >>> dummy. >>> + * The fixed size union is needed to allow further >>> expansion >>> + * after the end of the union while still maintain >>> backward >>> + * compatibility. >>> * @vtd: Intel VT-d specific data >>> * >>> * Guest to host PASID mapping can be an identity or non-identity, >>> where guest @@ -365,6 +374,7 @@ struct iommu_gpasid_bind_data { >>> __u8 padding[12]; >>> /* Vendor specific data */ >>> union { >>> + __u8 dummy[128]; >>> struct iommu_gpasid_bind_data_vtd vtd; >>> }; >>> }; >> >> It's not the most space efficient thing and we're just guessing at >> what might need to be added into that union in future, but it >> works... until it doesn't ;) One might also argue that we could >> inflate the padding field even further to serve the same purpose. > That was our initial intention, the padding field is already inflated > to accommodate any foreseeable extensions :) > > Extensions beyond union was deemed unlikely that is why we use the > union at the end. > > The intention of this patchset is not to change that, but rather > clarify and simplify the version checking. > >> The only other route I can think of would be to let the user specify >> the offset of the variable size data from the start of the structure, >> for example similar to how we're laying out vfio migration region or >> how we do capabilities in vfio ioctls. This is where passing an >> argsz for each ioctl comes in handy so we're not limited to a >> structure, we can link various structures together in a chain. Of >> course that requires work on both the user and kernel side to pack >> and unpack, but it leaves a lot of flexibility in extending it. >> Thanks, >> > Yeah, that would work as well. I just feel IOMMU UAPI is unlikely to get > updated frequently, should be much less than adding new capabilities. > I think argsz could be viewed as the version field set by the > user, minsz is what kernel current code supports. > > So let me summarize the options we have > 1. Disallow adding new members to each structure other than reuse > padding bits or adding union members at the end. > 2. Allow extension of the structures beyond union, but union size has > to be fixed with reserved spaces > 3. Adopt VFIO argsz scheme, I don't think we need version for each > struct anymore. argsz implies the version that user is using assuming > UAPI data is extension only. > > Jean, Eric, any comments? My preference is #1. In the apocalyptic event > when we run out of padding, perhaps we can introduce a new API_v2 :) I had #1 in mind too.
Thanks
Eric > >> Alex >> > > [Jacob Pan] >
| |