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: <dd035701-62f0-0a68-ed4d-5347d26753f8@linux.intel.com>
Date:   Fri, 13 Jan 2023 13:19:59 -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 3/7] iommu/vt-d: Support Enhanced Command Interface
Content-Language: en-US
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: <20230111202504.378258-1-kan.liang@linux.intel.com>
 <20230111202504.378258-4-kan.liang@linux.intel.com>
 <6783f737-2701-77e3-ce83-8d287be51cf6@linux.intel.com>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <6783f737-2701-77e3-ce83-8d287be51cf6@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023-01-13 8:55 a.m., Baolu Lu wrote:
> On 2023/1/12 4:25, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> The Enhanced Command Register is to submit command and operand of
>> enhanced commands to DMA Remapping hardware. It can supports upto 256
>> enhanced commands.
>>
>> There is a HW register to indicate the availability of all 256 enhanced
>> commands. Each bit stands for each command. But there isn't an existing
>> interface to read/write all 256 bits. Introduce the u64 ecmdcap[4] to
>> store the existence of each enhanced command. Read 4 times to get
>> all of them in map_iommu().
>>
>> Add a helper to facilitate an enhanced command launch. Make sure hardware
>> complete the command.
>>
>> Add a helper to facilitate the check of PMU essentials.
>>
>> The helpers will be used later.
>>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>   drivers/iommu/intel/dmar.c  | 63 +++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/intel/iommu.h | 46 +++++++++++++++++++++++++++
>>   2 files changed, 109 insertions(+)
>>
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index 91bb48267df2..dcafa32875c1 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -1025,6 +1025,16 @@ static int map_iommu(struct intel_iommu *iommu,
>> struct dmar_drhd_unit *drhd)
>>               goto release;
>>           }
>>       }
>> +
>> +    if (cap_ecmds(iommu->cap)) {
>> +        int i;
>> +
>> +        for (i = 0; i < DMA_MAX_NUM_ECMDCAP; i++) {
>> +            iommu->ecmdcap[i] = dmar_readq(iommu->reg + DMAR_ECCAP_REG +
>> +                               i * DMA_ECMD_REG_STEP);
>> +        }
>> +    }
>> +
>>       err = 0;
>>       goto out;
>>   @@ -2434,3 +2444,56 @@ bool dmar_platform_optin(void)
>>       return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(dmar_platform_optin);
>> +
>> +#ifdef CONFIG_INTEL_IOMMU
> 
> Why do you need above #ifdef?
> 
>> +#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, bool has_ob, 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;
>> +    }
>> +
>> +    if (has_ob)
>> +        dmar_writeq(iommu->reg + DMAR_ECEO_REG, ob);
> 
> The ecmds that require a Operand B are statically defined in the spec,
> right? What will it look like if we define a static ignore_ob(ecmd)?
> 
>> +    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 */
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 5bcefbea55c9..f227107434ac 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -130,6 +130,10 @@
>>   #define DMAR_PERFOVFOFF_REG    0x318
>>   #define DMAR_PERFCNTROFF_REG    0x31c
>>   #define DMAR_PERFEVNTCAP_REG    0x380
>> +#define DMAR_ECMD_REG        0x400
>> +#define DMAR_ECEO_REG        0x408
>> +#define DMAR_ECRSP_REG        0x410
>> +#define DMAR_ECCAP_REG        0x430
>>   #define DMAR_VCCAP_REG        0xe30 /* Virtual command capability
>> register */
>>   #define DMAR_VCMD_REG        0xe00 /* Virtual command register */
>>   #define DMAR_VCRSP_REG        0xe10 /* Virtual command response
>> register */
>> @@ -304,6 +308,26 @@
>>   #define DMA_CCMD_SID(s) (((u64)((s) & 0xffff)) << 16)
>>   #define DMA_CCMD_DID(d) ((u64)((d) & 0xffff))
>>   +/* ECMD_REG */
>> +#define DMA_MAX_NUM_ECMD        256
>> +#define DMA_MAX_NUM_ECMDCAP        (DMA_MAX_NUM_ECMD / 64)
>> +#define DMA_ECMD_REG_STEP        8
>> +#define DMA_ECMD_ENABLE            0xf0
>> +#define DMA_ECMD_DISABLE        0xf1
>> +#define DMA_ECMD_FREEZE            0xf4
>> +#define DMA_ECMD_UNFREEZE        0xf5
>> +#define DMA_ECMD_OA_SHIFT        16
>> +#define DMA_ECMD_ECRSP_IP        0x1
>> +#define DMA_ECMD_ECCAP3            3
>> +#define DMA_ECMD_ECCAP3_ECNTS        (1ULL << 48)
>> +#define DMA_ECMD_ECCAP3_DCNTS        (1ULL << 49)
>> +#define DMA_ECMD_ECCAP3_FCNTS        (1ULL << 52)
>> +#define DMA_ECMD_ECCAP3_UFCNTS        (1ULL << 53)
>> +#define DMA_ECMD_ECCAP3_ESSENTIAL    (DMA_ECMD_ECCAP3_ECNTS |    \
>> +                     DMA_ECMD_ECCAP3_DCNTS |    \
>> +                     DMA_ECMD_ECCAP3_FCNTS |    \
>> +                     DMA_ECMD_ECCAP3_UFCNTS)
>> +
>>   /* FECTL_REG */
>>   #define DMA_FECTL_IM (((u32)1) << 31)
>>   @@ -600,6 +624,7 @@ struct intel_iommu {
>>       u64        cap;
>>       u64        ecap;
>>       u64        vccap;
>> +    u64        ecmdcap[DMA_MAX_NUM_ECMDCAP];
>>       u32        gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
>>       raw_spinlock_t    register_lock; /* protect register handling */
>>       int        seq_id;    /* sequence id of the iommu */
>> @@ -841,6 +866,15 @@ extern const struct iommu_ops intel_iommu_ops;
>>   extern int intel_iommu_sm;
>>   extern int iommu_calculate_agaw(struct intel_iommu *iommu);
>>   extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
>> +extern int ecmd_submit_sync(struct intel_iommu *iommu, u8 ecmd,
>> +                u64 oa, bool has_ob, u64 ob);
>> +
>> +static inline bool ecmd_has_pmu_essential(struct intel_iommu *iommu)
>> +{
>> +    return (iommu->ecmdcap[DMA_ECMD_ECCAP3] &
>> DMA_ECMD_ECCAP3_ESSENTIAL) ==
>> +        DMA_ECMD_ECCAP3_ESSENTIAL;
>> +}
>> +
>>   extern int dmar_disabled;
>>   extern int intel_iommu_enabled;
>>   #else
>> @@ -852,6 +886,18 @@ static inline int
>> iommu_calculate_max_sagaw(struct intel_iommu *iommu)
>>   {
>>       return 0;
>>   }
>> +
>> +static inline int ecmd_submit_sync(struct intel_iommu *iommu, u8 ecmd,
>> +                   u64 oa, bool has_ob, u64 ob)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static inline bool ecmd_has_pmu_essential(struct intel_iommu *iommu)
>> +{
>> +    return false;
>> +}
> 
> Is there any need to call these helpers when INTEL_IOMMU is not
> configured?


No, I will remove them from non-INTEL_IOMMU.

Thanks,
Kan

> 
>> +
>>   #define dmar_disabled    (1)
>>   #define intel_iommu_enabled (0)
>>   #define intel_iommu_sm (0)
> 
> -- 
> Best regards,
> baolu