Re: [Xen-devel] [PATCH V2 4/6] iommu/arm: Add lightweight iommu_fwspec support

[Oleksandr <olekstysh@gmail.com>]

On 13.08.19 15:39, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien.


>
> On 8/2/19 5:39 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to have some abstract way to add new device to the IOMMU
>> based on the generic IOMMU DT binding [1] which can be used for
>> both DT (right now) and ACPI (in future).
>>
>> For that reason we can borrow the idea used in Linux these days
>> called "iommu_fwspec". Having this in, it will be possible
>> to configure IOMMU master interfaces of the device (device IDs)
>> from a single common place and avoid keeping almost identifical look-up
>
> s/identifical/identical/

ok


>
>> implementations in each IOMMU driver.
>>
>> There is no need to port the whole implementation of "iommu_fwspec"
>> to Xen, we could, probably, end up with a much simpler solution,
>> some "stripped down" version which fits our requirments.
>
> s/requirments/requirements/

ok


>
>> + */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/iommu.h>
>
> Please order the headers alphabetically.
>
> NIT: Can you a newline between xen and asm headers?

Will do


>
>> +#include <asm/device.h>
>> +#include <asm/iommu_fwspec.h>
>
>> +
>> +int iommu_fwspec_init(struct device *dev, struct device *iommu_dev)
>> +{
>> +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +
>> +    if ( fwspec )
>> +        return 0;
>> +
>> +    fwspec = xzalloc(struct iommu_fwspec);
>> +    if ( !fwspec )
>> +        return -ENOMEM;
>> +
>> +    fwspec->iommu_dev = iommu_dev;
>> +    dev_iommu_fwspec_set(dev, fwspec);
>> +
>> +    return 0;
>> +}
>> +
>> +void iommu_fwspec_free(struct device *dev)
>> +{
>> +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +
>> +    if ( fwspec )
>
> xfree is able to deal with NULL pointer, so the check is not necessary.

Yes, the reason I left this check is to not perform an extra operation 
(dev_iommu_fwspec_set). Shall I drop this check anyway?

>
>> +    {
>> +        xfree(fwspec);
>> +        dev_iommu_fwspec_set(dev, NULL);
>> +    }
>> +}
>> +
>> +int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids, int 
>> num_ids)
>
> While I realize the prototype is coming from Linux, num_ids cannot be 
> negative (the code below would not work properly). So the parameter 
> should be unsigned.

Agree, will use unsigned.


>
>> +{
>> +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +    size_t size;
>> +    int i;
>
> Any variable that can't be negative should be unsigned.

Yes, will follow.


>> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
>> index 20d865e..1853bd9 100644
>> --- a/xen/include/asm-arm/iommu.h
>> +++ b/xen/include/asm-arm/iommu.h
>> @@ -14,6 +14,8 @@
>>   #ifndef __ARCH_ARM_IOMMU_H__
>>   #define __ARCH_ARM_IOMMU_H__
>>   +#include <asm/iommu_fwspec.h>
>
> iommu.h does not seem to use anything defined in iommu_fwspec.h. So 
> why do you include it here?

I was thinking that every source which includes iommu.h will get 
iommu_fwspec.h included indirectly. No need to include iommu_fwspec.h in 
many sources.

This was a reason. Shall I included it directly where needed?


>
>> +
>>   struct arch_iommu
>>   {
>>       /* Private information for the IOMMU drivers */
>> diff --git a/xen/include/asm-arm/iommu_fwspec.h 
>> b/xen/include/asm-arm/iommu_fwspec.h
>> new file mode 100644
>> index 0000000..0676285
>> --- /dev/null
>> +++ b/xen/include/asm-arm/iommu_fwspec.h
>> @@ -0,0 +1,65 @@
>> +/*
>> + * xen/include/asm-arm/iommu_fwspec.h
>> + *
>> + * Contains a common structure to hold the per-device firmware data and
>> + * declaration of functions used to maintain that data
>> + *
>> + * Based on Linux's iommu_fwspec support you can find at:
>> + *    include/linux/iommu.h
>> + *
>> + * Copyright (C) 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/>.
>> + */
>> +
>> +#ifndef __ARCH_ARM_IOMMU_FWSPEC_H__
>> +#define __ARCH_ARM_IOMMU_FWSPEC_H__
>> +
>> +/* per-device IOMMU instance data */
>> +struct iommu_fwspec {
>> +    /* device which represents this IOMMU H/W */
>
> Did you intend to say "this device's IOMMU"?

Exactly)


-- 
Regards,

Oleksandr Tyshchenko


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