Re: [PATCH V2 3/7] iommu/vt-d: Support Enhanced Command Interface

[Liang, Kan]

On 2023-01-19 3:55 a.m., Baolu Lu wrote:
> On 2023/1/19 4:50, kan.liang@xxxxxxxxxxxxxxx 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