From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762647AbdAEMJG (ORCPT ); Thu, 5 Jan 2017 07:09:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53424 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757802AbdAEMIs (ORCPT ); Thu, 5 Jan 2017 07:08:48 -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> <1e881510-bc18-ae97-ec2f-feadcba54150@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: Date: Thu, 5 Jan 2017 13:08:40 +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.27]); Thu, 05 Jan 2017 12:08:49 +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:57, Marc Zyngier wrote: > On 05/01/17 11:29, Auger Eric wrote: >> 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. > > Ah, sorry - I blame the ARM coffee. > > This function builds a domain for a single device on top of the MSI > domain that has been already created (see the dev->msi_domain passed to > irq_domain_create_hierarchy). The structure looks like this: > > device-domain -> platform MSI domain -> HW MSI domain -> whatever > > So what we're *really* interested in is the platform MSI domain, which > is going to carry the IRQ_DOMAIN_FLAG_MSI flag. The device-domain only > describes a portion of it, and can safely be ignored. > > In the end, what matters for this patch is that we can prove that from > any domain carrying the IRQ_DOMAIN_FLAG_MSI flag, we can find a domain > carrying the IRQ_DOMAIN_FLAG_MSI_REMAP flag. If that property holds, > we're safe. Otherwise, we disable the Guest MSI feature. > > Does it make sense? Yes it makes sense. Thank you for the explanation! Eric > > 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 13:08:40 +0100 Message-ID: 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> <1e881510-bc18-ae97-ec2f-feadcba54150@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:57, Marc Zyngier wrote: > On 05/01/17 11:29, Auger Eric wrote: >> 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. > > Ah, sorry - I blame the ARM coffee. > > This function builds a domain for a single device on top of the MSI > domain that has been already created (see the dev->msi_domain passed to > irq_domain_create_hierarchy). The structure looks like this: > > device-domain -> platform MSI domain -> HW MSI domain -> whatever > > So what we're *really* interested in is the platform MSI domain, which > is going to carry the IRQ_DOMAIN_FLAG_MSI flag. The device-domain only > describes a portion of it, and can safely be ignored. > > In the end, what matters for this patch is that we can prove that from > any domain carrying the IRQ_DOMAIN_FLAG_MSI flag, we can find a domain > carrying the IRQ_DOMAIN_FLAG_MSI_REMAP flag. If that property holds, > we're safe. Otherwise, we disable the Guest MSI feature. > > Does it make sense? Yes it makes sense. Thank you for the explanation! Eric > > Thanks, > > M. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@redhat.com (Auger Eric) Date: Thu, 5 Jan 2017 13:08:40 +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> <1e881510-bc18-ae97-ec2f-feadcba54150@redhat.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Marc, On 05/01/2017 12:57, Marc Zyngier wrote: > On 05/01/17 11:29, Auger Eric wrote: >> 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. > > Ah, sorry - I blame the ARM coffee. > > This function builds a domain for a single device on top of the MSI > domain that has been already created (see the dev->msi_domain passed to > irq_domain_create_hierarchy). The structure looks like this: > > device-domain -> platform MSI domain -> HW MSI domain -> whatever > > So what we're *really* interested in is the platform MSI domain, which > is going to carry the IRQ_DOMAIN_FLAG_MSI flag. The device-domain only > describes a portion of it, and can safely be ignored. > > In the end, what matters for this patch is that we can prove that from > any domain carrying the IRQ_DOMAIN_FLAG_MSI flag, we can find a domain > carrying the IRQ_DOMAIN_FLAG_MSI_REMAP flag. If that property holds, > we're safe. Otherwise, we disable the Guest MSI feature. > > Does it make sense? Yes it makes sense. Thank you for the explanation! Eric > > Thanks, > > M. >