| Message ID | 1483643086-2883-18-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Eric, > -----Original Message----- > From: Eric Auger [mailto:eric.auger@redhat.com] > Sent: Friday, January 06, 2017 12:35 AM > To: eric.auger@redhat.com; eric.auger.pro@gmail.com; > christoffer.dall@linaro.org; marc.zyngier@arm.com; > robin.murphy@arm.com; alex.williamson@redhat.com; > will.deacon@arm.com; joro@8bytes.org; tglx@linutronix.de; > jason@lakedaemon.net; linux-arm-kernel@lists.infradead.org > Cc: kvm@vger.kernel.org; drjones@redhat.com; linux- > kernel@vger.kernel.org; pranav.sawargaonkar@gmail.com; > iommu@lists.linux-foundation.org; punit.agrawal@arm.com; Diana Madalina > Craciun <diana.craciun@nxp.com>; gpkulkarni@gmail.com; > shankerd@codeaurora.org; Bharat Bhushan <bharat.bhushan@nxp.com>; > geethasowjanya.akula@gmail.com > Subject: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq domain > level > > In case the IOMMU translates MSI transactions (typical case on ARM), we > check MSI remapping capability at IRQ domain level. Otherwise it is checked > at IOMMU level. > > At this stage the arm-smmu-(v3) still advertise the > IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be removed > in subsequent patches. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v6: rewrite test > --- > drivers/vfio/vfio_iommu_type1.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c > b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -40,6 +40,7 @@ > #include <linux/mdev.h> > #include <linux/notifier.h> > #include <linux/dma-iommu.h> > +#include <linux/irqdomain.h> > > #define DRIVER_VERSION "0.2" > #define DRIVER_AUTHOR "Alex Williamson > <alex.williamson@redhat.com>" > @@ -1208,7 +1209,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > struct vfio_domain *domain, *d; > struct bus_type *bus = NULL, *mdev_bus; > int ret; > - bool resv_msi; > + bool resv_msi, msi_remap; > phys_addr_t resv_msi_base; > > mutex_lock(&iommu->lock); > @@ -1284,8 +1285,10 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > INIT_LIST_HEAD(&domain->group_list); > list_add(&group->next, &domain->group_list); > > - if (!allow_unsafe_interrupts && > - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) { > + msi_remap = resv_msi ? irq_domain_check_msi_remap() : There can be multiple interrupt-controller, at-least theoretically it is possible and not sure practically it exists and supported, where not all may support IRQ_REMAP. If that is the case be then should we check for IRQ-REMAP for that device-bus irq-domain? Thanks -Bharat > + iommu_capable(bus, > IOMMU_CAP_INTR_REMAP); > + > + if (!allow_unsafe_interrupts && !msi_remap) { > pr_warn("%s: No interrupt remapping support. Use the > module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support > on this platform\n", > __func__); > ret = -EPERM; > -- > 1.9.1
Hi Bharat On 06/01/2017 09:50, Bharat Bhushan wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger [mailto:eric.auger@redhat.com] >> Sent: Friday, January 06, 2017 12:35 AM >> To: eric.auger@redhat.com; eric.auger.pro@gmail.com; >> christoffer.dall@linaro.org; marc.zyngier@arm.com; >> robin.murphy@arm.com; alex.williamson@redhat.com; >> will.deacon@arm.com; joro@8bytes.org; tglx@linutronix.de; >> jason@lakedaemon.net; linux-arm-kernel@lists.infradead.org >> Cc: kvm@vger.kernel.org; drjones@redhat.com; linux- >> kernel@vger.kernel.org; pranav.sawargaonkar@gmail.com; >> iommu@lists.linux-foundation.org; punit.agrawal@arm.com; Diana Madalina >> Craciun <diana.craciun@nxp.com>; gpkulkarni@gmail.com; >> shankerd@codeaurora.org; Bharat Bhushan <bharat.bhushan@nxp.com>; >> geethasowjanya.akula@gmail.com >> Subject: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq domain >> level >> >> In case the IOMMU translates MSI transactions (typical case on ARM), we >> check MSI remapping capability at IRQ domain level. Otherwise it is checked >> at IOMMU level. >> >> At this stage the arm-smmu-(v3) still advertise the >> IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be removed >> in subsequent patches. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v6: rewrite test >> --- >> drivers/vfio/vfio_iommu_type1.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c >> b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -40,6 +40,7 @@ >> #include <linux/mdev.h> >> #include <linux/notifier.h> >> #include <linux/dma-iommu.h> >> +#include <linux/irqdomain.h> >> >> #define DRIVER_VERSION "0.2" >> #define DRIVER_AUTHOR "Alex Williamson >> <alex.williamson@redhat.com>" >> @@ -1208,7 +1209,7 @@ static int vfio_iommu_type1_attach_group(void >> *iommu_data, >> struct vfio_domain *domain, *d; >> struct bus_type *bus = NULL, *mdev_bus; >> int ret; >> - bool resv_msi; >> + bool resv_msi, msi_remap; >> phys_addr_t resv_msi_base; >> >> mutex_lock(&iommu->lock); >> @@ -1284,8 +1285,10 @@ static int vfio_iommu_type1_attach_group(void >> *iommu_data, >> INIT_LIST_HEAD(&domain->group_list); >> list_add(&group->next, &domain->group_list); >> >> - if (!allow_unsafe_interrupts && >> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) { >> + msi_remap = resv_msi ? irq_domain_check_msi_remap() : > > There can be multiple interrupt-controller, at-least theoretically it is possible and not sure practically it exists and supported, where not all may support IRQ_REMAP. If that is the case be then should we check for IRQ-REMAP for that device-bus irq-domain? > I mentioned in the cover letter that the approach was defensive and rough today. As soon as we detect an MSI controller in the platform that has no support for MSI remapping we flag the assignment as unsafe. I think this approach was agreed on the ML. Such rough assessment was used in the past on x86. I am reluctant to add more complexity at that stage. This can be improved latter I think when such platforms show up. Best Regards Eric > Thanks > -Bharat > >> + iommu_capable(bus, >> IOMMU_CAP_INTR_REMAP); >> + >> + if (!allow_unsafe_interrupts && !msi_remap) { >> pr_warn("%s: No interrupt remapping support. Use the >> module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support >> on this platform\n", >> __func__); >> ret = -EPERM; >> -- >> 1.9.1 > > -- > 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 06/01/17 09:08, Auger Eric wrote: > Hi Bharat > > On 06/01/2017 09:50, Bharat Bhushan wrote: >> Hi Eric, >> >>> -----Original Message----- >>> From: Eric Auger [mailto:eric.auger@redhat.com] >>> Sent: Friday, January 06, 2017 12:35 AM >>> To: eric.auger@redhat.com; eric.auger.pro@gmail.com; >>> christoffer.dall@linaro.org; marc.zyngier@arm.com; >>> robin.murphy@arm.com; alex.williamson@redhat.com; >>> will.deacon@arm.com; joro@8bytes.org; tglx@linutronix.de; >>> jason@lakedaemon.net; linux-arm-kernel@lists.infradead.org >>> Cc: kvm@vger.kernel.org; drjones@redhat.com; linux- >>> kernel@vger.kernel.org; pranav.sawargaonkar@gmail.com; >>> iommu@lists.linux-foundation.org; punit.agrawal@arm.com; Diana Madalina >>> Craciun <diana.craciun@nxp.com>; gpkulkarni@gmail.com; >>> shankerd@codeaurora.org; Bharat Bhushan <bharat.bhushan@nxp.com>; >>> geethasowjanya.akula@gmail.com >>> Subject: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq domain >>> level >>> >>> In case the IOMMU translates MSI transactions (typical case on ARM), we >>> check MSI remapping capability at IRQ domain level. Otherwise it is checked >>> at IOMMU level. >>> >>> At this stage the arm-smmu-(v3) still advertise the >>> IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be removed >>> in subsequent patches. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> --- >>> >>> v6: rewrite test >>> --- >>> drivers/vfio/vfio_iommu_type1.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/vfio/vfio_iommu_type1.c >>> b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644 >>> --- a/drivers/vfio/vfio_iommu_type1.c >>> +++ b/drivers/vfio/vfio_iommu_type1.c >>> @@ -40,6 +40,7 @@ >>> #include <linux/mdev.h> >>> #include <linux/notifier.h> >>> #include <linux/dma-iommu.h> >>> +#include <linux/irqdomain.h> >>> >>> #define DRIVER_VERSION "0.2" >>> #define DRIVER_AUTHOR "Alex Williamson >>> <alex.williamson@redhat.com>" >>> @@ -1208,7 +1209,7 @@ static int vfio_iommu_type1_attach_group(void >>> *iommu_data, >>> struct vfio_domain *domain, *d; >>> struct bus_type *bus = NULL, *mdev_bus; >>> int ret; >>> - bool resv_msi; >>> + bool resv_msi, msi_remap; >>> phys_addr_t resv_msi_base; >>> >>> mutex_lock(&iommu->lock); >>> @@ -1284,8 +1285,10 @@ static int vfio_iommu_type1_attach_group(void >>> *iommu_data, >>> INIT_LIST_HEAD(&domain->group_list); >>> list_add(&group->next, &domain->group_list); >>> >>> - if (!allow_unsafe_interrupts && >>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) { >>> + msi_remap = resv_msi ? irq_domain_check_msi_remap() : >> >> There can be multiple interrupt-controller, at-least theoretically it is possible and not sure practically it exists and supported, where not all may support IRQ_REMAP. If that is the case be then should we check for IRQ-REMAP for that device-bus irq-domain? >> > I mentioned in the cover letter that the approach was defensive and > rough today. As soon as we detect an MSI controller in the platform that > has no support for MSI remapping we flag the assignment as unsafe. I > think this approach was agreed on the ML. Such rough assessment was used > in the past on x86. > > I am reluctant to add more complexity at that stage. This can be > improved latter I think when such platforms show up. I don't think this is worth it. If the system is so broken that the designer cannot make up their mind about device isolation, too bad. People will either disable the non-isolating MSI controller altogether, or force the unsafe flag. Thanks, M.
> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > Sent: Friday, January 06, 2017 2:50 PM > To: Auger Eric <eric.auger@redhat.com>; Bharat Bhushan > <bharat.bhushan@nxp.com>; eric.auger.pro@gmail.com; > christoffer.dall@linaro.org; robin.murphy@arm.com; > alex.williamson@redhat.com; will.deacon@arm.com; joro@8bytes.org; > tglx@linutronix.de; jason@lakedaemon.net; linux-arm- > kernel@lists.infradead.org > Cc: kvm@vger.kernel.org; drjones@redhat.com; linux- > kernel@vger.kernel.org; pranav.sawargaonkar@gmail.com; > iommu@lists.linux-foundation.org; punit.agrawal@arm.com; Diana Madalina > Craciun <diana.craciun@nxp.com>; gpkulkarni@gmail.com; > shankerd@codeaurora.org; geethasowjanya.akula@gmail.com > Subject: Re: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq > domain level > > On 06/01/17 09:08, Auger Eric wrote: > > Hi Bharat > > > > On 06/01/2017 09:50, Bharat Bhushan wrote: > >> Hi Eric, > >> > >>> -----Original Message----- > >>> From: Eric Auger [mailto:eric.auger@redhat.com] > >>> Sent: Friday, January 06, 2017 12:35 AM > >>> To: eric.auger@redhat.com; eric.auger.pro@gmail.com; > >>> christoffer.dall@linaro.org; marc.zyngier@arm.com; > >>> robin.murphy@arm.com; alex.williamson@redhat.com; > >>> will.deacon@arm.com; joro@8bytes.org; tglx@linutronix.de; > >>> jason@lakedaemon.net; linux-arm-kernel@lists.infradead.org > >>> Cc: kvm@vger.kernel.org; drjones@redhat.com; linux- > >>> kernel@vger.kernel.org; pranav.sawargaonkar@gmail.com; > >>> iommu@lists.linux-foundation.org; punit.agrawal@arm.com; Diana > >>> Madalina Craciun <diana.craciun@nxp.com>; gpkulkarni@gmail.com; > >>> shankerd@codeaurora.org; Bharat Bhushan > <bharat.bhushan@nxp.com>; > >>> geethasowjanya.akula@gmail.com > >>> Subject: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq > >>> domain level > >>> > >>> In case the IOMMU translates MSI transactions (typical case on ARM), > >>> we check MSI remapping capability at IRQ domain level. Otherwise it > >>> is checked at IOMMU level. > >>> > >>> At this stage the arm-smmu-(v3) still advertise the > >>> IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be > removed > >>> in subsequent patches. > >>> > >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >>> > >>> --- > >>> > >>> v6: rewrite test > >>> --- > >>> drivers/vfio/vfio_iommu_type1.c | 9 ++++++--- > >>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/vfio/vfio_iommu_type1.c > >>> b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644 > >>> --- a/drivers/vfio/vfio_iommu_type1.c > >>> +++ b/drivers/vfio/vfio_iommu_type1.c > >>> @@ -40,6 +40,7 @@ > >>> #include <linux/mdev.h> > >>> #include <linux/notifier.h> > >>> #include <linux/dma-iommu.h> > >>> +#include <linux/irqdomain.h> > >>> > >>> #define DRIVER_VERSION "0.2" > >>> #define DRIVER_AUTHOR "Alex Williamson > >>> <alex.williamson@redhat.com>" > >>> @@ -1208,7 +1209,7 @@ static int > vfio_iommu_type1_attach_group(void > >>> *iommu_data, > >>> struct vfio_domain *domain, *d; > >>> struct bus_type *bus = NULL, *mdev_bus; > >>> int ret; > >>> - bool resv_msi; > >>> + bool resv_msi, msi_remap; > >>> phys_addr_t resv_msi_base; > >>> > >>> mutex_lock(&iommu->lock); > >>> @@ -1284,8 +1285,10 @@ static int > vfio_iommu_type1_attach_group(void > >>> *iommu_data, > >>> INIT_LIST_HEAD(&domain->group_list); > >>> list_add(&group->next, &domain->group_list); > >>> > >>> - if (!allow_unsafe_interrupts && > >>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) { > >>> + msi_remap = resv_msi ? irq_domain_check_msi_remap() : > >> > >> There can be multiple interrupt-controller, at-least theoretically it is > possible and not sure practically it exists and supported, where not all may > support IRQ_REMAP. If that is the case be then should we check for IRQ- > REMAP for that device-bus irq-domain? > >> > > I mentioned in the cover letter that the approach was defensive and > > rough today. As soon as we detect an MSI controller in the platform > > that has no support for MSI remapping we flag the assignment as > > unsafe. I think this approach was agreed on the ML. Such rough > > assessment was used in the past on x86. > > > > I am reluctant to add more complexity at that stage. This can be > > improved latter I think when such platforms show up. > > I don't think this is worth it. If the system is so broken that the designer > cannot make up their mind about device isolation, too bad. > People will either disable the non-isolating MSI controller altogether, or force > the unsafe flag. Understand, just want to be sure that this is a known limitation. It will be good if we have some comment around this function. Thanks -Bharat > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -40,6 +40,7 @@ #include <linux/mdev.h> #include <linux/notifier.h> #include <linux/dma-iommu.h> +#include <linux/irqdomain.h> #define DRIVER_VERSION "0.2" #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" @@ -1208,7 +1209,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, struct vfio_domain *domain, *d; struct bus_type *bus = NULL, *mdev_bus; int ret; - bool resv_msi; + bool resv_msi, msi_remap; phys_addr_t resv_msi_base; mutex_lock(&iommu->lock); @@ -1284,8 +1285,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, INIT_LIST_HEAD(&domain->group_list); list_add(&group->next, &domain->group_list); - if (!allow_unsafe_interrupts && - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) { + msi_remap = resv_msi ? irq_domain_check_msi_remap() : + iommu_capable(bus, IOMMU_CAP_INTR_REMAP); + + if (!allow_unsafe_interrupts && !msi_remap) { pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n", __func__); ret = -EPERM;
In case the IOMMU translates MSI transactions (typical case on ARM), we check MSI remapping capability at IRQ domain level. Otherwise it is checked at IOMMU level. At this stage the arm-smmu-(v3) still advertise the IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be removed in subsequent patches. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v6: rewrite test --- drivers/vfio/vfio_iommu_type1.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)