Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Julien Grall <julien.grall@arm.com>]

(+ Lars)

Hi,

On 8/7/19 5:01 PM, Oleksandr wrote:
>>> + * you can found at:
>>> + *    url: 
>>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git
>>> + *    branch: v4.14.75-ltsi/rcar-3.9.6
>>> + *    commit: e206eb5b81a60e64c35fbc3a999b1a0db2b98044
>>> + * and Xen's SMMU driver:
>>> + *    xen/drivers/passthrough/arm/smmu.c
>>> + *
>>> + * Copyright (C) 2016-2019 EPAM Systems Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms and conditions of the GNU General Public
>>> + * License, version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public
>>> + * License along with this program; If not, see 
>>> <http://www.gnu.org/licenses/>.
>> I don't know that Xen license description rule, but since a few source 
>> files have
>> SPDX-License-Identifier, can we also use it on the driver?
> 
> I am afraid, I don't know a correct answer for this question. I would 
> leave this to maintainers.
> 
> I just followed sample copyright notice for GPL v2 License according to 
> the document:
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=CONTRIBUTING

The file CONTRIBUTING is only giving example of common example of 
license. So I think this is fine to use SPDX, the more they are already 
used. The only request is to put either SDPX or the full-blown text but 
not the two :). Lars, any objection?

I am quite in favor of SPDX because it is easier to find out the 
license. With the full-blown text, the text may slightly vary between 
licenses. For instance, the only difference between GPLv2 and GPLv2+ is 
",or (at your option) any later version". I let you imagine how it can 
be easy to miss it when reviewing ;).

We had a discussion last year about using SPDX in Xen code base but I 
never got the time to formally suggest it.

> 
>>> + */
>>> +
>>> +#include <xen/delay.h>
>>> +#include <xen/err.h>
>>> +#include <xen/irq.h>
>>> +#include <xen/lib.h>
>>> +#include <xen/list.h>
>> I don't know that Xen passthrough driver rule though, doesn't here need
>> #include <xen/iommu.h>? (The xen/sched.h seems to have it so that
>> no compile error happens though.)
> 
> Probably, yes, I should have included that header.

I am fine either way :). The indirect inclusion happens quite often and 
we only notice it when someone decide to rework the headers.

[...]
>> +/* Xen IOMMU ops */
>>> +static int __must_check ipmmu_iotlb_flush_all(struct domain *d)
>>> +{
>>> +    struct ipmmu_vmsa_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
>>> +
>>> +    if ( !xen_domain || !xen_domain->root_domain )
>>> +        return 0;
>>> +
>>> +    spin_lock(&xen_domain->lock);
>> Is local irq is already disabled here?
>> If no, you should use spin_lock_irqsave() because the ipmmu_irq() also
>> gets the lock.
> 
> 
> No, it is not disabled. But, ipmmu_irq() uses another mmu->lock. So, I 
> think, there won't be a deadlock.
> 
> Or I really missed something?
> 
> If we worry about ipmmu_tlb_invalidate() which is called here (to 
> perform a flush by request from P2M code, which manages a page table) 
> and from the irq handler (to perform a flush to resume address 
> translation), I could use a tasklet to schedule ipmmu_tlb_invalidate() 
> from the irq handler then. This way we would get this serialized. What 
> do you think?

I am afraid a tasklet is not an option. You need to perform the TLB 
flush when requested otherwise you are introducing a security issue.

This is because as soon as a region is unmapped in the page table, we 
remove the drop the reference on any page backing that region. When the 
reference is dropped to zero, the page can be reallocated to another 
domain or even Xen. If the TLB flush happen after, then the guest may 
still be able to access the page for a short time if the translation has 
been cached by the any TLB (IOMMU, Processor).

[...]

>>> +    /*
>>> +     * Destroy Root IPMMU domain which context is mapped to this Xen 
>>> domain
>>> +     * if exits.
>>> +     */
>>> +    if ( xen_domain->root_domain )
>>> +        ipmmu_free_root_domain(xen_domain->root_domain);
>>> +
>>> +    spin_unlock(&xen_domain->lock);
>>> +
>>> +    /*
>>> +     * We assume that all master devices have already been detached 
>>> from
>>> +     * this Xen domain and there must be no associated Cache IPMMU 
>>> domains
>>> +     * in use.
>>> +     */
>>> +    ASSERT(list_empty(&xen_domain->cache_domains));
>> I think this should be in the spin lock held by &xen_domain->lock.
> 
> OK. Will put spin_unlock after it.

The spin_lock is actually pointless here. This is done when the domain 
is destroyed, so nobody should touch it.

If you think concurrent access can still happen, then you are going to 
be in deep trouble as you free the xen_domain (and therefore the 
spinlock) below :).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel