Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-11-01 Thread Pranavkumar Sawargaonkar 
Hi Bharat,

On 6 October 2015 at 15:56, Bhushan Bharat <bharat.bhus...@freescale.com> wrote:
>
>
>> -Original Message-
>> From: Alex Williamson [mailto:alex.william...@redhat.com]
>> Sent: Tuesday, October 06, 2015 4:25 AM
>> To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
>> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
>> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
>> marc.zyng...@arm.com; will.dea...@arm.com
>> Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for
>> MSI
>>
>> On Mon, 2015-10-05 at 08:33 +, Bhushan Bharat wrote:
>> >
>> >
>> > > -Original Message-
>> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
>> > > Sent: Saturday, October 03, 2015 4:17 AM
>> > > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
>> > > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
>> > > christoffer.d...@linaro.org; eric.au...@linaro.org;
>> > > pranavku...@linaro.org; marc.zyng...@arm.com; will.dea...@arm.com
>> > > Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping
>> > > for MSI
>> > >
>> > > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
>> > > > Finally ARM SMMU declare that iommu-mapping for MSI-pages are not
>> > > > set automatically and it should be set explicitly.
>> > > >
>> > > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
>> > > > ---
>> > > >  drivers/iommu/arm-smmu.c | 7 ++-
>> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> > > index
>> > > > a3956fb..9d37e72 100644
>> > > > --- a/drivers/iommu/arm-smmu.c
>> > > > +++ b/drivers/iommu/arm-smmu.c
>> > > > @@ -1401,13 +1401,18 @@ static int
>> arm_smmu_domain_get_attr(struct
>> > > iommu_domain *domain,
>> > > > enum iommu_attr attr, void *data)  
>> > > > {
>> > > > struct arm_smmu_domain *smmu_domain =
>> > > to_smmu_domain(domain);
>> > > > +   struct iommu_domain_msi_maps *msi_maps;
>> > > >
>> > > > switch (attr) {
>> > > > case DOMAIN_ATTR_NESTING:
>> > > > *(int *)data = (smmu_domain->stage ==
>> > > ARM_SMMU_DOMAIN_NESTED);
>> > > > return 0;
>> > > > case DOMAIN_ATTR_MSI_MAPPING:
>> > > > -   /* Dummy handling added */
>> > > > +   msi_maps = data;
>> > > > +
>> > > > +   msi_maps->automap = false;
>> > > > +   msi_maps->override_automap = true;
>> > > > +
>> > > > return 0;
>> > > > default:
>> > > > return -ENODEV;
>> > >
>> > > In previous discussions I understood one of the problems you were
>> > > trying to solve was having a limited number of MSI banks and while
>> > > you may be able to get isolated MSI banks for some number of users,
>> > > it wasn't unlimited and sharing may be required.  I don't see any of that
>> addressed in this series.
>> >
>> > That problem was on PowerPC. Infact there were two problems, one which
>> MSI bank to be used and second how to create iommu-mapping for device
>> assigned to userspace.
>> > First problem was PowerPC specific and that will be solved separately.
>> > For second problem, earlier I tried to added a couple of MSI specific 
>> > ioctls
>> and you suggested (IIUC) that we should have a generic reserved-iova type
>> of API and then we can map MSI bank using reserved-iova and this will not
>> require involvement of user-space.
>> >
>> > >
>> > > Also, the management of reserved IOVAs vs MSI addresses looks really
>> > > dubious to me.  How does your platform pick an MSI address and what
>> > > are we breaking by covertly changing it?  We seem to be masking over
>> > > at the VFIO level, where there should be lower level interfaces
>> > > doing the right thing when we configure MSI on the device.
>> >
>> > Yes, In my understanding the right solution

Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-10-26 Thread Christoffer Dall 
On Tue, Oct 06, 2015 at 10:26:34AM +, Bhushan Bharat wrote:

[...]

> > 
> > I'm certainly not interested in applying an maintaining an interim solution 
> > that
> > isn't the right one.  It seems like VFIO is too involved in this process in 
> > your
> > example.  On x86 we have per vector isolation and the only thing we're
> > missing is reporting back of the region used by MSI vectors as reserved IOVA
> > space (but it's standard on x86, so an x86 VM user will never use it for 
> > IOVA).
> 
> I remember you mentioned that there is no problem when running an x86 guest 
> on an x86 host.  But it will interesting when running a non-x86 VMs on an x86 
> host  or non-VM userspace use of VFIO though. 
> 
> > In your model, the MSI IOVA space is programmable,
> 
> Yes, on PowerPC and ARM-SMMU case also we have to create mapping with an 
> IOVA. First question is which IOVA to be used, and we added the reserved iova 
> ioctl for same.
> 
> Second problem is we needed an msi-page physical address for setting up 
> iommu-mapping, and so we needed to reserve an msi-page. I did this for 
> PowerPC but not in a generic extension in msi-driver and will look the code a 
> bit more details on adding an interface to reserve an msi-page or get a 
> shared msi-page with allow-unsafe-interrupt.

Sorry, I'm far from familiar with how x86 does interrupt handling and
I know very little of PCIe and MSIs, so please allow me to ask some
stupid questions:

What does an msi-page physical address mean?

> 
> Third problem is to report the reserved IOVA to be used for MSI vectors for 
> the given set of devices (devices in a vfio-group). 

What do MSI vectors mean in this context?  Is this a Linux kernel
construct, something tied to PCIe, something tied to the interrupt
controller, or?

In the case of ARM, AFAIU, you have a single doorbell register per ITS
and devices can write to this register with their device id and the
eventid.  So it's a register in a page somewhere.

Now, what is the problem you don't understand with ARM here?

> 
> Mark/Christopher,
> I am not an expert in this area so I might have to understand that code. If 
> you think you can give solution to 2nd and 3rd problem quickly then please 
> let me know.
> 
I don't really understand what you're asking, but if you can educate me
on the concepts above I may be able to offer some advice.

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-10-06 Thread Bhushan Bharat 


> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, October 06, 2015 4:25 AM
> To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> marc.zyng...@arm.com; will.dea...@arm.com
> Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for
> MSI
> 
> On Mon, 2015-10-05 at 08:33 +, Bhushan Bharat wrote:
> >
> >
> > > -Original Message-
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Saturday, October 03, 2015 4:17 AM
> > > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > > christoffer.d...@linaro.org; eric.au...@linaro.org;
> > > pranavku...@linaro.org; marc.zyng...@arm.com; will.dea...@arm.com
> > > Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping
> > > for MSI
> > >
> > > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > > Finally ARM SMMU declare that iommu-mapping for MSI-pages are not
> > > > set automatically and it should be set explicitly.
> > > >
> > > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu.c | 7 ++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index
> > > > a3956fb..9d37e72 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -1401,13 +1401,18 @@ static int
> arm_smmu_domain_get_attr(struct
> > > iommu_domain *domain,
> > > > enum iommu_attr attr, void *data)  {
> > > > struct arm_smmu_domain *smmu_domain =
> > > to_smmu_domain(domain);
> > > > +   struct iommu_domain_msi_maps *msi_maps;
> > > >
> > > > switch (attr) {
> > > > case DOMAIN_ATTR_NESTING:
> > > > *(int *)data = (smmu_domain->stage ==
> > > ARM_SMMU_DOMAIN_NESTED);
> > > > return 0;
> > > > case DOMAIN_ATTR_MSI_MAPPING:
> > > > -   /* Dummy handling added */
> > > > +   msi_maps = data;
> > > > +
> > > > +   msi_maps->automap = false;
> > > > +   msi_maps->override_automap = true;
> > > > +
> > > > return 0;
> > > > default:
> > > > return -ENODEV;
> > >
> > > In previous discussions I understood one of the problems you were
> > > trying to solve was having a limited number of MSI banks and while
> > > you may be able to get isolated MSI banks for some number of users,
> > > it wasn't unlimited and sharing may be required.  I don't see any of that
> addressed in this series.
> >
> > That problem was on PowerPC. Infact there were two problems, one which
> MSI bank to be used and second how to create iommu-mapping for device
> assigned to userspace.
> > First problem was PowerPC specific and that will be solved separately.
> > For second problem, earlier I tried to added a couple of MSI specific ioctls
> and you suggested (IIUC) that we should have a generic reserved-iova type
> of API and then we can map MSI bank using reserved-iova and this will not
> require involvement of user-space.
> >
> > >
> > > Also, the management of reserved IOVAs vs MSI addresses looks really
> > > dubious to me.  How does your platform pick an MSI address and what
> > > are we breaking by covertly changing it?  We seem to be masking over
> > > at the VFIO level, where there should be lower level interfaces
> > > doing the right thing when we configure MSI on the device.
> >
> > Yes, In my understanding the right solution should be:
> >  1) VFIO driver should know what physical-msi-address will be used for
> devices in an iommu-group.
> > I did not find an generic API, on PowerPC I added some function in
> ffrescale msi-driver and called from vfio-iommu-fsl-pamu.c (not yet
> upstreamed).
> >  2) VFIO driver should know what IOVA to be used for creating
> > iommu-mapping (VFIO APIs patch of this patch series)
> >  3) VFIO driver will create the iommu-

RE: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-10-05 Thread Bhushan Bharat 


> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Saturday, October 03, 2015 4:17 AM
> To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> marc.zyng...@arm.com; will.dea...@arm.com
> Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for
> MSI
> 
> On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > Finally ARM SMMU declare that iommu-mapping for MSI-pages are not set
> > automatically and it should be set explicitly.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index
> > a3956fb..9d37e72 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1401,13 +1401,18 @@ static int arm_smmu_domain_get_attr(struct
> iommu_domain *domain,
> > enum iommu_attr attr, void *data)  {
> > struct arm_smmu_domain *smmu_domain =
> to_smmu_domain(domain);
> > +   struct iommu_domain_msi_maps *msi_maps;
> >
> > switch (attr) {
> > case DOMAIN_ATTR_NESTING:
> > *(int *)data = (smmu_domain->stage ==
> ARM_SMMU_DOMAIN_NESTED);
> > return 0;
> > case DOMAIN_ATTR_MSI_MAPPING:
> > -   /* Dummy handling added */
> > +   msi_maps = data;
> > +
> > +   msi_maps->automap = false;
> > +   msi_maps->override_automap = true;
> > +
> > return 0;
> > default:
> > return -ENODEV;
> 
> In previous discussions I understood one of the problems you were trying to
> solve was having a limited number of MSI banks and while you may be able
> to get isolated MSI banks for some number of users, it wasn't unlimited and
> sharing may be required.  I don't see any of that addressed in this series.

That problem was on PowerPC. Infact there were two problems, one which MSI bank 
to be used and second how to create iommu-mapping for device assigned to 
userspace.
First problem was PowerPC specific and that will be solved separately.
For second problem, earlier I tried to added a couple of MSI specific ioctls 
and you suggested (IIUC) that we should have a generic reserved-iova type of 
API and then we can map MSI bank using reserved-iova and this will not require 
involvement of user-space.

> 
> Also, the management of reserved IOVAs vs MSI addresses looks really
> dubious to me.  How does your platform pick an MSI address and what are
> we breaking by covertly changing it?  We seem to be masking over at the
> VFIO level, where there should be lower level interfaces doing the right thing
> when we configure MSI on the device.

Yes, In my understanding the right solution should be:
 1) VFIO driver should know what physical-msi-address will be used for devices 
in an iommu-group.
I did not find an generic API, on PowerPC I added some function in 
ffrescale msi-driver and called from vfio-iommu-fsl-pamu.c (not yet upstreamed).
 2) VFIO driver should know what IOVA to be used for creating iommu-mapping 
(VFIO APIs patch of this patch series)
 3) VFIO driver will create the iommu-mapping using (1) and (2)
 4) VFIO driver should be able to tell the msi-driver that for a given device 
it should use different IOVA. So when composing the msi message (for the 
devices is the given iommu-group) it should use that programmed iova as 
MSI-address. This interface also needed to be developed.

I was not sure of which approach we should take. The current approach in the 
patch is simple to develop so I went ahead to take input but I agree this does 
not look very good.
What do you think, should drop this approach and work out the approach as 
described above.

Thanks
-Bharat
> 
> The problem of reporting "automap" base address isn't addressed more than
> leaving some unused field in iommu_domain_msi_maps.

N�r��yb�X��ǧv�^�)޺{.n�+h����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-10-05 Thread Alex Williamson 
On Mon, 2015-10-05 at 08:33 +, Bhushan Bharat wrote:
> 
> 
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Saturday, October 03, 2015 4:17 AM
> > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> > marc.zyng...@arm.com; will.dea...@arm.com
> > Subject: Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for
> > MSI
> > 
> > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > Finally ARM SMMU declare that iommu-mapping for MSI-pages are not set
> > > automatically and it should be set explicitly.
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > ---
> > >  drivers/iommu/arm-smmu.c | 7 ++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index
> > > a3956fb..9d37e72 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -1401,13 +1401,18 @@ static int arm_smmu_domain_get_attr(struct
> > iommu_domain *domain,
> > >   enum iommu_attr attr, void *data)  {
> > >   struct arm_smmu_domain *smmu_domain =
> > to_smmu_domain(domain);
> > > + struct iommu_domain_msi_maps *msi_maps;
> > >
> > >   switch (attr) {
> > >   case DOMAIN_ATTR_NESTING:
> > >   *(int *)data = (smmu_domain->stage ==
> > ARM_SMMU_DOMAIN_NESTED);
> > >   return 0;
> > >   case DOMAIN_ATTR_MSI_MAPPING:
> > > - /* Dummy handling added */
> > > + msi_maps = data;
> > > +
> > > + msi_maps->automap = false;
> > > + msi_maps->override_automap = true;
> > > +
> > >   return 0;
> > >   default:
> > >   return -ENODEV;
> > 
> > In previous discussions I understood one of the problems you were trying to
> > solve was having a limited number of MSI banks and while you may be able
> > to get isolated MSI banks for some number of users, it wasn't unlimited and
> > sharing may be required.  I don't see any of that addressed in this series.
> 
> That problem was on PowerPC. Infact there were two problems, one which MSI 
> bank to be used and second how to create iommu-mapping for device assigned to 
> userspace.
> First problem was PowerPC specific and that will be solved separately.
> For second problem, earlier I tried to added a couple of MSI specific ioctls 
> and you suggested (IIUC) that we should have a generic reserved-iova type of 
> API and then we can map MSI bank using reserved-iova and this will not 
> require involvement of user-space.
> 
> > 
> > Also, the management of reserved IOVAs vs MSI addresses looks really
> > dubious to me.  How does your platform pick an MSI address and what are
> > we breaking by covertly changing it?  We seem to be masking over at the
> > VFIO level, where there should be lower level interfaces doing the right 
> > thing
> > when we configure MSI on the device.
> 
> Yes, In my understanding the right solution should be:
>  1) VFIO driver should know what physical-msi-address will be used for 
> devices in an iommu-group.
> I did not find an generic API, on PowerPC I added some function in 
> ffrescale msi-driver and called from vfio-iommu-fsl-pamu.c (not yet 
> upstreamed).
>  2) VFIO driver should know what IOVA to be used for creating iommu-mapping 
> (VFIO APIs patch of this patch series)
>  3) VFIO driver will create the iommu-mapping using (1) and (2)
>  4) VFIO driver should be able to tell the msi-driver that for a given device 
> it should use different IOVA. So when composing the msi message (for the 
> devices is the given iommu-group) it should use that programmed iova as 
> MSI-address. This interface also needed to be developed.
> 
> I was not sure of which approach we should take. The current approach in the 
> patch is simple to develop so I went ahead to take input but I agree this 
> does not look very good.
> What do you think, should drop this approach and work out the approach as 
> described above.

I'm certainly not interested in applying an maintaining an interim
solution that isn't the right one.  It seems like VFIO is too involved
in this process in your example.  On x86 we have per vector isolation
and the only thing we're missing is reporting back of the region used by
MS

Re: [RFC PATCH 6/6] arm-smmu: Allow to set iommu mapping for MSI

2015-10-02 Thread Alex Williamson 
On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> Finally ARM SMMU declare that iommu-mapping for MSI-pages are not
> set automatically and it should be set explicitly.
> 
> Signed-off-by: Bharat Bhushan 
> ---
>  drivers/iommu/arm-smmu.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a3956fb..9d37e72 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1401,13 +1401,18 @@ static int arm_smmu_domain_get_attr(struct 
> iommu_domain *domain,
>   enum iommu_attr attr, void *data)
>  {
>   struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> + struct iommu_domain_msi_maps *msi_maps;
>  
>   switch (attr) {
>   case DOMAIN_ATTR_NESTING:
>   *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
>   return 0;
>   case DOMAIN_ATTR_MSI_MAPPING:
> - /* Dummy handling added */
> + msi_maps = data;
> +
> + msi_maps->automap = false;
> + msi_maps->override_automap = true;
> +
>   return 0;
>   default:
>   return -ENODEV;

In previous discussions I understood one of the problems you were trying
to solve was having a limited number of MSI banks and while you may be
able to get isolated MSI banks for some number of users, it wasn't
unlimited and sharing may be required.  I don't see any of that
addressed in this series.

Also, the management of reserved IOVAs vs MSI addresses looks really
dubious to me.  How does your platform pick an MSI address and what are
we breaking by covertly changing it?  We seem to be masking over at the
VFIO level, where there should be lower level interfaces doing the right
thing when we configure MSI on the device.

The problem of reporting "automap" base address isn't addressed more
than leaving some unused field in iommu_domain_msi_maps.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html