Re: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by endpoint
On Mon, Mar 23, 2020 at 05:25:17PM +, Bharat Bhushan wrote:
> Hi Jean,
>
> > -Original Message-
> > From: Jean-Philippe Brucker
> > Sent: Monday, March 23, 2020 3:30 PM
> > To: Bharat Bhushan
> > Cc: j...@8bytes.org; m...@redhat.com; jasow...@redhat.com;
> > virtualizat...@lists.linux-foundation.org; iommu@lists.linux-foundation.org;
> > eric.au...@redhat.com
> > Subject: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported
> > by
> > endpoint
> >
> > External Email
> >
> > --
> > Hi Bharat,
> >
> > Please add the IOMMU list on your next posting
>
> iommu@lists.linux-foundation.org is there, any other mailing list we need to
> add?
I added that address when replying :) It doesn't look like your patch
reached the list:
https://lore.kernel.org/linux-iommu/20200323095943.GA2038940@myrica/T/
But virtualization and iommu lists are enough.
Thanks,
Jean
>
> >
> > On Mon, Mar 23, 2020 at 02:11:08PM +0530, 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
> > > ---
> > > drivers/iommu/virtio-iommu.c | 24
> > > include/uapi/linux/virtio_iommu.h | 6 ++
> > > 2 files changed, 26 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iommu/virtio-iommu.c
> > > b/drivers/iommu/virtio-iommu.c index cce329d71fba..e69347ca4ee6 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_headresv_regions;
> > > + u64 pgsize_bitmap;
> > > };
> > >
> > > struct viommu_request {
> > > @@ -415,6 +416,14 @@ 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)
> > > +
> > > +{
> > > + vdev->pgsize_bitmap = mask->pgsize_bitmap;
> >
> > We need to read this through le64_to_cpu(). Also check that the length of
> > the field
> > provided by the device is >= sizeof(mask) (like
> > viommu_add_resv_mem() does)
>
> Will take care of all the comments in next verions
>
> Thank
> -Bharat
>
> >
> > > + return 0;
> > > +}
> > > +
> > > static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> > > struct virtio_iommu_probe_resv_mem *mem,
> > > size_t len)
> > > @@ -494,11 +503,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);
> > > + break;
> > > default:
> > > dev_err(dev, "unknown viommu prop 0x%x\n", type);
> > > }
> > > @@ -607,16 +618,21 @@ 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;
> > > + if (vdev->pgsize_bitmap)
> > > + domain->pgsize_bitmap = vdev->pgsize_bitmap;
> > > + else
> > > + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> > > +
> >
> > nit: it could be nicer to initialize vdev->pgsize_bitmap in add_device(),
> > override it
> > in probe_endpoint(), and just copy it here.
> >
> > > domain->geometry= viommu->geometry;
> > >
> > > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, @@
> > > -657,7 +673,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,
Re: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by endpoint
On Mon, Mar 23, 2020 at 06:04:37PM +, Bharat Bhushan wrote:
> > > -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;
> > > + if (vdev->pgsize_bitmap)
> > > + domain->pgsize_bitmap = vdev->pgsize_bitmap;
> > > + else
> > > + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> > > +
> >
> > nit: it could be nicer to initialize vdev->pgsize_bitmap in add_device(),
>
> To what size we should initialize in add_device, PAGE_SIZE?
No to viommu->pgsize_bitmap
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by endpoint
Hi Jean,
> -Original Message-
> From: Jean-Philippe Brucker
> Sent: Monday, March 23, 2020 3:30 PM
> To: Bharat Bhushan
> Cc: j...@8bytes.org; m...@redhat.com; jasow...@redhat.com;
> virtualizat...@lists.linux-foundation.org; iommu@lists.linux-foundation.org;
> eric.au...@redhat.com
> Subject: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by
> endpoint
>
> External Email
>
> --
> Hi Bharat,
>
> Please add the IOMMU list on your next posting
>
> On Mon, Mar 23, 2020 at 02:11:08PM +0530, 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
> > ---
> > drivers/iommu/virtio-iommu.c | 24
> > include/uapi/linux/virtio_iommu.h | 6 ++
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c
> > b/drivers/iommu/virtio-iommu.c index cce329d71fba..e69347ca4ee6 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_headresv_regions;
> > + u64 pgsize_bitmap;
> > };
> >
> > struct viommu_request {
> > @@ -415,6 +416,14 @@ 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)
> > +
> > +{
> > + vdev->pgsize_bitmap = mask->pgsize_bitmap;
>
> We need to read this through le64_to_cpu(). Also check that the length of the
> field
> provided by the device is >= sizeof(mask) (like
> viommu_add_resv_mem() does)
>
> > + return 0;
> > +}
> > +
> > static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> >struct virtio_iommu_probe_resv_mem *mem,
> >size_t len)
> > @@ -494,11 +503,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);
> > + break;
> > default:
> > dev_err(dev, "unknown viommu prop 0x%x\n", type);
> > }
> > @@ -607,16 +618,21 @@ 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;
> > + if (vdev->pgsize_bitmap)
> > + domain->pgsize_bitmap = vdev->pgsize_bitmap;
> > + else
> > + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> > +
>
> nit: it could be nicer to initialize vdev->pgsize_bitmap in add_device(),
To what size we should initialize in add_device, PAGE_SIZE?
Thanks
-Bharat
> override it
> in probe_endpoint(), and just copy it here.
>
> > domain->geometry= viommu->geometry;
> >
> > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, @@
> > -657,7 +673,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);
>
> Attaching additional endpoints with different masks to the domain should
> return
> an error
>
> > } else if (vdomain->viommu != vdev->viommu) {
> > dev_err(dev, "cannot attach to foreign vIOMMU\n");
> > ret = -EXDEV;
> > diff --git a/include/uapi/linux/virtio_iommu.h
> > b/include/uapi/linux/virtio_iommu.h
> > index 237e36a280cb..aff3db0ef54b 100644
> > --- a/include/uapi/linux/virtio_iommu.h
> > +++ b/include/uapi/linux/virtio_iommu.h
> > @@ -111,6 +111,7 @@ struct virtio_iommu_req_un
RE: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by endpoint
Hi Jean,
> -Original Message-
> From: Jean-Philippe Brucker
> Sent: Monday, March 23, 2020 3:30 PM
> To: Bharat Bhushan
> Cc: j...@8bytes.org; m...@redhat.com; jasow...@redhat.com;
> virtualizat...@lists.linux-foundation.org; iommu@lists.linux-foundation.org;
> eric.au...@redhat.com
> Subject: [EXT] Re: [PATCH RFC] iommu/virtio: Use page size bitmap supported by
> endpoint
>
> External Email
>
> --
> Hi Bharat,
>
> Please add the IOMMU list on your next posting
iommu@lists.linux-foundation.org is there, any other mailing list we need to
add?
>
> On Mon, Mar 23, 2020 at 02:11:08PM +0530, 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
> > ---
> > drivers/iommu/virtio-iommu.c | 24
> > include/uapi/linux/virtio_iommu.h | 6 ++
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c
> > b/drivers/iommu/virtio-iommu.c index cce329d71fba..e69347ca4ee6 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_headresv_regions;
> > + u64 pgsize_bitmap;
> > };
> >
> > struct viommu_request {
> > @@ -415,6 +416,14 @@ 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)
> > +
> > +{
> > + vdev->pgsize_bitmap = mask->pgsize_bitmap;
>
> We need to read this through le64_to_cpu(). Also check that the length of the
> field
> provided by the device is >= sizeof(mask) (like
> viommu_add_resv_mem() does)
Will take care of all the comments in next verions
Thank
-Bharat
>
> > + return 0;
> > +}
> > +
> > static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> >struct virtio_iommu_probe_resv_mem *mem,
> >size_t len)
> > @@ -494,11 +503,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);
> > + break;
> > default:
> > dev_err(dev, "unknown viommu prop 0x%x\n", type);
> > }
> > @@ -607,16 +618,21 @@ 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;
> > + if (vdev->pgsize_bitmap)
> > + domain->pgsize_bitmap = vdev->pgsize_bitmap;
> > + else
> > + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> > +
>
> nit: it could be nicer to initialize vdev->pgsize_bitmap in add_device(),
> override it
> in probe_endpoint(), and just copy it here.
>
> > domain->geometry= viommu->geometry;
> >
> > ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain, @@
> > -657,7 +673,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);
>
> Attaching additional endpoints with different masks to the domain should
> return
> an error
>
> > } else if (vdomain->viommu != vdev->viommu) {
> > dev_err(dev, "cannot attach to foreign vIOMMU\n");
> > ret = -EXDEV;
> > diff --git a/include/uapi/linux/virtio_iommu.h
> > b/include/uapi/linux/virtio_iommu.h
> > index 237e36a280cb..aff3db0ef54b 100644
> > --- a/include/uapi/linux/virtio_iommu.h
> > +++ b/include/
