IOASID users fit into the publisher-subscriber pattern, a system wide
blocking notifier chain can be used to inform subscribers of state
changes. Notifier mechanism also abstracts publisher from knowing the
private context each subcriber may have.
This patch adds APIs and a global notifier chain, a further optimization
might be per set notifier for ioasid_set aware users.
Usage example:
KVM register notifier block such that it can keep its guest-host PASID
translation table in sync with any IOASID updates.
VFIO publish IOASID change by performing alloc/free, bind/unbind
operations.
IOMMU driver gets notified when IOASID is freed by VFIO or core mm code
such that PASID context can be cleaned up.
Signed-off-by: Liu Yi L <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
---
drivers/iommu/ioasid.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/ioasid.h | 40 +++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
index 8612fe6477dc..27dce2cb5af2 100644
--- a/drivers/iommu/ioasid.c
+++ b/drivers/iommu/ioasid.c
@@ -11,6 +11,22 @@
#include <linux/xarray.h>
static DEFINE_XARRAY_ALLOC(ioasid_sets);
+/*
+ * An IOASID could have multiple consumers. When a status change occurs,
+ * this notifier chain is used to keep them in sync. Each consumer of the
+ * IOASID service must register notifier block early to ensure no events
+ * are missed.
+ *
+ * This is a publisher-subscriber pattern where publisher can change the
+ * state of each IOASID, e.g. alloc/free, bind IOASID to a device and mm.
+ * On the other hand, subscribers gets notified for the state change and
+ * keep local states in sync.
+ *
+ * Currently, the notifier is global. A further optimization could be per
+ * IOASID set notifier chain.
+ */
+static BLOCKING_NOTIFIER_HEAD(ioasid_chain);
+
/**
* struct ioasid_set_data - Meta data about ioasid_set
*
@@ -408,6 +424,7 @@ static void ioasid_free_locked(ioasid_t ioasid)
{
struct ioasid_data *ioasid_data;
struct ioasid_set_data *sdata;
+ struct ioasid_nb_args args;
ioasid_data = xa_load(&active_allocator->xa, ioasid);
if (!ioasid_data) {
@@ -415,6 +432,13 @@ static void ioasid_free_locked(ioasid_t ioasid)
return;
}
+ args.id = ioasid;
+ args.sid = ioasid_data->sdata->sid;
+ args.pdata = ioasid_data->private;
+ args.set_token = ioasid_data->sdata->token;
+
+ /* Notify all users that this IOASID is being freed */
+ blocking_notifier_call_chain(&ioasid_chain, IOASID_FREE, &args);
active_allocator->ops->free(ioasid, active_allocator->ops->pdata);
/* Custom allocator needs additional steps to free the xa element */
if (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) {
@@ -624,6 +648,43 @@ int ioasid_find_sid(ioasid_t ioasid)
}
EXPORT_SYMBOL_GPL(ioasid_find_sid);
+int ioasid_add_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&ioasid_chain, nb);
+}
+EXPORT_SYMBOL_GPL(ioasid_add_notifier);
+
+void ioasid_remove_notifier(struct notifier_block *nb)
+{
+ blocking_notifier_chain_unregister(&ioasid_chain, nb);
+}
+EXPORT_SYMBOL_GPL(ioasid_remove_notifier);
+
+int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
+{
+ struct ioasid_data *ioasid_data;
+ struct ioasid_nb_args args;
+ int ret = 0;
+
+ mutex_lock(&ioasid_allocator_lock);
+ ioasid_data = xa_load(&active_allocator->xa, ioasid);
+ if (!ioasid_data) {
+ pr_err("Trying to free unknown IOASID %u\n", ioasid);
+ mutex_unlock(&ioasid_allocator_lock);
+ return -EINVAL;
+ }
+
+ args.id = ioasid;
+ args.sid = ioasid_data->sdata->sid;
+ args.pdata = ioasid_data->private;
+
+ ret = blocking_notifier_call_chain(&ioasid_chain, cmd, &args);
+ mutex_unlock(&ioasid_allocator_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ioasid_notify);
+
MODULE_AUTHOR("Jean-Philippe Brucker <[email protected]>");
MODULE_AUTHOR("Jacob Pan <[email protected]>");
MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
index e19c0ad93bd7..32d032913828 100644
--- a/include/linux/ioasid.h
+++ b/include/linux/ioasid.h
@@ -4,6 +4,7 @@
#include <linux/types.h>
#include <linux/errno.h>
+#include <linux/notifier.h>
#define INVALID_IOASID ((ioasid_t)-1)
#define INVALID_IOASID_SET (-1)
@@ -30,6 +31,27 @@ struct ioasid_allocator_ops {
void *pdata;
};
+/* Notification data when IOASID status changed */
+enum ioasid_notify_val {
+ IOASID_ALLOC = 1,
+ IOASID_FREE,
+ IOASID_BIND,
+ IOASID_UNBIND,
+};
+
+/**
+ * struct ioasid_nb_args - Argument provided by IOASID core when notifier
+ * is called.
+ * @id: the IOASID being notified
+ * @sid: the IOASID set @id belongs to
+ * @pdata: the private data attached to the IOASID
+ */
+struct ioasid_nb_args {
+ ioasid_t id;
+ int sid;
+ struct ioasid_set *set_token;
+ void *pdata;
+};
/* Shared IOASID set for reserved for host system use */
extern int system_ioasid_sid;
@@ -43,11 +65,15 @@ void *ioasid_find(int sid, ioasid_t ioasid, bool (*getter)(void *));
int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
int ioasid_attach_data(ioasid_t ioasid, void *data);
+int ioasid_add_notifier(struct notifier_block *nb);
+void ioasid_remove_notifier(struct notifier_block *nb);
void ioasid_install_capacity(ioasid_t total);
int ioasid_alloc_system_set(int quota);
int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid);
void ioasid_free_set(int sid, bool destroy_set);
int ioasid_find_sid(ioasid_t ioasid);
+int ioasid_notify(ioasid_t id, enum ioasid_notify_val cmd);
+
#else /* !CONFIG_IOASID */
static inline ioasid_t ioasid_alloc(int sid, ioasid_t min,
ioasid_t max, void *private)
@@ -73,6 +99,20 @@ static inline void *ioasid_find(int sid, ioasid_t ioasid, bool (*getter)(void *)
return NULL;
}
+static inline int ioasid_add_notifier(struct notifier_block *nb)
+{
+ return -ENOTSUPP;
+}
+
+static inline void ioasid_remove_notifier(struct notifier_block *nb)
+{
+}
+
+int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
+{
+ return -ENOTSUPP;
+}
+
static inline int ioasid_register_allocator(struct ioasid_allocator_ops *allocator)
{
return -ENOTSUPP;
--
2.7.4
> From: Jacob Pan <[email protected]>
> Sent: Thursday, March 26, 2020 1:55 AM
>
> IOASID users fit into the publisher-subscriber pattern, a system wide
> blocking notifier chain can be used to inform subscribers of state
> changes. Notifier mechanism also abstracts publisher from knowing the
> private context each subcriber may have.
>
> This patch adds APIs and a global notifier chain, a further optimization
> might be per set notifier for ioasid_set aware users.
>
> Usage example:
> KVM register notifier block such that it can keep its guest-host PASID
> translation table in sync with any IOASID updates.
>
> VFIO publish IOASID change by performing alloc/free, bind/unbind
> operations.
>
> IOMMU driver gets notified when IOASID is freed by VFIO or core mm code
> such that PASID context can be cleaned up.
above example looks mixed. You have KVM registers the notifier but
finally having IOMMU driver to get notified... ????
>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
> ---
> drivers/iommu/ioasid.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/ioasid.h | 40 +++++++++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index 8612fe6477dc..27dce2cb5af2 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -11,6 +11,22 @@
> #include <linux/xarray.h>
>
> static DEFINE_XARRAY_ALLOC(ioasid_sets);
> +/*
> + * An IOASID could have multiple consumers. When a status change occurs,
> + * this notifier chain is used to keep them in sync. Each consumer of the
> + * IOASID service must register notifier block early to ensure no events
> + * are missed.
> + *
> + * This is a publisher-subscriber pattern where publisher can change the
> + * state of each IOASID, e.g. alloc/free, bind IOASID to a device and mm.
> + * On the other hand, subscribers gets notified for the state change and
> + * keep local states in sync.
> + *
> + * Currently, the notifier is global. A further optimization could be per
> + * IOASID set notifier chain.
> + */
> +static BLOCKING_NOTIFIER_HEAD(ioasid_chain);
> +
> /**
> * struct ioasid_set_data - Meta data about ioasid_set
> *
> @@ -408,6 +424,7 @@ static void ioasid_free_locked(ioasid_t ioasid)
> {
> struct ioasid_data *ioasid_data;
> struct ioasid_set_data *sdata;
> + struct ioasid_nb_args args;
>
> ioasid_data = xa_load(&active_allocator->xa, ioasid);
> if (!ioasid_data) {
> @@ -415,6 +432,13 @@ static void ioasid_free_locked(ioasid_t ioasid)
> return;
> }
>
> + args.id = ioasid;
> + args.sid = ioasid_data->sdata->sid;
> + args.pdata = ioasid_data->private;
> + args.set_token = ioasid_data->sdata->token;
> +
> + /* Notify all users that this IOASID is being freed */
> + blocking_notifier_call_chain(&ioasid_chain, IOASID_FREE, &args);
> active_allocator->ops->free(ioasid, active_allocator->ops->pdata);
> /* Custom allocator needs additional steps to free the xa element */
> if (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) {
> @@ -624,6 +648,43 @@ int ioasid_find_sid(ioasid_t ioasid)
> }
> EXPORT_SYMBOL_GPL(ioasid_find_sid);
>
> +int ioasid_add_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&ioasid_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_add_notifier);
> +
> +void ioasid_remove_notifier(struct notifier_block *nb)
> +{
> + blocking_notifier_chain_unregister(&ioasid_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_remove_notifier);
register/unregister
> +
> +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
add a comment on when this function should be used?
> +{
> + struct ioasid_data *ioasid_data;
> + struct ioasid_nb_args args;
> + int ret = 0;
> +
> + mutex_lock(&ioasid_allocator_lock);
> + ioasid_data = xa_load(&active_allocator->xa, ioasid);
> + if (!ioasid_data) {
> + pr_err("Trying to free unknown IOASID %u\n", ioasid);
why is it fixed to 'free'?
> + mutex_unlock(&ioasid_allocator_lock);
> + return -EINVAL;
> + }
> +
> + args.id = ioasid;
> + args.sid = ioasid_data->sdata->sid;
> + args.pdata = ioasid_data->private;
why no token info as did in ioasid_free?
> +
> + ret = blocking_notifier_call_chain(&ioasid_chain, cmd, &args);
> + mutex_unlock(&ioasid_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_notify);
> +
> MODULE_AUTHOR("Jean-Philippe Brucker <jean-
> [email protected]>");
> MODULE_AUTHOR("Jacob Pan <[email protected]>");
> MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
> diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> index e19c0ad93bd7..32d032913828 100644
> --- a/include/linux/ioasid.h
> +++ b/include/linux/ioasid.h
> @@ -4,6 +4,7 @@
>
> #include <linux/types.h>
> #include <linux/errno.h>
> +#include <linux/notifier.h>
>
> #define INVALID_IOASID ((ioasid_t)-1)
> #define INVALID_IOASID_SET (-1)
> @@ -30,6 +31,27 @@ struct ioasid_allocator_ops {
> void *pdata;
> };
>
> +/* Notification data when IOASID status changed */
> +enum ioasid_notify_val {
> + IOASID_ALLOC = 1,
> + IOASID_FREE,
> + IOASID_BIND,
> + IOASID_UNBIND,
> +};
Curious why IOASID_ALLOC is not notified aumatically within ioasid_alloc
similar to ioasid_free, while leaving to the publisher? BIND/UNBIND is
a publisher thing but a bit strange to see ALLOC/FREE with different policy here.
> +
> +/**
> + * struct ioasid_nb_args - Argument provided by IOASID core when notifier
> + * is called.
> + * @id: the IOASID being notified
> + * @sid: the IOASID set @id belongs to
> + * @pdata: the private data attached to the IOASID
> + */
> +struct ioasid_nb_args {
> + ioasid_t id;
> + int sid;
> + struct ioasid_set *set_token;
> + void *pdata;
> +};
> /* Shared IOASID set for reserved for host system use */
> extern int system_ioasid_sid;
>
> @@ -43,11 +65,15 @@ void *ioasid_find(int sid, ioasid_t ioasid, bool
> (*getter)(void *));
> int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
> void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
> int ioasid_attach_data(ioasid_t ioasid, void *data);
> +int ioasid_add_notifier(struct notifier_block *nb);
> +void ioasid_remove_notifier(struct notifier_block *nb);
> void ioasid_install_capacity(ioasid_t total);
> int ioasid_alloc_system_set(int quota);
> int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int *sid);
> void ioasid_free_set(int sid, bool destroy_set);
> int ioasid_find_sid(ioasid_t ioasid);
> +int ioasid_notify(ioasid_t id, enum ioasid_notify_val cmd);
> +
> #else /* !CONFIG_IOASID */
> static inline ioasid_t ioasid_alloc(int sid, ioasid_t min,
> ioasid_t max, void *private)
> @@ -73,6 +99,20 @@ static inline void *ioasid_find(int sid, ioasid_t ioasid,
> bool (*getter)(void *)
> return NULL;
> }
>
> +static inline int ioasid_add_notifier(struct notifier_block *nb)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline void ioasid_remove_notifier(struct notifier_block *nb)
> +{
> +}
> +
> +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
> +{
> + return -ENOTSUPP;
> +}
> +
> static inline int ioasid_register_allocator(struct ioasid_allocator_ops
> *allocator)
> {
> return -ENOTSUPP;
> --
> 2.7.4
On Fri, 27 Mar 2020 10:03:26 +0000
"Tian, Kevin" <[email protected]> wrote:
> > From: Jacob Pan <[email protected]>
> > Sent: Thursday, March 26, 2020 1:55 AM
> >
> > IOASID users fit into the publisher-subscriber pattern, a system
> > wide blocking notifier chain can be used to inform subscribers of
> > state changes. Notifier mechanism also abstracts publisher from
> > knowing the private context each subcriber may have.
> >
> > This patch adds APIs and a global notifier chain, a further
> > optimization might be per set notifier for ioasid_set aware users.
> >
> > Usage example:
> > KVM register notifier block such that it can keep its guest-host
> > PASID translation table in sync with any IOASID updates.
> >
> > VFIO publish IOASID change by performing alloc/free, bind/unbind
> > operations.
> >
> > IOMMU driver gets notified when IOASID is freed by VFIO or core mm
> > code such that PASID context can be cleaned up.
>
> above example looks mixed. You have KVM registers the notifier but
> finally having IOMMU driver to get notified... ????
>
Right, felt like a tale of two subscribers got mixed. I meant to list a
few use cases with publisher and subscriber roles separate.
I will change that to "Usage examples", and explicit state each role.
> >
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
> > ---
> > drivers/iommu/ioasid.c | 61
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/ioasid.h | 40 +++++++++++++++++++++++++++++++++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index 8612fe6477dc..27dce2cb5af2 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -11,6 +11,22 @@
> > #include <linux/xarray.h>
> >
> > static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/*
> > + * An IOASID could have multiple consumers. When a status change
> > occurs,
> > + * this notifier chain is used to keep them in sync. Each consumer
> > of the
> > + * IOASID service must register notifier block early to ensure no
> > events
> > + * are missed.
> > + *
> > + * This is a publisher-subscriber pattern where publisher can
> > change the
> > + * state of each IOASID, e.g. alloc/free, bind IOASID to a device
> > and mm.
> > + * On the other hand, subscribers gets notified for the state
> > change and
> > + * keep local states in sync.
> > + *
> > + * Currently, the notifier is global. A further optimization could
> > be per
> > + * IOASID set notifier chain.
> > + */
> > +static BLOCKING_NOTIFIER_HEAD(ioasid_chain);
> > +
> > /**
> > * struct ioasid_set_data - Meta data about ioasid_set
> > *
> > @@ -408,6 +424,7 @@ static void ioasid_free_locked(ioasid_t ioasid)
> > {
> > struct ioasid_data *ioasid_data;
> > struct ioasid_set_data *sdata;
> > + struct ioasid_nb_args args;
> >
> > ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > if (!ioasid_data) {
> > @@ -415,6 +432,13 @@ static void ioasid_free_locked(ioasid_t ioasid)
> > return;
> > }
> >
> > + args.id = ioasid;
> > + args.sid = ioasid_data->sdata->sid;
> > + args.pdata = ioasid_data->private;
> > + args.set_token = ioasid_data->sdata->token;
> > +
> > + /* Notify all users that this IOASID is being freed */
> > + blocking_notifier_call_chain(&ioasid_chain, IOASID_FREE,
> > &args); active_allocator->ops->free(ioasid,
> > active_allocator->ops->pdata); /* Custom allocator needs additional
> > steps to free the xa element */ if (active_allocator->flags &
> > IOASID_ALLOCATOR_CUSTOM) { @@ -624,6 +648,43 @@ int
> > ioasid_find_sid(ioasid_t ioasid) }
> > EXPORT_SYMBOL_GPL(ioasid_find_sid);
> >
> > +int ioasid_add_notifier(struct notifier_block *nb)
> > +{
> > + return blocking_notifier_chain_register(&ioasid_chain, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_add_notifier);
> > +
> > +void ioasid_remove_notifier(struct notifier_block *nb)
> > +{
> > + blocking_notifier_chain_unregister(&ioasid_chain, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_remove_notifier);
>
> register/unregister
>
Sounds good.
> > +
> > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
>
> add a comment on when this function should be used?
>
Sure, how about:
/**
* ioasid_notify - Send notification on a given IOASID for status change.
* Used by publishers when the status change may affect
* subscriber's internal state.
*
* @ioasid: The IOASID to which the notification will send
* @cmd: The notification event
*
*/
> > +{
> > + struct ioasid_data *ioasid_data;
> > + struct ioasid_nb_args args;
> > + int ret = 0;
> > +
> > + mutex_lock(&ioasid_allocator_lock);
> > + ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > + if (!ioasid_data) {
> > + pr_err("Trying to free unknown IOASID %u\n",
> > ioasid);
>
> why is it fixed to 'free'?
>
Good catch, it shouldn;t be just free. It was a relic of early test
case.
> > + mutex_unlock(&ioasid_allocator_lock);
> > + return -EINVAL;
> > + }
> > +
> > + args.id = ioasid;
> > + args.sid = ioasid_data->sdata->sid;
> > + args.pdata = ioasid_data->private;
>
> why no token info as did in ioasid_free?
>
Good catch, should include token as well. It is better to include all
the data such that subscribers don't have to do any lookup which may
cause race.
> > +
> > + ret = blocking_notifier_call_chain(&ioasid_chain, cmd,
> > &args);
> > + mutex_unlock(&ioasid_allocator_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_notify);
> > +
> > MODULE_AUTHOR("Jean-Philippe Brucker <jean-
> > [email protected]>");
> > MODULE_AUTHOR("Jacob Pan <[email protected]>");
> > MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
> > diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> > index e19c0ad93bd7..32d032913828 100644
> > --- a/include/linux/ioasid.h
> > +++ b/include/linux/ioasid.h
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/errno.h>
> > +#include <linux/notifier.h>
> >
> > #define INVALID_IOASID ((ioasid_t)-1)
> > #define INVALID_IOASID_SET (-1)
> > @@ -30,6 +31,27 @@ struct ioasid_allocator_ops {
> > void *pdata;
> > };
> >
> > +/* Notification data when IOASID status changed */
> > +enum ioasid_notify_val {
> > + IOASID_ALLOC = 1,
> > + IOASID_FREE,
> > + IOASID_BIND,
> > + IOASID_UNBIND,
> > +};
>
> Curious why IOASID_ALLOC is not notified aumatically within
> ioasid_alloc similar to ioasid_free, while leaving to the publisher?
> BIND/UNBIND is a publisher thing but a bit strange to see ALLOC/FREE
> with different policy here.
>
I don't see a use case for ALLOC notification yet. Any user does the
allocation would the be the first and only one know about this IOASID.
Unless we have set level notifier, which may be interested in a new
IOASID being allocated within the set.
> > +
> > +/**
> > + * struct ioasid_nb_args - Argument provided by IOASID core when
> > notifier
> > + * is called.
> > + * @id: the IOASID being notified
> > + * @sid: the IOASID set @id belongs to
> > + * @pdata: the private data attached to the IOASID
> > + */
> > +struct ioasid_nb_args {
> > + ioasid_t id;
> > + int sid;
> > + struct ioasid_set *set_token;
> > + void *pdata;
> > +};
> > /* Shared IOASID set for reserved for host system use */
> > extern int system_ioasid_sid;
> >
> > @@ -43,11 +65,15 @@ void *ioasid_find(int sid, ioasid_t ioasid, bool
> > (*getter)(void *));
> > int ioasid_register_allocator(struct ioasid_allocator_ops
> > *allocator); void ioasid_unregister_allocator(struct
> > ioasid_allocator_ops *allocator); int ioasid_attach_data(ioasid_t
> > ioasid, void *data); +int ioasid_add_notifier(struct notifier_block
> > *nb); +void ioasid_remove_notifier(struct notifier_block *nb);
> > void ioasid_install_capacity(ioasid_t total);
> > int ioasid_alloc_system_set(int quota);
> > int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > *sid); void ioasid_free_set(int sid, bool destroy_set);
> > int ioasid_find_sid(ioasid_t ioasid);
> > +int ioasid_notify(ioasid_t id, enum ioasid_notify_val cmd);
> > +
> > #else /* !CONFIG_IOASID */
> > static inline ioasid_t ioasid_alloc(int sid, ioasid_t min,
> > ioasid_t max, void *private)
> > @@ -73,6 +99,20 @@ static inline void *ioasid_find(int sid,
> > ioasid_t ioasid, bool (*getter)(void *)
> > return NULL;
> > }
> >
> > +static inline int ioasid_add_notifier(struct notifier_block *nb)
> > +{
> > + return -ENOTSUPP;
> > +}
> > +
> > +static inline void ioasid_remove_notifier(struct notifier_block
> > *nb) +{
> > +}
> > +
> > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
> > +{
> > + return -ENOTSUPP;
> > +}
> > +
> > static inline int ioasid_register_allocator(struct
> > ioasid_allocator_ops *allocator)
> > {
> > return -ENOTSUPP;
> > --
> > 2.7.4
>
[Jacob Pan]
> From: Jacob Pan <[email protected]>
> Sent: Saturday, March 28, 2020 2:37 AM
>
> On Fri, 27 Mar 2020 10:03:26 +0000
> "Tian, Kevin" <[email protected]> wrote:
>
> > > From: Jacob Pan <[email protected]>
> > > Sent: Thursday, March 26, 2020 1:55 AM
> > >
> > > IOASID users fit into the publisher-subscriber pattern, a system
> > > wide blocking notifier chain can be used to inform subscribers of
> > > state changes. Notifier mechanism also abstracts publisher from
> > > knowing the private context each subcriber may have.
> > >
> > > This patch adds APIs and a global notifier chain, a further
> > > optimization might be per set notifier for ioasid_set aware users.
> > >
> > > Usage example:
> > > KVM register notifier block such that it can keep its guest-host
> > > PASID translation table in sync with any IOASID updates.
> > >
> > > VFIO publish IOASID change by performing alloc/free, bind/unbind
> > > operations.
> > >
> > > IOMMU driver gets notified when IOASID is freed by VFIO or core mm
> > > code such that PASID context can be cleaned up.
> >
> > above example looks mixed. You have KVM registers the notifier but
> > finally having IOMMU driver to get notified... ????
> >
> Right, felt like a tale of two subscribers got mixed. I meant to list a
> few use cases with publisher and subscriber roles separate.
> I will change that to "Usage examples", and explicit state each role.
>
> > >
> > > Signed-off-by: Liu Yi L <[email protected]>
> > > Signed-off-by: Jacob Pan <[email protected]>
> > > ---
> > > drivers/iommu/ioasid.c | 61
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/ioasid.h | 40 +++++++++++++++++++++++++++++++++
> > > 2 files changed, 101 insertions(+)
> > >
> > > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > > index 8612fe6477dc..27dce2cb5af2 100644
> > > --- a/drivers/iommu/ioasid.c
> > > +++ b/drivers/iommu/ioasid.c
> > > @@ -11,6 +11,22 @@
> > > #include <linux/xarray.h>
> > >
> > > static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > > +/*
> > > + * An IOASID could have multiple consumers. When a status change
> > > occurs,
> > > + * this notifier chain is used to keep them in sync. Each consumer
> > > of the
> > > + * IOASID service must register notifier block early to ensure no
> > > events
> > > + * are missed.
> > > + *
> > > + * This is a publisher-subscriber pattern where publisher can
> > > change the
> > > + * state of each IOASID, e.g. alloc/free, bind IOASID to a device
> > > and mm.
> > > + * On the other hand, subscribers gets notified for the state
> > > change and
> > > + * keep local states in sync.
> > > + *
> > > + * Currently, the notifier is global. A further optimization could
> > > be per
> > > + * IOASID set notifier chain.
> > > + */
> > > +static BLOCKING_NOTIFIER_HEAD(ioasid_chain);
> > > +
> > > /**
> > > * struct ioasid_set_data - Meta data about ioasid_set
> > > *
> > > @@ -408,6 +424,7 @@ static void ioasid_free_locked(ioasid_t ioasid)
> > > {
> > > struct ioasid_data *ioasid_data;
> > > struct ioasid_set_data *sdata;
> > > + struct ioasid_nb_args args;
> > >
> > > ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > > if (!ioasid_data) {
> > > @@ -415,6 +432,13 @@ static void ioasid_free_locked(ioasid_t ioasid)
> > > return;
> > > }
> > >
> > > + args.id = ioasid;
> > > + args.sid = ioasid_data->sdata->sid;
> > > + args.pdata = ioasid_data->private;
> > > + args.set_token = ioasid_data->sdata->token;
> > > +
> > > + /* Notify all users that this IOASID is being freed */
> > > + blocking_notifier_call_chain(&ioasid_chain, IOASID_FREE,
> > > &args); active_allocator->ops->free(ioasid,
> > > active_allocator->ops->pdata); /* Custom allocator needs additional
> > > steps to free the xa element */ if (active_allocator->flags &
> > > IOASID_ALLOCATOR_CUSTOM) { @@ -624,6 +648,43 @@ int
> > > ioasid_find_sid(ioasid_t ioasid) }
> > > EXPORT_SYMBOL_GPL(ioasid_find_sid);
> > >
> > > +int ioasid_add_notifier(struct notifier_block *nb)
> > > +{
> > > + return blocking_notifier_chain_register(&ioasid_chain, nb);
> > > +}
> > > +EXPORT_SYMBOL_GPL(ioasid_add_notifier);
> > > +
> > > +void ioasid_remove_notifier(struct notifier_block *nb)
> > > +{
> > > + blocking_notifier_chain_unregister(&ioasid_chain, nb);
> > > +}
> > > +EXPORT_SYMBOL_GPL(ioasid_remove_notifier);
> >
> > register/unregister
> >
> Sounds good.
>
> > > +
> > > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
> >
> > add a comment on when this function should be used?
> >
> Sure, how about:
> /**
> * ioasid_notify - Send notification on a given IOASID for status change.
> * Used by publishers when the status change may affect
> * subscriber's internal state.
> *
> * @ioasid: The IOASID to which the notification will send
> * @cmd: The notification event
> *
> */
looks good.
>
> > > +{
> > > + struct ioasid_data *ioasid_data;
> > > + struct ioasid_nb_args args;
> > > + int ret = 0;
> > > +
> > > + mutex_lock(&ioasid_allocator_lock);
> > > + ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > > + if (!ioasid_data) {
> > > + pr_err("Trying to free unknown IOASID %u\n",
> > > ioasid);
> >
> > why is it fixed to 'free'?
> >
> Good catch, it shouldn;t be just free. It was a relic of early test
> case.
>
> > > + mutex_unlock(&ioasid_allocator_lock);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + args.id = ioasid;
> > > + args.sid = ioasid_data->sdata->sid;
> > > + args.pdata = ioasid_data->private;
> >
> > why no token info as did in ioasid_free?
> >
> Good catch, should include token as well. It is better to include all
> the data such that subscribers don't have to do any lookup which may
> cause race.
>
> > > +
> > > + ret = blocking_notifier_call_chain(&ioasid_chain, cmd,
> > > &args);
> > > + mutex_unlock(&ioasid_allocator_lock);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(ioasid_notify);
> > > +
> > > MODULE_AUTHOR("Jean-Philippe Brucker <jean-
> > > [email protected]>");
> > > MODULE_AUTHOR("Jacob Pan <[email protected]>");
> > > MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
> > > diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> > > index e19c0ad93bd7..32d032913828 100644
> > > --- a/include/linux/ioasid.h
> > > +++ b/include/linux/ioasid.h
> > > @@ -4,6 +4,7 @@
> > >
> > > #include <linux/types.h>
> > > #include <linux/errno.h>
> > > +#include <linux/notifier.h>
> > >
> > > #define INVALID_IOASID ((ioasid_t)-1)
> > > #define INVALID_IOASID_SET (-1)
> > > @@ -30,6 +31,27 @@ struct ioasid_allocator_ops {
> > > void *pdata;
> > > };
> > >
> > > +/* Notification data when IOASID status changed */
> > > +enum ioasid_notify_val {
> > > + IOASID_ALLOC = 1,
> > > + IOASID_FREE,
> > > + IOASID_BIND,
> > > + IOASID_UNBIND,
> > > +};
> >
> > Curious why IOASID_ALLOC is not notified aumatically within
> > ioasid_alloc similar to ioasid_free, while leaving to the publisher?
> > BIND/UNBIND is a publisher thing but a bit strange to see ALLOC/FREE
> > with different policy here.
> >
> I don't see a use case for ALLOC notification yet. Any user does the
> allocation would the be the first and only one know about this IOASID.
>
> Unless we have set level notifier, which may be interested in a new
> IOASID being allocated within the set.
then remove this type until it is actually required when supporting
set-level notifier?
>
> > > +
> > > +/**
> > > + * struct ioasid_nb_args - Argument provided by IOASID core when
> > > notifier
> > > + * is called.
> > > + * @id: the IOASID being notified
> > > + * @sid: the IOASID set @id belongs to
> > > + * @pdata: the private data attached to the IOASID
> > > + */
> > > +struct ioasid_nb_args {
> > > + ioasid_t id;
> > > + int sid;
> > > + struct ioasid_set *set_token;
> > > + void *pdata;
> > > +};
> > > /* Shared IOASID set for reserved for host system use */
> > > extern int system_ioasid_sid;
> > >
> > > @@ -43,11 +65,15 @@ void *ioasid_find(int sid, ioasid_t ioasid, bool
> > > (*getter)(void *));
> > > int ioasid_register_allocator(struct ioasid_allocator_ops
> > > *allocator); void ioasid_unregister_allocator(struct
> > > ioasid_allocator_ops *allocator); int ioasid_attach_data(ioasid_t
> > > ioasid, void *data); +int ioasid_add_notifier(struct notifier_block
> > > *nb); +void ioasid_remove_notifier(struct notifier_block *nb);
> > > void ioasid_install_capacity(ioasid_t total);
> > > int ioasid_alloc_system_set(int quota);
> > > int ioasid_alloc_set(struct ioasid_set *token, ioasid_t quota, int
> > > *sid); void ioasid_free_set(int sid, bool destroy_set);
> > > int ioasid_find_sid(ioasid_t ioasid);
> > > +int ioasid_notify(ioasid_t id, enum ioasid_notify_val cmd);
> > > +
> > > #else /* !CONFIG_IOASID */
> > > static inline ioasid_t ioasid_alloc(int sid, ioasid_t min,
> > > ioasid_t max, void *private)
> > > @@ -73,6 +99,20 @@ static inline void *ioasid_find(int sid,
> > > ioasid_t ioasid, bool (*getter)(void *)
> > > return NULL;
> > > }
> > >
> > > +static inline int ioasid_add_notifier(struct notifier_block *nb)
> > > +{
> > > + return -ENOTSUPP;
> > > +}
> > > +
> > > +static inline void ioasid_remove_notifier(struct notifier_block
> > > *nb) +{
> > > +}
> > > +
> > > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
> > > +{
> > > + return -ENOTSUPP;
> > > +}
> > > +
> > > static inline int ioasid_register_allocator(struct
> > > ioasid_allocator_ops *allocator)
> > > {
> > > return -ENOTSUPP;
> > > --
> > > 2.7.4
> >
>
> [Jacob Pan]
On Sat, 28 Mar 2020 06:43:37 +0000
"Tian, Kevin" <[email protected]> wrote:
> > From: Jacob Pan <[email protected]>
> > Sent: Saturday, March 28, 2020 2:37 AM
> >
> > On Fri, 27 Mar 2020 10:03:26 +0000
> > "Tian, Kevin" <[email protected]> wrote:
> >
> > > > From: Jacob Pan <[email protected]>
> > > > Sent: Thursday, March 26, 2020 1:55 AM
> > > >
> > > > IOASID users fit into the publisher-subscriber pattern, a system
> > > > wide blocking notifier chain can be used to inform subscribers
> > > > of state changes. Notifier mechanism also abstracts publisher
> > > > from knowing the private context each subcriber may have.
> > > >
> > > > This patch adds APIs and a global notifier chain, a further
> > > > optimization might be per set notifier for ioasid_set aware
> > > > users.
> > > >
> > > > Usage example:
> > > > KVM register notifier block such that it can keep its guest-host
> > > > PASID translation table in sync with any IOASID updates.
> > > >
> > > > VFIO publish IOASID change by performing alloc/free, bind/unbind
> > > > operations.
> > > >
> > > > IOMMU driver gets notified when IOASID is freed by VFIO or core
> > > > mm code such that PASID context can be cleaned up.
> > >
> > > above example looks mixed. You have KVM registers the notifier but
> > > finally having IOMMU driver to get notified... ????
> > >
> > Right, felt like a tale of two subscribers got mixed. I meant to
> > list a few use cases with publisher and subscriber roles separate.
> > I will change that to "Usage examples", and explicit state each
> > role.
> > > >
> > > > Signed-off-by: Liu Yi L <[email protected]>
> > > > Signed-off-by: Jacob Pan <[email protected]>
> > > > ---
> > > > drivers/iommu/ioasid.c | 61
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/ioasid.h | 40 +++++++++++++++++++++++++++++++++
> > > > 2 files changed, 101 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > > > index 8612fe6477dc..27dce2cb5af2 100644
> > > > --- a/drivers/iommu/ioasid.c
> > > > +++ b/drivers/iommu/ioasid.c
> > > > @@ -11,6 +11,22 @@
> > > > #include <linux/xarray.h>
> > > >
> > > > static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > > > +/*
> > > > + * An IOASID could have multiple consumers. When a status
> > > > change occurs,
> > > > + * this notifier chain is used to keep them in sync. Each
> > > > consumer of the
> > > > + * IOASID service must register notifier block early to ensure
> > > > no events
> > > > + * are missed.
> > > > + *
> > > > + * This is a publisher-subscriber pattern where publisher can
> > > > change the
> > > > + * state of each IOASID, e.g. alloc/free, bind IOASID to a
> > > > device and mm.
> > > > + * On the other hand, subscribers gets notified for the state
> > > > change and
> > > > + * keep local states in sync.
> > > > + *
> > > > + * Currently, the notifier is global. A further optimization
> > > > could be per
> > > > + * IOASID set notifier chain.
> > > > + */
> > > > +static BLOCKING_NOTIFIER_HEAD(ioasid_chain);
> > > > +
> > > > /**
> > > > * struct ioasid_set_data - Meta data about ioasid_set
> > > > *
> > > > @@ -408,6 +424,7 @@ static void ioasid_free_locked(ioasid_t
> > > > ioasid) {
> > > > struct ioasid_data *ioasid_data;
> > > > struct ioasid_set_data *sdata;
> > > > + struct ioasid_nb_args args;
> > > >
> > > > ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > > > if (!ioasid_data) {
> > > > @@ -415,6 +432,13 @@ static void ioasid_free_locked(ioasid_t
> > > > ioasid) return;
> > > > }
> > > >
> > > > + args.id = ioasid;
> > > > + args.sid = ioasid_data->sdata->sid;
> > > > + args.pdata = ioasid_data->private;
> > > > + args.set_token = ioasid_data->sdata->token;
> > > > +
> > > > + /* Notify all users that this IOASID is being freed */
> > > > + blocking_notifier_call_chain(&ioasid_chain,
> > > > IOASID_FREE, &args); active_allocator->ops->free(ioasid,
> > > > active_allocator->ops->pdata); /* Custom allocator needs
> > > > additional steps to free the xa element */ if
> > > > (active_allocator->flags & IOASID_ALLOCATOR_CUSTOM) { @@ -624,6
> > > > +648,43 @@ int ioasid_find_sid(ioasid_t ioasid) }
> > > > EXPORT_SYMBOL_GPL(ioasid_find_sid);
> > > >
> > > > +int ioasid_add_notifier(struct notifier_block *nb)
> > > > +{
> > > > + return blocking_notifier_chain_register(&ioasid_chain,
> > > > nb); +}
> > > > +EXPORT_SYMBOL_GPL(ioasid_add_notifier);
> > > > +
> > > > +void ioasid_remove_notifier(struct notifier_block *nb)
> > > > +{
> > > > + blocking_notifier_chain_unregister(&ioasid_chain, nb);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(ioasid_remove_notifier);
> > >
> > > register/unregister
> > >
> > Sounds good.
> >
> > > > +
> > > > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val
> > > > cmd)
> > >
> > > add a comment on when this function should be used?
> > >
> > Sure, how about:
> > /**
> > * ioasid_notify - Send notification on a given IOASID for status
> > change.
> > * Used by publishers when the status change may
> > affect
> > * subscriber's internal state.
> > *
> > * @ioasid: The IOASID to which the notification will send
> > * @cmd: The notification event
> > *
> > */
>
> looks good.
>
> >
> > > > +{
> > > > + struct ioasid_data *ioasid_data;
> > > > + struct ioasid_nb_args args;
> > > > + int ret = 0;
> > > > +
> > > > + mutex_lock(&ioasid_allocator_lock);
> > > > + ioasid_data = xa_load(&active_allocator->xa, ioasid);
> > > > + if (!ioasid_data) {
> > > > + pr_err("Trying to free unknown IOASID %u\n",
> > > > ioasid);
> > >
> > > why is it fixed to 'free'?
> > >
> > Good catch, it shouldn;t be just free. It was a relic of early test
> > case.
> >
> > > > + mutex_unlock(&ioasid_allocator_lock);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + args.id = ioasid;
> > > > + args.sid = ioasid_data->sdata->sid;
> > > > + args.pdata = ioasid_data->private;
> > >
> > > why no token info as did in ioasid_free?
> > >
> > Good catch, should include token as well. It is better to include
> > all the data such that subscribers don't have to do any lookup
> > which may cause race.
> >
> > > > +
> > > > + ret = blocking_notifier_call_chain(&ioasid_chain, cmd,
> > > > &args);
> > > > + mutex_unlock(&ioasid_allocator_lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(ioasid_notify);
> > > > +
> > > > MODULE_AUTHOR("Jean-Philippe Brucker <jean-
> > > > [email protected]>");
> > > > MODULE_AUTHOR("Jacob Pan <[email protected]>");
> > > > MODULE_DESCRIPTION("IO Address Space ID (IOASID) allocator");
> > > > diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> > > > index e19c0ad93bd7..32d032913828 100644
> > > > --- a/include/linux/ioasid.h
> > > > +++ b/include/linux/ioasid.h
> > > > @@ -4,6 +4,7 @@
> > > >
> > > > #include <linux/types.h>
> > > > #include <linux/errno.h>
> > > > +#include <linux/notifier.h>
> > > >
> > > > #define INVALID_IOASID ((ioasid_t)-1)
> > > > #define INVALID_IOASID_SET (-1)
> > > > @@ -30,6 +31,27 @@ struct ioasid_allocator_ops {
> > > > void *pdata;
> > > > };
> > > >
> > > > +/* Notification data when IOASID status changed */
> > > > +enum ioasid_notify_val {
> > > > + IOASID_ALLOC = 1,
> > > > + IOASID_FREE,
> > > > + IOASID_BIND,
> > > > + IOASID_UNBIND,
> > > > +};
> > >
> > > Curious why IOASID_ALLOC is not notified aumatically within
> > > ioasid_alloc similar to ioasid_free, while leaving to the
> > > publisher? BIND/UNBIND is a publisher thing but a bit strange to
> > > see ALLOC/FREE with different policy here.
> > >
> > I don't see a use case for ALLOC notification yet. Any user does the
> > allocation would the be the first and only one know about this
> > IOASID.
> >
> > Unless we have set level notifier, which may be interested in a new
> > IOASID being allocated within the set.
>
> then remove this type until it is actually required when supporting
> set-level notifier?
>
Sounds good. I will change it to:
/*
* Notification event when IOASID status changed. Note that there is no
* event for ALLOC in that the allocator is the only one knows about the
* new IOASID.
*/
enum ioasid_notify_val {
IOASID_FREE = 1,
IOASID_BIND,
IOASID_UNBIND,
};
> >
> > > > +
> > > > +/**
> > > > + * struct ioasid_nb_args - Argument provided by IOASID core
> > > > when notifier
> > > > + * is called.
> > > > + * @id: the IOASID being notified
> > > > + * @sid: the IOASID set @id belongs to
> > > > + * @pdata: the private data attached to the IOASID
> > > > + */
> > > > +struct ioasid_nb_args {
> > > > + ioasid_t id;
> > > > + int sid;
> > > > + struct ioasid_set *set_token;
> > > > + void *pdata;
> > > > +};
> > > > /* Shared IOASID set for reserved for host system use */
> > > > extern int system_ioasid_sid;
> > > >
> > > > @@ -43,11 +65,15 @@ void *ioasid_find(int sid, ioasid_t ioasid,
> > > > bool (*getter)(void *));
> > > > int ioasid_register_allocator(struct ioasid_allocator_ops
> > > > *allocator); void ioasid_unregister_allocator(struct
> > > > ioasid_allocator_ops *allocator); int
> > > > ioasid_attach_data(ioasid_t ioasid, void *data); +int
> > > > ioasid_add_notifier(struct notifier_block *nb); +void
> > > > ioasid_remove_notifier(struct notifier_block *nb); void
> > > > ioasid_install_capacity(ioasid_t total); int
> > > > ioasid_alloc_system_set(int quota); int ioasid_alloc_set(struct
> > > > ioasid_set *token, ioasid_t quota, int *sid); void
> > > > ioasid_free_set(int sid, bool destroy_set); int
> > > > ioasid_find_sid(ioasid_t ioasid); +int ioasid_notify(ioasid_t
> > > > id, enum ioasid_notify_val cmd); +
> > > > #else /* !CONFIG_IOASID */
> > > > static inline ioasid_t ioasid_alloc(int sid, ioasid_t min,
> > > > ioasid_t max, void
> > > > *private) @@ -73,6 +99,20 @@ static inline void
> > > > *ioasid_find(int sid, ioasid_t ioasid, bool (*getter)(void *)
> > > > return NULL;
> > > > }
> > > >
> > > > +static inline int ioasid_add_notifier(struct notifier_block
> > > > *nb) +{
> > > > + return -ENOTSUPP;
> > > > +}
> > > > +
> > > > +static inline void ioasid_remove_notifier(struct notifier_block
> > > > *nb) +{
> > > > +}
> > > > +
> > > > +int ioasid_notify(ioasid_t ioasid, enum ioasid_notify_val cmd)
> > > > +{
> > > > + return -ENOTSUPP;
> > > > +}
> > > > +
> > > > static inline int ioasid_register_allocator(struct
> > > > ioasid_allocator_ops *allocator)
> > > > {
> > > > return -ENOTSUPP;
> > > > --
> > > > 2.7.4
> > >
> >
> > [Jacob Pan]
[Jacob Pan]
On Wed, Mar 25, 2020 at 10:55:29AM -0700, Jacob Pan wrote:
> IOASID users fit into the publisher-subscriber pattern, a system wide
> blocking notifier chain can be used to inform subscribers of state
> changes. Notifier mechanism also abstracts publisher from knowing the
> private context each subcriber may have.
>
> This patch adds APIs and a global notifier chain, a further optimization
> might be per set notifier for ioasid_set aware users.
>
> Usage example:
> KVM register notifier block such that it can keep its guest-host PASID
> translation table in sync with any IOASID updates.
When you talk about KVM, is it for
[PATCH 0/7] x86: tag application address space for devices
or something else as well? (I don't see mentions of KVM in that series)
>
> VFIO publish IOASID change by performing alloc/free, bind/unbind
> operations.
I was rather seeing IOASID as the end of the VFIO-IOMMU-IOASID chain,
putting it in the middle complicates locking. If you only need to FREE
notifier for this calse, maybe VFIO could talk directly to the IOMMU
driver before freeing an IOASID? gpasid_unbind() should already clear the
PASID contexts, no?
Thanks,
Jean
> IOMMU driver gets notified when IOASID is freed by VFIO or core mm code
> such that PASID context can be cleaned up.
>
> Signed-off-by: Liu Yi L <[email protected]>
> Signed-off-by: Jacob Pan <[email protected]>
On Wed, 1 Apr 2020 16:00:06 +0200
Jean-Philippe Brucker <[email protected]> wrote:
> On Wed, Mar 25, 2020 at 10:55:29AM -0700, Jacob Pan wrote:
> > IOASID users fit into the publisher-subscriber pattern, a system
> > wide blocking notifier chain can be used to inform subscribers of
> > state changes. Notifier mechanism also abstracts publisher from
> > knowing the private context each subcriber may have.
> >
> > This patch adds APIs and a global notifier chain, a further
> > optimization might be per set notifier for ioasid_set aware users.
> >
> > Usage example:
> > KVM register notifier block such that it can keep its guest-host
> > PASID translation table in sync with any IOASID updates.
>
> When you talk about KVM, is it for
>
> [PATCH 0/7] x86: tag application address space for devices
>
> or something else as well? (I don't see mentions of KVM in that
> series)
>
Yes, related to this set. This is set is for native ENQCMD support.
VMCS use of IOASID notifier is for the guest SVA + ENQCMD.
We need to maintain a G-H PASID translation in VMCS PASID translation
table. When guest binds a GPASID to a host PASID, this translation
table can be updated such that subsequent ENQCMD in the guest can
resolve to a host PASID.
CH 7.3.1 of DSA spec.
https://software.intel.com/sites/default/files/341204-intel-data-streaming-accelerator-spec.pdf
> >
> > VFIO publish IOASID change by performing alloc/free, bind/unbind
> > operations.
>
> I was rather seeing IOASID as the end of the VFIO-IOMMU-IOASID chain,
> putting it in the middle complicates locking. If you only need to FREE
> notifier for this calse, maybe VFIO could talk directly to the IOMMU
> driver before freeing an IOASID? gpasid_unbind() should already
> clear the PASID contexts, no?
>
Yes, VFIO can track all the PASIDs and make sure they do unbind before
free. But that might be more complicated in VFIO, whereas here, when a
guest exits, VFIO can just free the entire IOASID set, IOASID will
notify IOMMU and do all the cleanup.
For maintaining VMCS pasid translation table, KVM still need to know
bind/unbind in addition to free events.
In addition, we also have VDCM (virtual device composition module) that
needs to perform G-H PASID translation and sanity check. VDCM needs the
free event only. This is also in the DSA spec above. The use is that
when the guest programs a GPASID into a virtual device, VDCM (similar
to SRIOV PDEV driver) needs to intercept (via vfio mdev) and translate
GPASID to HPASID.
> Thanks,
> Jean
>
> > IOMMU driver gets notified when IOASID is freed by VFIO or core mm
> > code such that PASID context can be cleaned up.
> >
> > Signed-off-by: Liu Yi L <[email protected]>
> > Signed-off-by: Jacob Pan <[email protected]>
[Jacob Pan]