Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b9619bee-7f5c-81db-710d-20550ea01517@linux.intel.com>
Date:   Thu, 19 Jan 2023 08:24:25 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH V2 3/7] iommu/vt-d: Support Enhanced Command Interface
To:     Baolu Lu <baolu.lu@linux.intel.com>, joro@8bytes.org,
        will@kernel.org, dwmw2@infradead.org, robin.murphy@arm.com,
        robert.moore@intel.com, rafael.j.wysocki@intel.com,
        lenb@kernel.org, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20230118205006.3182907-1-kan.liang@linux.intel.com>
 <20230118205006.3182907-4-kan.liang@linux.intel.com>
 <379823f4-d1c3-8943-4afe-e33f4d21785c@linux.intel.com>
Content-Language: en-US
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <379823f4-d1c3-8943-4afe-e33f4d21785c@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023-01-19 3:55 a.m., Baolu Lu wrote:
> On 2023/1/19 4:50, kan.liang@linux.intel.com wrote:
>> +#ifdef CONFIG_INTEL_IOMMU
>> +#define ecmd_get_status_code(res)    ((res & 0xff) >> 1)
>> +
>> +/*
>> + * Function to submit a command to the enhanced command interface. The
>> + * valid enhanced command descriptions are defined in Table 47 of the
>> + * VT-d spec. The VT-d hardware implementation may support some but not
>> + * all commands, which can be determined by checking the Enhanced
>> + * Command Capability Register.
>> + *
>> + * Return values:
>> + *  - 0: Command successful without any error;
>> + *  - Negative: software error value;
>> + *  - Nonzero positive: failure status code defined in Table 48.
>> + */
>> +int ecmd_submit_sync(struct intel_iommu *iommu, u8 ecmd, u64 oa, u64 ob)
>> +{
>> +    unsigned long flags;
>> +    u64 res;
>> +    int ret;
>> +
>> +    if (!cap_ecmds(iommu->cap))
>> +        return -ENODEV;
>> +
>> +    raw_spin_lock_irqsave(&iommu->register_lock, flags);
>> +
>> +    res = dmar_readq(iommu->reg + DMAR_ECRSP_REG);
>> +    if (res & DMA_ECMD_ECRSP_IP) {
>> +        ret = -EBUSY;
>> +        goto err;
>> +    }
>> +
>> +    /*
>> +     * Unconditionally write the operand B, because
>> +     * - There is no side effect if an ecmd doesn't require an
>> +     *   operand B, but we set the register to some value.
>> +     * - It's not invoked in any critical path. The extra MMIO
>> +     *   write doesn't bring any performance concerns.
>> +     */
>> +    dmar_writeq(iommu->reg + DMAR_ECEO_REG, ob);
>> +    dmar_writeq(iommu->reg + DMAR_ECMD_REG, ecmd | (oa <<
>> DMA_ECMD_OA_SHIFT));
>> +
>> +    IOMMU_WAIT_OP(iommu, DMAR_ECRSP_REG, dmar_readq,
>> +              !(res & DMA_ECMD_ECRSP_IP), res);
>> +
>> +    if (res & DMA_ECMD_ECRSP_IP) {
>> +        ret = -ETIMEDOUT;
>> +        goto err;
>> +    }
>> +
>> +    ret = ecmd_get_status_code(res);
>> +err:
>> +    raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
>> +
>> +    return ret;
>> +}
>> +#endif /* CONFIG_INTEL_IOMMU */
> 
> Can we remove the "#ifdef CONFIG_INTEL_IOMMU"? 

In dmar.c, no, there will be a compiler warning when the
CONFIG_INTEL_IOMMU is not set.

> Or if this is currently
> only intel-iommu specific, how about moving it to drivers/iommu/intel
> /iommu.c?
> 

Yes, it should OK to move it to iommu.c to avoid the "#ifdef
CONFIG_INTEL_IOMMU". Now, it's intel-iommu specific.

Thanks,
Kan