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 <eric.auger@xxxxxxxxxx>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> 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?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
