* [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint @ 2020-04-01 11:38 Bharat Bhushan 2020-04-01 13:00 ` Robin Murphy 0 siblings, 1 reply; 5+ messages in thread From: Bharat Bhushan @ 2020-04-01 11:38 UTC (permalink / raw) To: jean-philippe, joro, mst, jasowang, virtualization, iommu, linux-kernel, eric.auger.pro, eric.auger Cc: Bharat Bhushan Different endpoint can support different page size, probe endpoint if it supports specific page size otherwise use global page sizes. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- drivers/iommu/virtio-iommu.c | 33 +++++++++++++++++++++++++++---- include/uapi/linux/virtio_iommu.h | 7 +++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index cce329d71fba..c794cb5b7b3e 100644 --- a/drivers/iommu/virtio-iommu.c +++ b/drivers/iommu/virtio-iommu.c @@ -78,6 +78,7 @@ struct viommu_endpoint { struct viommu_dev *viommu; struct viommu_domain *vdomain; struct list_head resv_regions; + u64 pgsize_bitmap; }; struct viommu_request { @@ -415,6 +416,20 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain) return ret; } +static int viommu_set_pgsize_bitmap(struct viommu_endpoint *vdev, + struct virtio_iommu_probe_pgsize_mask *mask, + size_t len) + +{ + u64 pgsize_bitmap = le64_to_cpu(mask->pgsize_bitmap); + + if (len < sizeof(*mask)) + return -EINVAL; + + vdev->pgsize_bitmap = pgsize_bitmap; + return 0; +} + static int viommu_add_resv_mem(struct viommu_endpoint *vdev, struct virtio_iommu_probe_resv_mem *mem, size_t len) @@ -494,11 +509,13 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev) while (type != VIRTIO_IOMMU_PROBE_T_NONE && cur < viommu->probe_size) { len = le16_to_cpu(prop->length) + sizeof(*prop); - switch (type) { case VIRTIO_IOMMU_PROBE_T_RESV_MEM: ret = viommu_add_resv_mem(vdev, (void *)prop, len); break; + case VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK: + ret = viommu_set_pgsize_bitmap(vdev, (void *)prop, len); + break; default: dev_err(dev, "unknown viommu prop 0x%x\n", type); } @@ -607,16 +624,23 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type) return &vdomain->domain; } -static int viommu_domain_finalise(struct viommu_dev *viommu, +static int viommu_domain_finalise(struct viommu_endpoint *vdev, struct iommu_domain *domain) { int ret; struct viommu_domain *vdomain = to_viommu_domain(domain); + struct viommu_dev *viommu = vdev->viommu; vdomain->viommu = viommu; vdomain->map_flags = viommu->map_flags; - domain->pgsize_bitmap = viommu->pgsize_bitmap; + /* Devices in same domain must support same size pages */ + if ((domain->pgsize_bitmap != viommu->pgsize_bitmap) && + (domain->pgsize_bitmap != vdev->pgsize_bitmap)) + return -EINVAL; + + domain->pgsize_bitmap = vdev->pgsize_bitmap; + domain->geometry = viommu->geometry; ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, @@ -657,7 +681,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) * Properly initialize the domain now that we know which viommu * owns it. */ - ret = viommu_domain_finalise(vdev->viommu, domain); + ret = viommu_domain_finalise(vdev, domain); } else if (vdomain->viommu != vdev->viommu) { dev_err(dev, "cannot attach to foreign vIOMMU\n"); ret = -EXDEV; @@ -875,6 +899,7 @@ static int viommu_add_device(struct device *dev) vdev->dev = dev; vdev->viommu = viommu; + vdev->pgsize_bitmap = viommu->pgsize_bitmap; INIT_LIST_HEAD(&vdev->resv_regions); fwspec->iommu_priv = vdev; diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h index 237e36a280cb..dc9d3f40bcd8 100644 --- a/include/uapi/linux/virtio_iommu.h +++ b/include/uapi/linux/virtio_iommu.h @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap { #define VIRTIO_IOMMU_PROBE_T_NONE 0 #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1 +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2 #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff @@ -119,6 +120,12 @@ struct virtio_iommu_probe_property { __le16 length; }; +struct virtio_iommu_probe_pgsize_mask { + struct virtio_iommu_probe_property head; + __u8 reserved[4]; + __u64 pgsize_bitmap; +}; + #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0 #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1 -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint 2020-04-01 11:38 [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint Bharat Bhushan @ 2020-04-01 13:00 ` Robin Murphy 2020-04-01 15:49 ` Jean-Philippe Brucker 0 siblings, 1 reply; 5+ messages in thread From: Robin Murphy @ 2020-04-01 13:00 UTC (permalink / raw) To: Bharat Bhushan, jean-philippe, joro, mst, jasowang, virtualization, iommu, linux-kernel, eric.auger.pro, eric.auger On 2020-04-01 12:38 pm, Bharat Bhushan wrote: > Different endpoint can support different page size, probe > endpoint if it supports specific page size otherwise use > global page sizes. > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > --- > drivers/iommu/virtio-iommu.c | 33 +++++++++++++++++++++++++++---- > include/uapi/linux/virtio_iommu.h | 7 +++++++ > 2 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > index cce329d71fba..c794cb5b7b3e 100644 > --- a/drivers/iommu/virtio-iommu.c > +++ b/drivers/iommu/virtio-iommu.c > @@ -78,6 +78,7 @@ struct viommu_endpoint { > struct viommu_dev *viommu; > struct viommu_domain *vdomain; > struct list_head resv_regions; > + u64 pgsize_bitmap; > }; > > struct viommu_request { > @@ -415,6 +416,20 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain) > return ret; > } > > +static int viommu_set_pgsize_bitmap(struct viommu_endpoint *vdev, > + struct virtio_iommu_probe_pgsize_mask *mask, > + size_t len) > + > +{ > + u64 pgsize_bitmap = le64_to_cpu(mask->pgsize_bitmap); > + > + if (len < sizeof(*mask)) > + return -EINVAL; > + > + vdev->pgsize_bitmap = pgsize_bitmap; > + return 0; > +} > + > static int viommu_add_resv_mem(struct viommu_endpoint *vdev, > struct virtio_iommu_probe_resv_mem *mem, > size_t len) > @@ -494,11 +509,13 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev) > while (type != VIRTIO_IOMMU_PROBE_T_NONE && > cur < viommu->probe_size) { > len = le16_to_cpu(prop->length) + sizeof(*prop); > - > switch (type) { > case VIRTIO_IOMMU_PROBE_T_RESV_MEM: > ret = viommu_add_resv_mem(vdev, (void *)prop, len); > break; > + case VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK: > + ret = viommu_set_pgsize_bitmap(vdev, (void *)prop, len); > + break; > default: > dev_err(dev, "unknown viommu prop 0x%x\n", type); > } > @@ -607,16 +624,23 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type) > return &vdomain->domain; > } > > -static int viommu_domain_finalise(struct viommu_dev *viommu, > +static int viommu_domain_finalise(struct viommu_endpoint *vdev, > struct iommu_domain *domain) > { > int ret; > struct viommu_domain *vdomain = to_viommu_domain(domain); > + struct viommu_dev *viommu = vdev->viommu; > > vdomain->viommu = viommu; > vdomain->map_flags = viommu->map_flags; > > - domain->pgsize_bitmap = viommu->pgsize_bitmap; > + /* Devices in same domain must support same size pages */ AFAICS what the code appears to do is enforce that the first endpoint attached to any domain has the same pgsize_bitmap as the most recently probed viommu_dev instance, then ignore any subsequent endpoints attached to the same domain. Thus I'm not sure that comment is accurate. Robin. > + if ((domain->pgsize_bitmap != viommu->pgsize_bitmap) && > + (domain->pgsize_bitmap != vdev->pgsize_bitmap)) > + return -EINVAL; > + > + domain->pgsize_bitmap = vdev->pgsize_bitmap; > + > domain->geometry = viommu->geometry; > > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, > @@ -657,7 +681,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) > * Properly initialize the domain now that we know which viommu > * owns it. > */ > - ret = viommu_domain_finalise(vdev->viommu, domain); > + ret = viommu_domain_finalise(vdev, domain); > } else if (vdomain->viommu != vdev->viommu) { > dev_err(dev, "cannot attach to foreign vIOMMU\n"); > ret = -EXDEV; > @@ -875,6 +899,7 @@ static int viommu_add_device(struct device *dev) > > vdev->dev = dev; > vdev->viommu = viommu; > + vdev->pgsize_bitmap = viommu->pgsize_bitmap; > INIT_LIST_HEAD(&vdev->resv_regions); > fwspec->iommu_priv = vdev; > > diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h > index 237e36a280cb..dc9d3f40bcd8 100644 > --- a/include/uapi/linux/virtio_iommu.h > +++ b/include/uapi/linux/virtio_iommu.h > @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap { > > #define VIRTIO_IOMMU_PROBE_T_NONE 0 > #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1 > +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2 > > #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff > > @@ -119,6 +120,12 @@ struct virtio_iommu_probe_property { > __le16 length; > }; > > +struct virtio_iommu_probe_pgsize_mask { > + struct virtio_iommu_probe_property head; > + __u8 reserved[4]; > + __u64 pgsize_bitmap; > +}; > + > #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0 > #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint 2020-04-01 13:00 ` Robin Murphy @ 2020-04-01 15:49 ` Jean-Philippe Brucker 2020-04-02 3:53 ` [EXT] " Bharat Bhushan 0 siblings, 1 reply; 5+ messages in thread From: Jean-Philippe Brucker @ 2020-04-01 15:49 UTC (permalink / raw) To: Robin Murphy Cc: Bharat Bhushan, joro, mst, jasowang, virtualization, iommu, linux-kernel, eric.auger.pro, eric.auger On Wed, Apr 01, 2020 at 02:00:13PM +0100, Robin Murphy wrote: > On 2020-04-01 12:38 pm, Bharat Bhushan wrote: > > Different endpoint can support different page size, probe > > endpoint if it supports specific page size otherwise use > > global page sizes. > > > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > > --- > > drivers/iommu/virtio-iommu.c | 33 +++++++++++++++++++++++++++---- > > include/uapi/linux/virtio_iommu.h | 7 +++++++ > > 2 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > > index cce329d71fba..c794cb5b7b3e 100644 > > --- a/drivers/iommu/virtio-iommu.c > > +++ b/drivers/iommu/virtio-iommu.c > > @@ -78,6 +78,7 @@ struct viommu_endpoint { > > struct viommu_dev *viommu; > > struct viommu_domain *vdomain; > > struct list_head resv_regions; > > + u64 pgsize_bitmap; > > }; > > struct viommu_request { > > @@ -415,6 +416,20 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain) > > return ret; > > } > > +static int viommu_set_pgsize_bitmap(struct viommu_endpoint *vdev, > > + struct virtio_iommu_probe_pgsize_mask *mask, > > + size_t len) > > + > > +{ > > + u64 pgsize_bitmap = le64_to_cpu(mask->pgsize_bitmap); > > + > > + if (len < sizeof(*mask)) > > + return -EINVAL; > > + > > + vdev->pgsize_bitmap = pgsize_bitmap; > > + return 0; > > +} > > + > > static int viommu_add_resv_mem(struct viommu_endpoint *vdev, > > struct virtio_iommu_probe_resv_mem *mem, > > size_t len) > > @@ -494,11 +509,13 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev) > > while (type != VIRTIO_IOMMU_PROBE_T_NONE && > > cur < viommu->probe_size) { > > len = le16_to_cpu(prop->length) + sizeof(*prop); > > - Whitespace change > > switch (type) { > > case VIRTIO_IOMMU_PROBE_T_RESV_MEM: > > ret = viommu_add_resv_mem(vdev, (void *)prop, len); > > break; > > + case VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK: > > + ret = viommu_set_pgsize_bitmap(vdev, (void *)prop, len); > > + break; > > default: > > dev_err(dev, "unknown viommu prop 0x%x\n", type); > > } > > @@ -607,16 +624,23 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type) > > return &vdomain->domain; > > } > > -static int viommu_domain_finalise(struct viommu_dev *viommu, > > +static int viommu_domain_finalise(struct viommu_endpoint *vdev, > > struct iommu_domain *domain) > > { > > int ret; > > struct viommu_domain *vdomain = to_viommu_domain(domain); > > + struct viommu_dev *viommu = vdev->viommu; > > vdomain->viommu = viommu; > > vdomain->map_flags = viommu->map_flags; > > - domain->pgsize_bitmap = viommu->pgsize_bitmap; > > + /* Devices in same domain must support same size pages */ > > AFAICS what the code appears to do is enforce that the first endpoint > attached to any domain has the same pgsize_bitmap as the most recently > probed viommu_dev instance, then ignore any subsequent endpoints attached to > the same domain. Thus I'm not sure that comment is accurate. > Yes viommu_domain_finalise() is only called once. What I had in mind is something like: ---- 8< ---- diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index 750f69c49b95..8303b7b513ff 100644 --- a/drivers/iommu/virtio-iommu.c +++ b/drivers/iommu/virtio-iommu.c @@ -639,6 +639,29 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev, return 0; } +/* + * Check whether the endpoint's capabilities are compatible with other endpoints + * in the domain. Report any inconsistency. + */ +static bool viommu_endpoint_is_compatible(struct viommu_endpoint *vdev, + struct viommu_domain *vdomain) +{ + struct device *dev = vdev->dev; + + if (vdomain->viommu != vdev->viommu) { + dev_err(dev, "cannot attach to foreign vIOMMU\n"); + return false; + } + + if (vdomain->domain.pgsize_bitmap != vdev->pgsize_bitmap) { + dev_err(dev, "incompatible domain bitmap 0x%lx != 0x%lx\n", + vdomain->domain.pgsize_bitmap, vdev->pgsize_bitmap); + return false; + } + + return true; +} + static void viommu_domain_free(struct iommu_domain *domain) { struct viommu_domain *vdomain = to_viommu_domain(domain); @@ -670,9 +693,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) * owns it. */ ret = viommu_domain_finalise(vdev, domain); - } else if (vdomain->viommu != vdev->viommu) { - dev_err(dev, "cannot attach to foreign vIOMMU\n"); - ret = -EXDEV; + } else if (!viommu_endpoint_is_compatible(vdev, vdomain)) { + ret = -EINVAL; } mutex_unlock(&vdomain->mutex); ---- >8 ---- > > > + if ((domain->pgsize_bitmap != viommu->pgsize_bitmap) && > > + (domain->pgsize_bitmap != vdev->pgsize_bitmap)) > > + return -EINVAL; > > + > > + domain->pgsize_bitmap = vdev->pgsize_bitmap; > > + > > domain->geometry = viommu->geometry; > > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, > > @@ -657,7 +681,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) > > * Properly initialize the domain now that we know which viommu > > * owns it. > > */ > > - ret = viommu_domain_finalise(vdev->viommu, domain); > > + ret = viommu_domain_finalise(vdev, domain); > > } else if (vdomain->viommu != vdev->viommu) { > > dev_err(dev, "cannot attach to foreign vIOMMU\n"); > > ret = -EXDEV; > > @@ -875,6 +899,7 @@ static int viommu_add_device(struct device *dev) > > vdev->dev = dev; > > vdev->viommu = viommu; > > + vdev->pgsize_bitmap = viommu->pgsize_bitmap; > > INIT_LIST_HEAD(&vdev->resv_regions); > > fwspec->iommu_priv = vdev; > > diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h > > index 237e36a280cb..dc9d3f40bcd8 100644 > > --- a/include/uapi/linux/virtio_iommu.h > > +++ b/include/uapi/linux/virtio_iommu.h > > @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap { > > #define VIRTIO_IOMMU_PROBE_T_NONE 0 > > #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1 > > +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2 > > #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff > > @@ -119,6 +120,12 @@ struct virtio_iommu_probe_property { > > __le16 length; > > }; > > +struct virtio_iommu_probe_pgsize_mask { > > + struct virtio_iommu_probe_property head; > > + __u8 reserved[4]; > > + __u64 pgsize_bitmap; Should be __le64 Thanks, Jean > > +}; > > + > > #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0 > > #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1 > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [EXT] Re: [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint 2020-04-01 15:49 ` Jean-Philippe Brucker @ 2020-04-02 3:53 ` Bharat Bhushan 2020-04-02 9:24 ` Jean-Philippe Brucker 0 siblings, 1 reply; 5+ messages in thread From: Bharat Bhushan @ 2020-04-02 3:53 UTC (permalink / raw) To: Jean-Philippe Brucker, Robin Murphy Cc: joro, mst, jasowang, virtualization, iommu, linux-kernel, eric.auger.pro, eric.auger > -----Original Message----- > From: Jean-Philippe Brucker <jean-philippe@linaro.org> > Sent: Wednesday, April 1, 2020 9:20 PM > To: Robin Murphy <robin.murphy@arm.com> > Cc: Bharat Bhushan <bbhushan2@marvell.com>; joro@8bytes.org; > mst@redhat.com; jasowang@redhat.com; virtualization@lists.linux- > foundation.org; iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org; > eric.auger.pro@gmail.com; eric.auger@redhat.com > Subject: [EXT] Re: [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by > endpoint > > External Email > > ---------------------------------------------------------------------- > On Wed, Apr 01, 2020 at 02:00:13PM +0100, Robin Murphy wrote: > > On 2020-04-01 12:38 pm, Bharat Bhushan wrote: > > > Different endpoint can support different page size, probe endpoint > > > if it supports specific page size otherwise use global page sizes. > > > > > > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> > > > --- > > > drivers/iommu/virtio-iommu.c | 33 +++++++++++++++++++++++++++---- > > > include/uapi/linux/virtio_iommu.h | 7 +++++++ > > > 2 files changed, 36 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iommu/virtio-iommu.c > > > b/drivers/iommu/virtio-iommu.c index cce329d71fba..c794cb5b7b3e > > > 100644 > > > --- a/drivers/iommu/virtio-iommu.c > > > +++ b/drivers/iommu/virtio-iommu.c > > > @@ -78,6 +78,7 @@ struct viommu_endpoint { > > > struct viommu_dev *viommu; > > > struct viommu_domain *vdomain; > > > struct list_head resv_regions; > > > + u64 pgsize_bitmap; > > > }; > > > struct viommu_request { > > > @@ -415,6 +416,20 @@ static int viommu_replay_mappings(struct > viommu_domain *vdomain) > > > return ret; > > > } > > > +static int viommu_set_pgsize_bitmap(struct viommu_endpoint *vdev, > > > + struct virtio_iommu_probe_pgsize_mask *mask, > > > + size_t len) > > > + > > > +{ > > > + u64 pgsize_bitmap = le64_to_cpu(mask->pgsize_bitmap); > > > + > > > + if (len < sizeof(*mask)) > > > + return -EINVAL; > > > + > > > + vdev->pgsize_bitmap = pgsize_bitmap; > > > + return 0; > > > +} > > > + > > > static int viommu_add_resv_mem(struct viommu_endpoint *vdev, > > > struct virtio_iommu_probe_resv_mem *mem, > > > size_t len) > > > @@ -494,11 +509,13 @@ static int viommu_probe_endpoint(struct > viommu_dev *viommu, struct device *dev) > > > while (type != VIRTIO_IOMMU_PROBE_T_NONE && > > > cur < viommu->probe_size) { > > > len = le16_to_cpu(prop->length) + sizeof(*prop); > > > - > > Whitespace change > > > > switch (type) { > > > case VIRTIO_IOMMU_PROBE_T_RESV_MEM: > > > ret = viommu_add_resv_mem(vdev, (void *)prop, len); > > > break; > > > + case VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK: > > > + ret = viommu_set_pgsize_bitmap(vdev, (void *)prop, len); > > > + break; > > > default: > > > dev_err(dev, "unknown viommu prop 0x%x\n", type); > > > } > > > @@ -607,16 +624,23 @@ static struct iommu_domain > *viommu_domain_alloc(unsigned type) > > > return &vdomain->domain; > > > } > > > -static int viommu_domain_finalise(struct viommu_dev *viommu, > > > +static int viommu_domain_finalise(struct viommu_endpoint *vdev, > > > struct iommu_domain *domain) > > > { > > > int ret; > > > struct viommu_domain *vdomain = to_viommu_domain(domain); > > > + struct viommu_dev *viommu = vdev->viommu; > > > vdomain->viommu = viommu; > > > vdomain->map_flags = viommu->map_flags; > > > - domain->pgsize_bitmap = viommu->pgsize_bitmap; > > > + /* Devices in same domain must support same size pages */ > > > > AFAICS what the code appears to do is enforce that the first endpoint > > attached to any domain has the same pgsize_bitmap as the most recently > > probed viommu_dev instance, then ignore any subsequent endpoints > > attached to the same domain. Thus I'm not sure that comment is accurate. > > > > Yes viommu_domain_finalise() is only called once. What I had in mind is > something like: Ohh, I did not realized that viommu_domain_finalise(). I have included below suggested changes > > ---- 8< ---- > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index > 750f69c49b95..8303b7b513ff 100644 > --- a/drivers/iommu/virtio-iommu.c > +++ b/drivers/iommu/virtio-iommu.c > @@ -639,6 +639,29 @@ static int viommu_domain_finalise(struct > viommu_endpoint *vdev, > return 0; > } > > +/* > + * Check whether the endpoint's capabilities are compatible with other > +endpoints > + * in the domain. Report any inconsistency. > + */ > +static bool viommu_endpoint_is_compatible(struct viommu_endpoint *vdev, > + struct viommu_domain *vdomain) > +{ > + struct device *dev = vdev->dev; > + > + if (vdomain->viommu != vdev->viommu) { > + dev_err(dev, "cannot attach to foreign vIOMMU\n"); > + return false; > + } > + > + if (vdomain->domain.pgsize_bitmap != vdev->pgsize_bitmap) { > + dev_err(dev, "incompatible domain bitmap 0x%lx != 0x%lx\n", > + vdomain->domain.pgsize_bitmap, vdev->pgsize_bitmap); > + return false; > + } > + > + return true; > +} > + > static void viommu_domain_free(struct iommu_domain *domain) { > struct viommu_domain *vdomain = to_viommu_domain(domain); @@ - > 670,9 +693,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, > struct device *dev) > * owns it. > */ > ret = viommu_domain_finalise(vdev, domain); > - } else if (vdomain->viommu != vdev->viommu) { > - dev_err(dev, "cannot attach to foreign vIOMMU\n"); > - ret = -EXDEV; > + } else if (!viommu_endpoint_is_compatible(vdev, vdomain)) { > + ret = -EINVAL; > } > mutex_unlock(&vdomain->mutex); > ---- >8 ---- > > > > > > + if ((domain->pgsize_bitmap != viommu->pgsize_bitmap) && > > > + (domain->pgsize_bitmap != vdev->pgsize_bitmap)) > > > + return -EINVAL; > > > + > > > + domain->pgsize_bitmap = vdev->pgsize_bitmap; > > > + > > > domain->geometry = viommu->geometry; > > > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, > > > @@ -657,7 +681,7 @@ static int viommu_attach_dev(struct iommu_domain > *domain, struct device *dev) > > > * Properly initialize the domain now that we know which viommu > > > * owns it. > > > */ > > > - ret = viommu_domain_finalise(vdev->viommu, domain); > > > + ret = viommu_domain_finalise(vdev, domain); > > > } else if (vdomain->viommu != vdev->viommu) { > > > dev_err(dev, "cannot attach to foreign vIOMMU\n"); > > > ret = -EXDEV; > > > @@ -875,6 +899,7 @@ static int viommu_add_device(struct device *dev) > > > vdev->dev = dev; > > > vdev->viommu = viommu; > > > + vdev->pgsize_bitmap = viommu->pgsize_bitmap; > > > INIT_LIST_HEAD(&vdev->resv_regions); > > > fwspec->iommu_priv = vdev; > > > diff --git a/include/uapi/linux/virtio_iommu.h > > > b/include/uapi/linux/virtio_iommu.h > > > index 237e36a280cb..dc9d3f40bcd8 100644 > > > --- a/include/uapi/linux/virtio_iommu.h > > > +++ b/include/uapi/linux/virtio_iommu.h > > > @@ -111,6 +111,7 @@ struct virtio_iommu_req_unmap { > > > #define VIRTIO_IOMMU_PROBE_T_NONE 0 > > > #define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1 > > > +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK 2 > > > #define VIRTIO_IOMMU_PROBE_T_MASK 0xfff > > > @@ -119,6 +120,12 @@ struct virtio_iommu_probe_property { > > > __le16 length; > > > }; > > > +struct virtio_iommu_probe_pgsize_mask { > > > + struct virtio_iommu_probe_property head; > > > + __u8 reserved[4]; > > > + __u64 pgsize_bitmap; > > Should be __le64 Based on" iommu/virtio: Fix sparse warning" patch https://www.spinics.net/lists/linux-virtualization/msg41944.html changed to __u64 (not __le64) Will keep __le64. Thanks -Bharat > > Thanks, > Jean > > > > +}; > > > + > > > #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0 > > > #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] Re: [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint 2020-04-02 3:53 ` [EXT] " Bharat Bhushan @ 2020-04-02 9:24 ` Jean-Philippe Brucker 0 siblings, 0 replies; 5+ messages in thread From: Jean-Philippe Brucker @ 2020-04-02 9:24 UTC (permalink / raw) To: Bharat Bhushan Cc: Robin Murphy, joro, mst, jasowang, virtualization, iommu, linux-kernel, eric.auger.pro, eric.auger On Thu, Apr 02, 2020 at 03:53:40AM +0000, Bharat Bhushan wrote: > > > > +struct virtio_iommu_probe_pgsize_mask { > > > > + struct virtio_iommu_probe_property head; > > > > + __u8 reserved[4]; > > > > + __u64 pgsize_bitmap; > > > > Should be __le64 > > Based on" iommu/virtio: Fix sparse warning" patch https://www.spinics.net/lists/linux-virtualization/msg41944.html changed to __u64 (not __le64) Yes that one was only for the virtio config struct, to play nice with other devices. We should still use __le for the other structures, including probe properties. Thanks, Jean > > Will keep __le64. > > Thanks > -Bharat > > > > > Thanks, > > Jean > > > > > > +}; > > > > + > > > > #define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0 > > > > #define VIRTIO_IOMMU_RESV_MEM_T_MSI 1 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-02 9:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-01 11:38 [RFC PATCH v2] iommu/virtio: Use page size bitmap supported by endpoint Bharat Bhushan 2020-04-01 13:00 ` Robin Murphy 2020-04-01 15:49 ` Jean-Philippe Brucker 2020-04-02 3:53 ` [EXT] " Bharat Bhushan 2020-04-02 9:24 ` Jean-Philippe Brucker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).