| Message ID | 1443624989-24346-6-git-send-email-Bharat.Bhushan@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.Bhushan@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. 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleCBXaWxsaWFtc29u IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFNhdHVyZGF5LCBP Y3RvYmVyIDAzLCAyMDE1IDQ6MTcgQU0NCj4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3NyA8Qmhh cmF0LkJodXNoYW5AZnJlZXNjYWxlLmNvbT4NCj4gQ2M6IGt2bWFybUBsaXN0cy5jcy5jb2x1bWJp YS5lZHU7IGt2bUB2Z2VyLmtlcm5lbC5vcmc7DQo+IGNocmlzdG9mZmVyLmRhbGxAbGluYXJvLm9y ZzsgZXJpYy5hdWdlckBsaW5hcm8ub3JnOyBwcmFuYXZrdW1hckBsaW5hcm8ub3JnOw0KPiBtYXJj Lnp5bmdpZXJAYXJtLmNvbTsgd2lsbC5kZWFjb25AYXJtLmNvbQ0KPiBTdWJqZWN0OiBSZTogW1JG QyBQQVRDSCA2LzZdIGFybS1zbW11OiBBbGxvdyB0byBzZXQgaW9tbXUgbWFwcGluZyBmb3INCj4g TVNJDQo+IA0KPiBPbiBXZWQsIDIwMTUtMDktMzAgYXQgMjA6MjYgKzA1MzAsIEJoYXJhdCBCaHVz aGFuIHdyb3RlOg0KPiA+IEZpbmFsbHkgQVJNIFNNTVUgZGVjbGFyZSB0aGF0IGlvbW11LW1hcHBp bmcgZm9yIE1TSS1wYWdlcyBhcmUgbm90IHNldA0KPiA+IGF1dG9tYXRpY2FsbHkgYW5kIGl0IHNo b3VsZCBiZSBzZXQgZXhwbGljaXRseS4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IEJoYXJhdCBC aHVzaGFuIDxCaGFyYXQuQmh1c2hhbkBmcmVlc2NhbGUuY29tPg0KPiA+IC0tLQ0KPiA+ICBkcml2 ZXJzL2lvbW11L2FybS1zbW11LmMgfCA3ICsrKysrKy0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDYg aW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZl cnMvaW9tbXUvYXJtLXNtbXUuYyBiL2RyaXZlcnMvaW9tbXUvYXJtLXNtbXUuYw0KPiBpbmRleA0K PiA+IGEzOTU2ZmIuLjlkMzdlNzIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9pb21tdS9hcm0t c21tdS5jDQo+ID4gKysrIGIvZHJpdmVycy9pb21tdS9hcm0tc21tdS5jDQo+ID4gQEAgLTE0MDEs MTMgKzE0MDEsMTggQEAgc3RhdGljIGludCBhcm1fc21tdV9kb21haW5fZ2V0X2F0dHIoc3RydWN0 DQo+IGlvbW11X2RvbWFpbiAqZG9tYWluLA0KPiA+ICAJCQkJICAgIGVudW0gaW9tbXVfYXR0ciBh dHRyLCB2b2lkICpkYXRhKSAgew0KPiA+ICAJc3RydWN0IGFybV9zbW11X2RvbWFpbiAqc21tdV9k b21haW4gPQ0KPiB0b19zbW11X2RvbWFpbihkb21haW4pOw0KPiA+ICsJc3RydWN0IGlvbW11X2Rv bWFpbl9tc2lfbWFwcyAqbXNpX21hcHM7DQo+ID4NCj4gPiAgCXN3aXRjaCAoYXR0cikgew0KPiA+ ICAJY2FzZSBET01BSU5fQVRUUl9ORVNUSU5HOg0KPiA+ICAJCSooaW50ICopZGF0YSA9IChzbW11 X2RvbWFpbi0+c3RhZ2UgPT0NCj4gQVJNX1NNTVVfRE9NQUlOX05FU1RFRCk7DQo+ID4gIAkJcmV0 dXJuIDA7DQo+ID4gIAljYXNlIERPTUFJTl9BVFRSX01TSV9NQVBQSU5HOg0KPiA+IC0JCS8qIER1 bW15IGhhbmRsaW5nIGFkZGVkICovDQo+ID4gKwkJbXNpX21hcHMgPSBkYXRhOw0KPiA+ICsNCj4g PiArCQltc2lfbWFwcy0+YXV0b21hcCA9IGZhbHNlOw0KPiA+ICsJCW1zaV9tYXBzLT5vdmVycmlk ZV9hdXRvbWFwID0gdHJ1ZTsNCj4gPiArDQo+ID4gIAkJcmV0dXJuIDA7DQo+ID4gIAlkZWZhdWx0 Og0KPiA+ICAJCXJldHVybiAtRU5PREVWOw0KPiANCj4gSW4gcHJldmlvdXMgZGlzY3Vzc2lvbnMg SSB1bmRlcnN0b29kIG9uZSBvZiB0aGUgcHJvYmxlbXMgeW91IHdlcmUgdHJ5aW5nIHRvDQo+IHNv bHZlIHdhcyBoYXZpbmcgYSBsaW1pdGVkIG51bWJlciBvZiBNU0kgYmFua3MgYW5kIHdoaWxlIHlv dSBtYXkgYmUgYWJsZQ0KPiB0byBnZXQgaXNvbGF0ZWQgTVNJIGJhbmtzIGZvciBzb21lIG51bWJl ciBvZiB1c2VycywgaXQgd2Fzbid0IHVubGltaXRlZCBhbmQNCj4gc2hhcmluZyBtYXkgYmUgcmVx dWlyZWQuICBJIGRvbid0IHNlZSBhbnkgb2YgdGhhdCBhZGRyZXNzZWQgaW4gdGhpcyBzZXJpZXMu DQoNClRoYXQgcHJvYmxlbSB3YXMgb24gUG93ZXJQQy4gSW5mYWN0IHRoZXJlIHdlcmUgdHdvIHBy b2JsZW1zLCBvbmUgd2hpY2ggTVNJIGJhbmsgdG8gYmUgdXNlZCBhbmQgc2Vjb25kIGhvdyB0byBj cmVhdGUgaW9tbXUtbWFwcGluZyBmb3IgZGV2aWNlIGFzc2lnbmVkIHRvIHVzZXJzcGFjZS4NCkZp cnN0IHByb2JsZW0gd2FzIFBvd2VyUEMgc3BlY2lmaWMgYW5kIHRoYXQgd2lsbCBiZSBzb2x2ZWQg c2VwYXJhdGVseS4NCkZvciBzZWNvbmQgcHJvYmxlbSwgZWFybGllciBJIHRyaWVkIHRvIGFkZGVk IGEgY291cGxlIG9mIE1TSSBzcGVjaWZpYyBpb2N0bHMgYW5kIHlvdSBzdWdnZXN0ZWQgKElJVUMp IHRoYXQgd2Ugc2hvdWxkIGhhdmUgYSBnZW5lcmljIHJlc2VydmVkLWlvdmEgdHlwZSBvZiBBUEkg YW5kIHRoZW4gd2UgY2FuIG1hcCBNU0kgYmFuayB1c2luZyByZXNlcnZlZC1pb3ZhIGFuZCB0aGlz IHdpbGwgbm90IHJlcXVpcmUgaW52b2x2ZW1lbnQgb2YgdXNlci1zcGFjZS4NCg0KPiANCj4gQWxz bywgdGhlIG1hbmFnZW1lbnQgb2YgcmVzZXJ2ZWQgSU9WQXMgdnMgTVNJIGFkZHJlc3NlcyBsb29r cyByZWFsbHkNCj4gZHViaW91cyB0byBtZS4gIEhvdyBkb2VzIHlvdXIgcGxhdGZvcm0gcGljayBh biBNU0kgYWRkcmVzcyBhbmQgd2hhdCBhcmUNCj4gd2UgYnJlYWtpbmcgYnkgY292ZXJ0bHkgY2hh bmdpbmcgaXQ/ICBXZSBzZWVtIHRvIGJlIG1hc2tpbmcgb3ZlciBhdCB0aGUNCj4gVkZJTyBsZXZl bCwgd2hlcmUgdGhlcmUgc2hvdWxkIGJlIGxvd2VyIGxldmVsIGludGVyZmFjZXMgZG9pbmcgdGhl IHJpZ2h0IHRoaW5nDQo+IHdoZW4gd2UgY29uZmlndXJlIE1TSSBvbiB0aGUgZGV2aWNlLg0KDQpZ ZXMsIEluIG15IHVuZGVyc3RhbmRpbmcgdGhlIHJpZ2h0IHNvbHV0aW9uIHNob3VsZCBiZToNCiAx KSBWRklPIGRyaXZlciBzaG91bGQga25vdyB3aGF0IHBoeXNpY2FsLW1zaS1hZGRyZXNzIHdpbGwg YmUgdXNlZCBmb3IgZGV2aWNlcyBpbiBhbiBpb21tdS1ncm91cC4NCiAgICBJIGRpZCBub3QgZmlu ZCBhbiBnZW5lcmljIEFQSSwgb24gUG93ZXJQQyBJIGFkZGVkIHNvbWUgZnVuY3Rpb24gaW4gZmZy ZXNjYWxlIG1zaS1kcml2ZXIgYW5kIGNhbGxlZCBmcm9tIHZmaW8taW9tbXUtZnNsLXBhbXUuYyAo bm90IHlldCB1cHN0cmVhbWVkKS4NCiAyKSBWRklPIGRyaXZlciBzaG91bGQga25vdyB3aGF0IElP VkEgdG8gYmUgdXNlZCBmb3IgY3JlYXRpbmcgaW9tbXUtbWFwcGluZyAoVkZJTyBBUElzIHBhdGNo IG9mIHRoaXMgcGF0Y2ggc2VyaWVzKQ0KIDMpIFZGSU8gZHJpdmVyIHdpbGwgY3JlYXRlIHRoZSBp b21tdS1tYXBwaW5nIHVzaW5nICgxKSBhbmQgKDIpDQogNCkgVkZJTyBkcml2ZXIgc2hvdWxkIGJl IGFibGUgdG8gdGVsbCB0aGUgbXNpLWRyaXZlciB0aGF0IGZvciBhIGdpdmVuIGRldmljZSBpdCBz aG91bGQgdXNlIGRpZmZlcmVudCBJT1ZBLiBTbyB3aGVuIGNvbXBvc2luZyB0aGUgbXNpIG1lc3Nh Z2UgKGZvciB0aGUgZGV2aWNlcyBpcyB0aGUgZ2l2ZW4gaW9tbXUtZ3JvdXApIGl0IHNob3VsZCB1 c2UgdGhhdCBwcm9ncmFtbWVkIGlvdmEgYXMgTVNJLWFkZHJlc3MuIFRoaXMgaW50ZXJmYWNlIGFs c28gbmVlZGVkIHRvIGJlIGRldmVsb3BlZC4NCg0KSSB3YXMgbm90IHN1cmUgb2Ygd2hpY2ggYXBw cm9hY2ggd2Ugc2hvdWxkIHRha2UuIFRoZSBjdXJyZW50IGFwcHJvYWNoIGluIHRoZSBwYXRjaCBp cyBzaW1wbGUgdG8gZGV2ZWxvcCBzbyBJIHdlbnQgYWhlYWQgdG8gdGFrZSBpbnB1dCBidXQgSSBh Z3JlZSB0aGlzIGRvZXMgbm90IGxvb2sgdmVyeSBnb29kLg0KV2hhdCBkbyB5b3UgdGhpbmssIHNo b3VsZCBkcm9wIHRoaXMgYXBwcm9hY2ggYW5kIHdvcmsgb3V0IHRoZSBhcHByb2FjaCBhcyBkZXNj cmliZWQgYWJvdmUuDQoNClRoYW5rcw0KLUJoYXJhdA0KPiANCj4gVGhlIHByb2JsZW0gb2YgcmVw b3J0aW5nICJhdXRvbWFwIiBiYXNlIGFkZHJlc3MgaXNuJ3QgYWRkcmVzc2VkIG1vcmUgdGhhbg0K PiBsZWF2aW5nIHNvbWUgdW51c2VkIGZpZWxkIGluIGlvbW11X2RvbWFpbl9tc2lfbWFwcy4NCg0K -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-10-05 at 08:33 +0000, Bhushan Bharat wrote: > > > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Saturday, October 03, 2015 4:17 AM > > To: Bhushan Bharat-R65777 <Bharat.Bhushan@freescale.com> > > Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; > > christoffer.dall@linaro.org; eric.auger@linaro.org; pranavkumar@linaro.org; > > marc.zyngier@arm.com; will.deacon@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.Bhushan@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 MSI vectors as reserved IOVA space (but it's standard on x86, so an x86 VM user will never use it for IOVA). In your model, the MSI IOVA space is programmable, but it has page granularity (I assume). Therefore we shouldn't be sharing that page with anyone. That seems to suggest we need to allocate a page of vector space from the host kernel, setup the IOVA mapping, and then the host kernel should know to only allocate MSI vectors for these devices from that pre-allocated page. Otherwise we need to call the interrupts unsafe, like we do on x86 without interrupt remapping. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Tuesday, October 06, 2015 4:25 AM > To: Bhushan Bharat-R65777 <Bharat.Bhushan@freescale.com> > Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; > christoffer.dall@linaro.org; eric.auger@linaro.org; pranavkumar@linaro.org; > marc.zyngier@arm.com; will.deacon@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 +0000, Bhushan Bharat wrote: > > > > > > > -----Original Message----- > > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > > Sent: Saturday, October 03, 2015 4:17 AM > > > To: Bhushan Bharat-R65777 <Bharat.Bhushan@freescale.com> > > > Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; > > > christoffer.dall@linaro.org; eric.auger@linaro.org; > > > pranavkumar@linaro.org; marc.zyngier@arm.com; will.deacon@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.Bhushan@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 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. 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). 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. Thanks -Bharat > but it has page > granularity (I assume). Therefore we shouldn't be sharing that page with > anyone. That seems to suggest we need to allocate a page of vector space > from the host kernel, setup the IOVA mapping, and then the host kernel > should know to only allocate MSI vectors for these devices from that pre- > allocated page. Otherwise we need to call the interrupts unsafe, like we do > on x86 without interrupt remapping. Thanks, > > Alex
On Tue, Oct 06, 2015 at 10:26:34AM +0000, 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bharat, On 6 October 2015 at 15:56, Bhushan Bharat <Bharat.Bhushan@freescale.com> wrote: > > >> -----Original Message----- >> From: Alex Williamson [mailto:alex.williamson@redhat.com] >> Sent: Tuesday, October 06, 2015 4:25 AM >> To: Bhushan Bharat-R65777 <Bharat.Bhushan@freescale.com> >> Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; >> christoffer.dall@linaro.org; eric.auger@linaro.org; pranavkumar@linaro.org; >> marc.zyngier@arm.com; will.deacon@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 +0000, Bhushan Bharat wrote: >> > >> > >> > > -----Original Message----- >> > > From: Alex Williamson [mailto:alex.williamson@redhat.com] >> > > Sent: Saturday, October 03, 2015 4:17 AM >> > > To: Bhushan Bharat-R65777 <Bharat.Bhushan@freescale.com> >> > > Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; >> > > christoffer.dall@linaro.org; eric.auger@linaro.org; >> > > pranavkumar@linaro.org; marc.zyngier@arm.com; will.deacon@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.Bhushan@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 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. I think reserving MSI page is tricky as in arm/arm64 in MSI controllers like GICv2M: 1. We have number of interrupts mapped to one physical MSI address. 2. Two root complexes can have same MSI controller this means two EP devices on two different RCs can have same MSI address programmed (and will have different data to generate different MSI interrupt) 3. This means reserving of a MSI physical address is difficult since we can have one EP device from one RC assigned to a VM and another EP device can be used by host linux or by a different VM. One way to get msi physical address which I have tried in my patch series was: 1. Let linux host controller assigned MSI address (by calling request_irq) which current VFIO PCI driver does, and then extract this address. 2. Map this extracted address with an IOVA address. > > 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). I am not sure what is the problem here ? as userspace is going to tell us which IOVA is to be used. Do you mean - 1. userspace is going to provide us a range of IOVA and we will choose one of them to map it with a physical MSI address. 2. Now how to tell userspace which IOVA we have used ??????? > > 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. > > Thanks > -Bharat > >> but it has page >> granularity (I assume). Therefore we shouldn't be sharing that page with >> anyone. That seems to suggest we need to allocate a page of vector space >> from the host kernel, setup the IOVA mapping, and then the host kernel >> should know to only allocate MSI vectors for these devices from that pre- >> allocated page. Otherwise we need to call the interrupts unsafe, like we do >> on x86 without interrupt remapping. Thanks, >> >> Alex > Thanks, Pranav -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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.Bhushan@freescale.com> --- drivers/iommu/arm-smmu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)