Hi Jacob, On 9/9/20 12:40 AM, Jacob Pan wrote: > On Tue, 1 Sep 2020 17:38:44 +0200 > Auger Eric <eric.au...@redhat.com> wrote: > >> Hi Jacob, >> On 8/22/20 6:35 AM, Jacob Pan wrote: >>> When an IOASID set is used for guest SVA, each VM will acquire its >>> ioasid_set for IOASID allocations. IOASIDs within the VM must have a >>> host/physical IOASID backing, mapping between guest and host >>> IOASIDs can be non-identical. IOASID set private ID (SPID) is >>> introduced in this patch to be used as guest IOASID. However, the >>> concept of ioasid_set specific namespace is generic, thus named >>> SPID. >>> >>> As SPID namespace is within the IOASID set, the IOASID core can >>> provide lookup services at both directions. SPIDs may not be >>> allocated when its IOASID is allocated, the mapping between SPID >>> and IOASID is usually established when a guest page table is bound >>> to a host PASID. >>> >>> Signed-off-by: Jacob Pan <jacob.jun....@linux.intel.com> >>> --- >>> drivers/iommu/ioasid.c | 54 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/ioasid.h | 12 +++++++++++ 2 files changed, 66 >>> insertions(+) >>> >>> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c >>> index 5f31d63c75b1..c0aef38a4fde 100644 >>> --- a/drivers/iommu/ioasid.c >>> +++ b/drivers/iommu/ioasid.c >>> @@ -21,6 +21,7 @@ enum ioasid_state { >>> * struct ioasid_data - Meta data about ioasid >>> * >>> * @id: Unique ID >>> + * @spid: Private ID unique within a set >>> * @users Number of active users >>> * @state Track state of the IOASID >>> * @set Meta data of the set this IOASID belongs to >>> @@ -29,6 +30,7 @@ enum ioasid_state { >>> */ >>> struct ioasid_data { >>> ioasid_t id; >>> + ioasid_t spid; >>> struct ioasid_set *set; >>> refcount_t users; >>> enum ioasid_state state; >>> @@ -326,6 +328,58 @@ int ioasid_attach_data(ioasid_t ioasid, void >>> *data) EXPORT_SYMBOL_GPL(ioasid_attach_data); >>> >>> /** >>> + * ioasid_attach_spid - Attach ioasid_set private ID to an IOASID >>> + * >>> + * @ioasid: the ID to attach >>> + * @spid: the ioasid_set private ID of @ioasid >>> + * >>> + * For IOASID that is already allocated, private ID within the set >>> can be >>> + * attached via this API. Future lookup can be done via >>> ioasid_find. >> I would remove "For IOASID that is already allocated, private ID >> within the set can be attached via this API" > I guess it is implied. Will remove. > >>> + */ >>> +int ioasid_attach_spid(ioasid_t ioasid, ioasid_t spid) >>> +{ >>> + struct ioasid_data *ioasid_data; >>> + int ret = 0; >>> + >>> + spin_lock(&ioasid_allocator_lock); >> We keep on saying the SPID is local to an IOASID set but we don't >> check any IOASID set contains this ioasid. It looks a bit weird to me. > We store ioasid_set inside ioasid_data when an IOASID is allocated, so > we don't need to search all the ioasid_sets. Perhaps I missed your > point? No I think it became clearer ;-) > >>> + ioasid_data = xa_load(&active_allocator->xa, ioasid); >>> + >>> + if (!ioasid_data) { >>> + pr_err("No IOASID entry %d to attach SPID %d\n", >>> + ioasid, spid); >>> + ret = -ENOENT; >>> + goto done_unlock; >>> + } >>> + ioasid_data->spid = spid; >> is there any way/need to remove an spid binding? > For guest SVA, we attach SPID as a guest PASID during bind guest page > table. Unbind does the opposite, ioasid_attach_spid() with > spid=INVALID_IOASID clears the binding. > > Perhaps add more symmetric functions? i.e. > ioasid_detach_spid(ioasid_t ioasid) > ioasid_attach_spid(struct ioasid_set *set, ioasid_t ioasid) yep make sense
Thanks Eric > >>> + >>> +done_unlock: >>> + spin_unlock(&ioasid_allocator_lock); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(ioasid_attach_spid); >>> + >>> +ioasid_t ioasid_find_by_spid(struct ioasid_set *set, ioasid_t spid) >>> +{ >>> + struct ioasid_data *entry; >>> + unsigned long index; >>> + >>> + if (!xa_load(&ioasid_sets, set->sid)) { >>> + pr_warn("Invalid set\n"); >>> + return INVALID_IOASID; >>> + } >>> + >>> + xa_for_each(&set->xa, index, entry) { >>> + if (spid == entry->spid) { >>> + pr_debug("Found ioasid %lu by spid %u\n", >>> index, spid); >>> + refcount_inc(&entry->users); >>> + return index; >>> + } >>> + } >>> + return INVALID_IOASID; >>> +} >>> +EXPORT_SYMBOL_GPL(ioasid_find_by_spid); >>> + >>> +/** >>> * ioasid_alloc - Allocate an IOASID >>> * @set: the IOASID set >>> * @min: the minimum ID (inclusive) >>> diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h >>> index 310abe4187a3..d4b3e83672f6 100644 >>> --- a/include/linux/ioasid.h >>> +++ b/include/linux/ioasid.h >>> @@ -73,6 +73,8 @@ bool ioasid_is_active(ioasid_t ioasid); >>> >>> void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid, bool >>> (*getter)(void *)); int ioasid_attach_data(ioasid_t ioasid, void >>> *data); +int ioasid_attach_spid(ioasid_t ioasid, ioasid_t spid); >>> +ioasid_t ioasid_find_by_spid(struct ioasid_set *set, ioasid_t >>> spid); int ioasid_register_allocator(struct ioasid_allocator_ops >>> *allocator); void ioasid_unregister_allocator(struct >>> ioasid_allocator_ops *allocator); void ioasid_is_in_set(struct >>> ioasid_set *set, ioasid_t ioasid); @@ -136,5 +138,15 @@ static >>> inline int ioasid_attach_data(ioasid_t ioasid, void *data) return >>> -ENOTSUPP; } >>> >>> +staic inline int ioasid_attach_spid(ioasid_t ioasid, ioasid_t spid) >>> +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> +static inline ioasid_t ioasid_find_by_spid(struct ioasid_set *set, >>> ioasid_t spid) +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> #endif /* CONFIG_IOASID */ >>> #endif /* __LINUX_IOASID_H */ >>> >> Thanks >> >> Eric >> >> _______________________________________________ >> iommu mailing list >> iommu@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu > > [Jacob Pan] > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu