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

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi, Julien.


> > 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.
I tried to locate files in Xen where SPDX is used. After finding only 
nospec.h I got an incorrect opinion this is not popular in Xen))

Just to clarify. So the title for the driver should be the following (if 
there are no objections):
// SPDX-License-Identifier: GPL-2.0
/*
 * xen/drivers/passthrough/arm/ipmmu-vmsa.c
 *
 * Driver for the Renesas IPMMU-VMSA found in R-Car Gen3 SoCs.
 *
 * Copyright (C) 2014-2019 Renesas Electronics Corporation
 *
 * The IPMMU-VMSA is VMSA-compatible I/O Memory Management Unit (IOMMU)
 * which provides address translation and access protection functionalities
 * to processing units and interconnect networks.
 *
 * Please note, current driver is supposed to work only with newest 
R-Car Gen3
 * SoCs revisions which IPMMU hardware supports stage 2 translation 
table format
 * and is able to use CPU's P2M table as is.
 *
 * Based on Linux's IPMMU-VMSA driver from Renesas BSP:
 *    drivers/iommu/ipmmu-vmsa.c
 * 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.
 */


Answer to myself:

Looks like, the same I have to do with all newly added files in this 
series (iommu_fwspec, etc).

> > > > +    /*
> > > > +     * 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 :).
Indeed, this is pointless. We don't really expect any other operations 
with the domain which is being destroyed. No assign/deassign devices, no 
flush, no map, nothing...

>
--
Regards,

Oleksandr Tyshchenko


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