From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968854AbdAELaC (ORCPT ); Thu, 5 Jan 2017 06:30:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40926 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761361AbdAEL3x (ORCPT ); Thu, 5 Jan 2017 06:29:53 -0500 Subject: Re: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap To: Marc Zyngier , 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 References: <1483536746-2725-1-git-send-email-eric.auger@redhat.com> <1483536746-2725-14-git-send-email-eric.auger@redhat.com> <92fcb464-3364-c432-29d8-1c2ce4a5f110@redhat.com> Cc: drjones@redhat.com, kvm@vger.kernel.org, punit.agrawal@arm.com, linux-kernel@vger.kernel.org, geethasowjanya.akula@gmail.com, diana.craciun@nxp.com, iommu@lists.linux-foundation.org, pranav.sawargaonkar@gmail.com, bharat.bhushan@nxp.com, shankerd@codeaurora.org, gpkulkarni@gmail.com From: Auger Eric Message-ID: <1e881510-bc18-ae97-ec2f-feadcba54150@redhat.com> Date: Thu, 5 Jan 2017 12:29:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 05 Jan 2017 11:29:54 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Marc, On 05/01/2017 12:25, Marc Zyngier wrote: > On 05/01/17 10:45, Auger Eric wrote: >> Hi Marc, >> >> On 04/01/2017 16:27, Marc Zyngier wrote: >>> On 04/01/17 14:11, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> On 04/01/2017 14:46, Marc Zyngier wrote: >>>>> Hi Eric, >>>>> >>>>> On 04/01/17 13:32, Eric Auger wrote: >>>>>> This new function checks whether all platform and PCI >>>>>> MSI domains implement IRQ remapping. This is useful to >>>>>> understand whether VFIO passthrough is safe with respect >>>>>> to interrupts. >>>>>> >>>>>> On ARM typically an MSI controller can sit downstream >>>>>> to the IOMMU without preventing VFIO passthrough. >>>>>> As such any assigned device can write into the MSI doorbell. >>>>>> In case the MSI controller implements IRQ remapping, assigned >>>>>> devices will not be able to trigger interrupts towards the >>>>>> host. On the contrary, the assignment must be emphasized as >>>>>> unsafe with respect to interrupts. >>>>>> >>>>>> Signed-off-by: Eric Auger >>>>>> >>>>>> --- >>>>>> >>>>>> v4 -> v5: >>>>>> - Handle DOMAIN_BUS_FSL_MC_MSI domains >>>>>> - Check parents >>>>>> --- >>>>>> include/linux/irqdomain.h | 1 + >>>>>> kernel/irq/irqdomain.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 42 insertions(+) >>>>>> >>>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>>>>> index ab017b2..281a40f 100644 >>>>>> --- a/include/linux/irqdomain.h >>>>>> +++ b/include/linux/irqdomain.h >>>>>> @@ -219,6 +219,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node, >>>>>> void *host_data); >>>>>> extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> enum irq_domain_bus_token bus_token); >>>>>> +extern bool irq_domain_check_msi_remap(void); >>>>>> extern void irq_set_default_host(struct irq_domain *host); >>>>>> extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs, >>>>>> irq_hw_number_t hwirq, int node, >>>>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>>>>> index 8c0a0ae..700caea 100644 >>>>>> --- a/kernel/irq/irqdomain.c >>>>>> +++ b/kernel/irq/irqdomain.c >>>>>> @@ -278,6 +278,47 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> EXPORT_SYMBOL_GPL(irq_find_matching_fwspec); >>>>>> >>>>>> /** >>>>>> + * irq_domain_is_msi_remap - Check if @domain or any parent >>>>>> + * has MSI remapping support >>>>>> + * @domain: domain pointer >>>>>> + */ >>>>>> +static bool irq_domain_is_msi_remap(struct irq_domain *domain) >>>>>> +{ >>>>>> + struct irq_domain *h = domain; >>>>>> + >>>>>> + for (; h; h = h->parent) { >>>>>> + if (h->flags & IRQ_DOMAIN_FLAG_MSI_REMAP) >>>>>> + return true; >>>>>> + } >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * irq_domain_check_msi_remap() - Checks whether all MSI >>>>>> + * irq domains implement IRQ remapping >>>>>> + */ >>>>>> +bool irq_domain_check_msi_remap(void) >>>>>> +{ >>>>>> + struct irq_domain *h; >>>>>> + bool ret = true; >>>>>> + >>>>>> + mutex_lock(&irq_domain_mutex); >>>>>> + list_for_each_entry(h, &irq_domain_list, link) { >>>>>> + if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>>>>> + !irq_domain_is_msi_remap(h)) { >>>>> >>>>> (h->bus_token & DOMAIN_BUS_PCI_MSI) and co looks quite wrong. bus_token >>>>> is not a bitmap, and DOMAIN_BUS_* not a single bit value (see enum >>>>> irq_domain_bus_token). Surely this should read >>>>> (h->bus_token == DOMAIN_BUS_PCI_MSI). >>>> Oh I did not notice that. Thanks. >>>> >>>> Any other comments on the irqdomain side? Do you think the current >>>> approach consisting in looking at those bus tokens and their parents >>>> looks good? >>> >>> To be completely honest, I don't like it much, as having to enumerate >>> all the bus types can come up with could become quite a burden in the >>> long run. I'd rather be able to identify MSI capable domains by >>> construction. I came up with the following approach (fully untested): >>> >>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>> index 281a40f..7779796 100644 >>> --- a/include/linux/irqdomain.h >>> +++ b/include/linux/irqdomain.h >>> @@ -183,8 +183,11 @@ enum { >>> /* Irq domain is an IPI domain with single virq */ >>> IRQ_DOMAIN_FLAG_IPI_SINGLE = (1 << 3), >>> >>> + /* Irq domain implements MSIs */ >>> + IRQ_DOMAIN_FLAG_MSI = (1 << 4), >>> + >>> /* Irq domain is MSI remapping capable */ >>> - IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 4), >>> + IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 5), >>> >>> /* >>> * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved >>> @@ -450,6 +453,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return domain->flags & IRQ_DOMAIN_FLAG_IPI_SINGLE; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return domain->flags & IRQ_DOMAIN_FLAG_MSI; >>> +} >>> #else /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> static inline void irq_domain_activate_irq(struct irq_data *data) { } >>> static inline void irq_domain_deactivate_irq(struct irq_data *data) { } >>> @@ -481,6 +489,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return false; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return false; >>> +} >>> #endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> >>> #else /* CONFIG_IRQ_DOMAIN */ >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 700caea..33b6921 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -304,10 +304,7 @@ bool irq_domain_check_msi_remap(void) >>> >>> mutex_lock(&irq_domain_mutex); >>> list_for_each_entry(h, &irq_domain_list, link) { >>> - if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>> - (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>> - (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>> - !irq_domain_is_msi_remap(h)) { >>> + if (irq_domain_is_msi(h) && !irq_domain_is_msi_remap(h)) { >>> ret = false; >>> goto out; >>> } >>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c >>> index ee23006..b637263 100644 >>> --- a/kernel/irq/msi.c >>> +++ b/kernel/irq/msi.c >>> @@ -270,7 +270,7 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, >>> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) >>> msi_domain_update_chip_ops(info); >>> >>> - return irq_domain_create_hierarchy(parent, 0, 0, fwnode, >>> + return irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, fwnode, >>> &msi_domain_ops, info); >>> } >>> >>> >>> >>> Thoughts? >> >> Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in >> platform_msi_create_device_domain too (drivers/base/platform-msi.c)? was mentioning platform_msi_create_*device*_domain. it calls irq_domain_create_hierarchy and looks to be MSI irq domain related. But I don't have a full understanding of the whole irq domain hierarchy. Thanks Eric > > Well, platform_msi_create_irq_domain does call msi_create_irq_domain, > doesn't it? That's one of the benefits of the generic MSI > infrastructure: it is the only function that performs the creation of an > MSI domain for any bus type. > > Or am I missing something completely obvious (which is perfectly > possible since I only had a couple of cups of the brown stuff...)? > > Thanks, > > M. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Auger Eric Subject: Re: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap Date: Thu, 5 Jan 2017 12:29:47 +0100 Message-ID: <1e881510-bc18-ae97-ec2f-feadcba54150@redhat.com> References: <1483536746-2725-1-git-send-email-eric.auger@redhat.com> <1483536746-2725-14-git-send-email-eric.auger@redhat.com> <92fcb464-3364-c432-29d8-1c2ce4a5f110@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, punit.agrawal-5wv7dgnIgG8@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, geethasowjanya.akula-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, gpkulkarni-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org To: Marc Zyngier , eric.auger.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, robin.murphy-5wv7dgnIgG8@public.gmane.org, alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org, tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: kvm.vger.kernel.org Hi Marc, On 05/01/2017 12:25, Marc Zyngier wrote: > On 05/01/17 10:45, Auger Eric wrote: >> Hi Marc, >> >> On 04/01/2017 16:27, Marc Zyngier wrote: >>> On 04/01/17 14:11, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> On 04/01/2017 14:46, Marc Zyngier wrote: >>>>> Hi Eric, >>>>> >>>>> On 04/01/17 13:32, Eric Auger wrote: >>>>>> This new function checks whether all platform and PCI >>>>>> MSI domains implement IRQ remapping. This is useful to >>>>>> understand whether VFIO passthrough is safe with respect >>>>>> to interrupts. >>>>>> >>>>>> On ARM typically an MSI controller can sit downstream >>>>>> to the IOMMU without preventing VFIO passthrough. >>>>>> As such any assigned device can write into the MSI doorbell. >>>>>> In case the MSI controller implements IRQ remapping, assigned >>>>>> devices will not be able to trigger interrupts towards the >>>>>> host. On the contrary, the assignment must be emphasized as >>>>>> unsafe with respect to interrupts. >>>>>> >>>>>> Signed-off-by: Eric Auger >>>>>> >>>>>> --- >>>>>> >>>>>> v4 -> v5: >>>>>> - Handle DOMAIN_BUS_FSL_MC_MSI domains >>>>>> - Check parents >>>>>> --- >>>>>> include/linux/irqdomain.h | 1 + >>>>>> kernel/irq/irqdomain.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 42 insertions(+) >>>>>> >>>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>>>>> index ab017b2..281a40f 100644 >>>>>> --- a/include/linux/irqdomain.h >>>>>> +++ b/include/linux/irqdomain.h >>>>>> @@ -219,6 +219,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node, >>>>>> void *host_data); >>>>>> extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> enum irq_domain_bus_token bus_token); >>>>>> +extern bool irq_domain_check_msi_remap(void); >>>>>> extern void irq_set_default_host(struct irq_domain *host); >>>>>> extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs, >>>>>> irq_hw_number_t hwirq, int node, >>>>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>>>>> index 8c0a0ae..700caea 100644 >>>>>> --- a/kernel/irq/irqdomain.c >>>>>> +++ b/kernel/irq/irqdomain.c >>>>>> @@ -278,6 +278,47 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> EXPORT_SYMBOL_GPL(irq_find_matching_fwspec); >>>>>> >>>>>> /** >>>>>> + * irq_domain_is_msi_remap - Check if @domain or any parent >>>>>> + * has MSI remapping support >>>>>> + * @domain: domain pointer >>>>>> + */ >>>>>> +static bool irq_domain_is_msi_remap(struct irq_domain *domain) >>>>>> +{ >>>>>> + struct irq_domain *h = domain; >>>>>> + >>>>>> + for (; h; h = h->parent) { >>>>>> + if (h->flags & IRQ_DOMAIN_FLAG_MSI_REMAP) >>>>>> + return true; >>>>>> + } >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * irq_domain_check_msi_remap() - Checks whether all MSI >>>>>> + * irq domains implement IRQ remapping >>>>>> + */ >>>>>> +bool irq_domain_check_msi_remap(void) >>>>>> +{ >>>>>> + struct irq_domain *h; >>>>>> + bool ret = true; >>>>>> + >>>>>> + mutex_lock(&irq_domain_mutex); >>>>>> + list_for_each_entry(h, &irq_domain_list, link) { >>>>>> + if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>>>>> + !irq_domain_is_msi_remap(h)) { >>>>> >>>>> (h->bus_token & DOMAIN_BUS_PCI_MSI) and co looks quite wrong. bus_token >>>>> is not a bitmap, and DOMAIN_BUS_* not a single bit value (see enum >>>>> irq_domain_bus_token). Surely this should read >>>>> (h->bus_token == DOMAIN_BUS_PCI_MSI). >>>> Oh I did not notice that. Thanks. >>>> >>>> Any other comments on the irqdomain side? Do you think the current >>>> approach consisting in looking at those bus tokens and their parents >>>> looks good? >>> >>> To be completely honest, I don't like it much, as having to enumerate >>> all the bus types can come up with could become quite a burden in the >>> long run. I'd rather be able to identify MSI capable domains by >>> construction. I came up with the following approach (fully untested): >>> >>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>> index 281a40f..7779796 100644 >>> --- a/include/linux/irqdomain.h >>> +++ b/include/linux/irqdomain.h >>> @@ -183,8 +183,11 @@ enum { >>> /* Irq domain is an IPI domain with single virq */ >>> IRQ_DOMAIN_FLAG_IPI_SINGLE = (1 << 3), >>> >>> + /* Irq domain implements MSIs */ >>> + IRQ_DOMAIN_FLAG_MSI = (1 << 4), >>> + >>> /* Irq domain is MSI remapping capable */ >>> - IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 4), >>> + IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 5), >>> >>> /* >>> * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved >>> @@ -450,6 +453,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return domain->flags & IRQ_DOMAIN_FLAG_IPI_SINGLE; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return domain->flags & IRQ_DOMAIN_FLAG_MSI; >>> +} >>> #else /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> static inline void irq_domain_activate_irq(struct irq_data *data) { } >>> static inline void irq_domain_deactivate_irq(struct irq_data *data) { } >>> @@ -481,6 +489,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return false; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return false; >>> +} >>> #endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> >>> #else /* CONFIG_IRQ_DOMAIN */ >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 700caea..33b6921 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -304,10 +304,7 @@ bool irq_domain_check_msi_remap(void) >>> >>> mutex_lock(&irq_domain_mutex); >>> list_for_each_entry(h, &irq_domain_list, link) { >>> - if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>> - (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>> - (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>> - !irq_domain_is_msi_remap(h)) { >>> + if (irq_domain_is_msi(h) && !irq_domain_is_msi_remap(h)) { >>> ret = false; >>> goto out; >>> } >>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c >>> index ee23006..b637263 100644 >>> --- a/kernel/irq/msi.c >>> +++ b/kernel/irq/msi.c >>> @@ -270,7 +270,7 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, >>> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) >>> msi_domain_update_chip_ops(info); >>> >>> - return irq_domain_create_hierarchy(parent, 0, 0, fwnode, >>> + return irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, fwnode, >>> &msi_domain_ops, info); >>> } >>> >>> >>> >>> Thoughts? >> >> Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in >> platform_msi_create_device_domain too (drivers/base/platform-msi.c)? was mentioning platform_msi_create_*device*_domain. it calls irq_domain_create_hierarchy and looks to be MSI irq domain related. But I don't have a full understanding of the whole irq domain hierarchy. Thanks Eric > > Well, platform_msi_create_irq_domain does call msi_create_irq_domain, > doesn't it? That's one of the benefits of the generic MSI > infrastructure: it is the only function that performs the creation of an > MSI domain for any bus type. > > Or am I missing something completely obvious (which is perfectly > possible since I only had a couple of cups of the brown stuff...)? > > Thanks, > > M. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@redhat.com (Auger Eric) Date: Thu, 5 Jan 2017 12:29:47 +0100 Subject: [PATCH v5 13/17] irqdomain: irq_domain_check_msi_remap In-Reply-To: References: <1483536746-2725-1-git-send-email-eric.auger@redhat.com> <1483536746-2725-14-git-send-email-eric.auger@redhat.com> <92fcb464-3364-c432-29d8-1c2ce4a5f110@redhat.com> Message-ID: <1e881510-bc18-ae97-ec2f-feadcba54150@redhat.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Marc, On 05/01/2017 12:25, Marc Zyngier wrote: > On 05/01/17 10:45, Auger Eric wrote: >> Hi Marc, >> >> On 04/01/2017 16:27, Marc Zyngier wrote: >>> On 04/01/17 14:11, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> On 04/01/2017 14:46, Marc Zyngier wrote: >>>>> Hi Eric, >>>>> >>>>> On 04/01/17 13:32, Eric Auger wrote: >>>>>> This new function checks whether all platform and PCI >>>>>> MSI domains implement IRQ remapping. This is useful to >>>>>> understand whether VFIO passthrough is safe with respect >>>>>> to interrupts. >>>>>> >>>>>> On ARM typically an MSI controller can sit downstream >>>>>> to the IOMMU without preventing VFIO passthrough. >>>>>> As such any assigned device can write into the MSI doorbell. >>>>>> In case the MSI controller implements IRQ remapping, assigned >>>>>> devices will not be able to trigger interrupts towards the >>>>>> host. On the contrary, the assignment must be emphasized as >>>>>> unsafe with respect to interrupts. >>>>>> >>>>>> Signed-off-by: Eric Auger >>>>>> >>>>>> --- >>>>>> >>>>>> v4 -> v5: >>>>>> - Handle DOMAIN_BUS_FSL_MC_MSI domains >>>>>> - Check parents >>>>>> --- >>>>>> include/linux/irqdomain.h | 1 + >>>>>> kernel/irq/irqdomain.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 42 insertions(+) >>>>>> >>>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>>>>> index ab017b2..281a40f 100644 >>>>>> --- a/include/linux/irqdomain.h >>>>>> +++ b/include/linux/irqdomain.h >>>>>> @@ -219,6 +219,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node, >>>>>> void *host_data); >>>>>> extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> enum irq_domain_bus_token bus_token); >>>>>> +extern bool irq_domain_check_msi_remap(void); >>>>>> extern void irq_set_default_host(struct irq_domain *host); >>>>>> extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs, >>>>>> irq_hw_number_t hwirq, int node, >>>>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>>>>> index 8c0a0ae..700caea 100644 >>>>>> --- a/kernel/irq/irqdomain.c >>>>>> +++ b/kernel/irq/irqdomain.c >>>>>> @@ -278,6 +278,47 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec, >>>>>> EXPORT_SYMBOL_GPL(irq_find_matching_fwspec); >>>>>> >>>>>> /** >>>>>> + * irq_domain_is_msi_remap - Check if @domain or any parent >>>>>> + * has MSI remapping support >>>>>> + * @domain: domain pointer >>>>>> + */ >>>>>> +static bool irq_domain_is_msi_remap(struct irq_domain *domain) >>>>>> +{ >>>>>> + struct irq_domain *h = domain; >>>>>> + >>>>>> + for (; h; h = h->parent) { >>>>>> + if (h->flags & IRQ_DOMAIN_FLAG_MSI_REMAP) >>>>>> + return true; >>>>>> + } >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * irq_domain_check_msi_remap() - Checks whether all MSI >>>>>> + * irq domains implement IRQ remapping >>>>>> + */ >>>>>> +bool irq_domain_check_msi_remap(void) >>>>>> +{ >>>>>> + struct irq_domain *h; >>>>>> + bool ret = true; >>>>>> + >>>>>> + mutex_lock(&irq_domain_mutex); >>>>>> + list_for_each_entry(h, &irq_domain_list, link) { >>>>>> + if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>>>>> + (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>>>>> + !irq_domain_is_msi_remap(h)) { >>>>> >>>>> (h->bus_token & DOMAIN_BUS_PCI_MSI) and co looks quite wrong. bus_token >>>>> is not a bitmap, and DOMAIN_BUS_* not a single bit value (see enum >>>>> irq_domain_bus_token). Surely this should read >>>>> (h->bus_token == DOMAIN_BUS_PCI_MSI). >>>> Oh I did not notice that. Thanks. >>>> >>>> Any other comments on the irqdomain side? Do you think the current >>>> approach consisting in looking at those bus tokens and their parents >>>> looks good? >>> >>> To be completely honest, I don't like it much, as having to enumerate >>> all the bus types can come up with could become quite a burden in the >>> long run. I'd rather be able to identify MSI capable domains by >>> construction. I came up with the following approach (fully untested): >>> >>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>> index 281a40f..7779796 100644 >>> --- a/include/linux/irqdomain.h >>> +++ b/include/linux/irqdomain.h >>> @@ -183,8 +183,11 @@ enum { >>> /* Irq domain is an IPI domain with single virq */ >>> IRQ_DOMAIN_FLAG_IPI_SINGLE = (1 << 3), >>> >>> + /* Irq domain implements MSIs */ >>> + IRQ_DOMAIN_FLAG_MSI = (1 << 4), >>> + >>> /* Irq domain is MSI remapping capable */ >>> - IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 4), >>> + IRQ_DOMAIN_FLAG_MSI_REMAP = (1 << 5), >>> >>> /* >>> * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved >>> @@ -450,6 +453,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return domain->flags & IRQ_DOMAIN_FLAG_IPI_SINGLE; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return domain->flags & IRQ_DOMAIN_FLAG_MSI; >>> +} >>> #else /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> static inline void irq_domain_activate_irq(struct irq_data *data) { } >>> static inline void irq_domain_deactivate_irq(struct irq_data *data) { } >>> @@ -481,6 +489,11 @@ static inline bool irq_domain_is_ipi_single(struct irq_domain *domain) >>> { >>> return false; >>> } >>> + >>> +static inline bool irq_domain_is_msi(struct irq_domain *domain) >>> +{ >>> + return false; >>> +} >>> #endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */ >>> >>> #else /* CONFIG_IRQ_DOMAIN */ >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 700caea..33b6921 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -304,10 +304,7 @@ bool irq_domain_check_msi_remap(void) >>> >>> mutex_lock(&irq_domain_mutex); >>> list_for_each_entry(h, &irq_domain_list, link) { >>> - if (((h->bus_token & DOMAIN_BUS_PCI_MSI) || >>> - (h->bus_token & DOMAIN_BUS_PLATFORM_MSI) || >>> - (h->bus_token & DOMAIN_BUS_FSL_MC_MSI)) && >>> - !irq_domain_is_msi_remap(h)) { >>> + if (irq_domain_is_msi(h) && !irq_domain_is_msi_remap(h)) { >>> ret = false; >>> goto out; >>> } >>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c >>> index ee23006..b637263 100644 >>> --- a/kernel/irq/msi.c >>> +++ b/kernel/irq/msi.c >>> @@ -270,7 +270,7 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode, >>> if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) >>> msi_domain_update_chip_ops(info); >>> >>> - return irq_domain_create_hierarchy(parent, 0, 0, fwnode, >>> + return irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0, fwnode, >>> &msi_domain_ops, info); >>> } >>> >>> >>> >>> Thoughts? >> >> Don't we need to set the IRQ_DOMAIN_FLAG_MSI flag in >> platform_msi_create_device_domain too (drivers/base/platform-msi.c)? was mentioning platform_msi_create_*device*_domain. it calls irq_domain_create_hierarchy and looks to be MSI irq domain related. But I don't have a full understanding of the whole irq domain hierarchy. Thanks Eric > > Well, platform_msi_create_irq_domain does call msi_create_irq_domain, > doesn't it? That's one of the benefits of the generic MSI > infrastructure: it is the only function that performs the creation of an > MSI domain for any bus type. > > Or am I missing something completely obvious (which is perfectly > possible since I only had a couple of cups of the brown stuff...)? > > Thanks, > > M. >