Re: [PATCH v2 4/8] xen/arm: Remove support for MSI on SMMUv3

[Rahul Singh <Rahul.Singh@arm.com>]

Hello Stefano,

> On 2 Dec 2020, at 12:40 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 1 Dec 2020, Stefano Stabellini wrote:
>> On Thu, 26 Nov 2020, Rahul Singh wrote:
>>> XEN does not support MSI on ARM platforms, therefore remove the MSI
>>> support from SMMUv3 driver.
>>> 
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> 
>> I wonder if it makes sense to #ifdef CONFIG_MSI this code instead of
>> removing it completely.
> 
> One more thought: could this patch be achieved by reverting
> 166bdbd23161160f2abcea70621adba179050bee ? If this patch could be done
> by a couple of revert, it would be great to say it in the commit
> message.
> 
 Ok will add in next version.

> 
>> In the past, we tried to keep the entire file as textually similar to
>> the original Linux driver as possible to make it easier to backport
>> features and fixes. So, in this case we would probably not even used an
>> #ifdef but maybe something like:
>> 
>>  if (/* msi_enabled */ 0)
>>      return;
>> 
>> at the beginning of arm_smmu_setup_msis.
>> 
>> 
>> However, that strategy didn't actually work very well because backports
>> have proven difficult to do anyway. So at that point we might as well at
>> least have clean code in Xen and do the changes properly.

Main reason to remove the feature/code that is not usable in XEN is to have a clean code.

Regards,
Rahul

>> 
>> So that's my reasoning for accepting this patch :-)
>> 
>> Julien, are you happy with this too?
>> 
>> 
>>> ---
>>> xen/drivers/passthrough/arm/smmu-v3.c | 176 +-------------------------
>>> 1 file changed, 3 insertions(+), 173 deletions(-)
>>> 
>>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
>>> index cec304e51a..401f7ae006 100644
>>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>>> @@ -416,31 +416,6 @@ enum pri_resp {
>>> 	PRI_RESP_SUCC = 2,
>>> };
>>> 
>>> -enum arm_smmu_msi_index {
>>> -	EVTQ_MSI_INDEX,
>>> -	GERROR_MSI_INDEX,
>>> -	PRIQ_MSI_INDEX,
>>> -	ARM_SMMU_MAX_MSIS,
>>> -};
>>> -
>>> -static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
>>> -	[EVTQ_MSI_INDEX] = {
>>> -		ARM_SMMU_EVTQ_IRQ_CFG0,
>>> -		ARM_SMMU_EVTQ_IRQ_CFG1,
>>> -		ARM_SMMU_EVTQ_IRQ_CFG2,
>>> -	},
>>> -	[GERROR_MSI_INDEX] = {
>>> -		ARM_SMMU_GERROR_IRQ_CFG0,
>>> -		ARM_SMMU_GERROR_IRQ_CFG1,
>>> -		ARM_SMMU_GERROR_IRQ_CFG2,
>>> -	},
>>> -	[PRIQ_MSI_INDEX] = {
>>> -		ARM_SMMU_PRIQ_IRQ_CFG0,
>>> -		ARM_SMMU_PRIQ_IRQ_CFG1,
>>> -		ARM_SMMU_PRIQ_IRQ_CFG2,
>>> -	},
>>> -};
>>> -
>>> struct arm_smmu_cmdq_ent {
>>> 	/* Common fields */
>>> 	u8				opcode;
>>> @@ -504,10 +479,6 @@ struct arm_smmu_cmdq_ent {
>>> 		} pri;
>>> 
>>> 		#define CMDQ_OP_CMD_SYNC	0x46
>>> -		struct {
>>> -			u32			msidata;
>>> -			u64			msiaddr;
>>> -		} sync;
>>> 	};
>>> };
>>> 
>>> @@ -649,12 +620,6 @@ struct arm_smmu_device {
>>> 
>>> 	struct arm_smmu_strtab_cfg	strtab_cfg;
>>> 
>>> -	/* Hi16xx adds an extra 32 bits of goodness to its MSI payload */
>>> -	union {
>>> -		u32			sync_count;
>>> -		u64			padding;
>>> -	};
>>> -
>>> 	/* IOMMU core code handle */
>>> 	struct iommu_device		iommu;
>>> };
>>> @@ -945,20 +910,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>>> 		cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
>>> 		break;
>>> 	case CMDQ_OP_CMD_SYNC:
>>> -		if (ent->sync.msiaddr)
>>> -			cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ);
>>> -		else
>>> -			cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
>>> -		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH);
>>> -		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
>>> -		/*
>>> -		 * Commands are written little-endian, but we want the SMMU to
>>> -		 * receive MSIData, and thus write it back to memory, in CPU
>>> -		 * byte order, so big-endian needs an extra byteswap here.
>>> -		 */
>>> -		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIDATA,
>>> -				     cpu_to_le32(ent->sync.msidata));
>>> -		cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
>>> +		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
>>> 		break;
>>> 	default:
>>> 		return -ENOENT;
>>> @@ -1054,50 +1006,6 @@ static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
>>> 	spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
>>> }
>>> 
>>> -/*
>>> - * The difference between val and sync_idx is bounded by the maximum size of
>>> - * a queue at 2^20 entries, so 32 bits is plenty for wrap-safe arithmetic.
>>> - */
>>> -static int __arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx)
>>> -{
>>> -	ktime_t timeout;
>>> -	u32 val;
>>> -
>>> -	timeout = ktime_add_us(ktime_get(), ARM_SMMU_CMDQ_SYNC_TIMEOUT_US);
>>> -	val = smp_cond_load_acquire(&smmu->sync_count,
>>> -				    (int)(VAL - sync_idx) >= 0 ||
>>> -				    !ktime_before(ktime_get(), timeout));
>>> -
>>> -	return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;
>>> -}
>>> -
>>> -static int __arm_smmu_cmdq_issue_sync_msi(struct arm_smmu_device *smmu)
>>> -{
>>> -	u64 cmd[CMDQ_ENT_DWORDS];
>>> -	unsigned long flags;
>>> -	struct arm_smmu_cmdq_ent  ent = {
>>> -		.opcode = CMDQ_OP_CMD_SYNC,
>>> -		.sync	= {
>>> -			.msiaddr = virt_to_phys(&smmu->sync_count),
>>> -		},
>>> -	};
>>> -
>>> -	spin_lock_irqsave(&smmu->cmdq.lock, flags);
>>> -
>>> -	/* Piggy-back on the previous command if it's a SYNC */
>>> -	if (smmu->prev_cmd_opcode == CMDQ_OP_CMD_SYNC) {
>>> -		ent.sync.msidata = smmu->sync_nr;
>>> -	} else {
>>> -		ent.sync.msidata = ++smmu->sync_nr;
>>> -		arm_smmu_cmdq_build_cmd(cmd, &ent);
>>> -		arm_smmu_cmdq_insert_cmd(smmu, cmd);
>>> -	}
>>> -
>>> -	spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
>>> -
>>> -	return __arm_smmu_sync_poll_msi(smmu, ent.sync.msidata);
>>> -}
>>> -
>>> static int __arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
>>> {
>>> 	u64 cmd[CMDQ_ENT_DWORDS];
>>> @@ -1119,12 +1027,9 @@ static int __arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
>>> static int arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
>>> {
>>> 	int ret;
>>> -	bool msi = (smmu->features & ARM_SMMU_FEAT_MSI) &&
>>> -		   (smmu->features & ARM_SMMU_FEAT_COHERENCY);
>>> 
>>> -	ret = msi ? __arm_smmu_cmdq_issue_sync_msi(smmu)
>>> -		  : __arm_smmu_cmdq_issue_sync(smmu);
>>> -	if (ret)
>>> +	ret = __arm_smmu_cmdq_issue_sync(smmu);
>>> +	if ( ret )
>>> 		dev_err_ratelimited(smmu->dev, "CMD_SYNC timeout\n");
>>> 	return ret;
>>> }
>>> @@ -2898,83 +2803,10 @@ static int arm_smmu_update_gbpa(struct arm_smmu_device *smmu, u32 set, u32 clr)
>>> 	return ret;
>>> }
>>> 
>>> -static void arm_smmu_free_msis(void *data)
>>> -{
>>> -	struct device *dev = data;
>>> -	platform_msi_domain_free_irqs(dev);
>>> -}
>>> -
>>> -static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>>> -{
>>> -	phys_addr_t doorbell;
>>> -	struct device *dev = msi_desc_to_dev(desc);
>>> -	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>> -	phys_addr_t *cfg = arm_smmu_msi_cfg[desc->platform.msi_index];
>>> -
>>> -	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>>> -	doorbell &= MSI_CFG0_ADDR_MASK;
>>> -
>>> -	writeq_relaxed(doorbell, smmu->base + cfg[0]);
>>> -	writel_relaxed(msg->data, smmu->base + cfg[1]);
>>> -	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>>> -}
>>> -
>>> -static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>> -{
>>> -	struct msi_desc *desc;
>>> -	int ret, nvec = ARM_SMMU_MAX_MSIS;
>>> -	struct device *dev = smmu->dev;
>>> -
>>> -	/* Clear the MSI address regs */
>>> -	writeq_relaxed(0, smmu->base + ARM_SMMU_GERROR_IRQ_CFG0);
>>> -	writeq_relaxed(0, smmu->base + ARM_SMMU_EVTQ_IRQ_CFG0);
>>> -
>>> -	if (smmu->features & ARM_SMMU_FEAT_PRI)
>>> -		writeq_relaxed(0, smmu->base + ARM_SMMU_PRIQ_IRQ_CFG0);
>>> -	else
>>> -		nvec--;
>>> -
>>> -	if (!(smmu->features & ARM_SMMU_FEAT_MSI))
>>> -		return;
>>> -
>>> -	if (!dev->msi_domain) {
>>> -		dev_info(smmu->dev, "msi_domain absent - falling back to wired irqs\n");
>>> -		return;
>>> -	}
>>> -
>>> -	/* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */
>>> -	ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);
>>> -	if (ret) {
>>> -		dev_warn(dev, "failed to allocate MSIs - falling back to wired irqs\n");
>>> -		return;
>>> -	}
>>> -
>>> -	for_each_msi_entry(desc, dev) {
>>> -		switch (desc->platform.msi_index) {
>>> -		case EVTQ_MSI_INDEX:
>>> -			smmu->evtq.q.irq = desc->irq;
>>> -			break;
>>> -		case GERROR_MSI_INDEX:
>>> -			smmu->gerr_irq = desc->irq;
>>> -			break;
>>> -		case PRIQ_MSI_INDEX:
>>> -			smmu->priq.q.irq = desc->irq;
>>> -			break;
>>> -		default:	/* Unknown */
>>> -			continue;
>>> -		}
>>> -	}
>>> -
>>> -	/* Add callback to free MSIs on teardown */
>>> -	devm_add_action(dev, arm_smmu_free_msis, dev);
>>> -}
>>> -
>>> static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>> {
>>> 	int irq, ret;
>>> 
>>> -	arm_smmu_setup_msis(smmu);
>>> -
>>> 	/* Request interrupt lines */
>>> 	irq = smmu->evtq.q.irq;
>>> 	if (irq) {
>>> @@ -3250,8 +3082,6 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>>> 	if (reg & IDR0_SEV)
>>> 		smmu->features |= ARM_SMMU_FEAT_SEV;
>>> 
>>> -	if (reg & IDR0_MSI)
>>> -		smmu->features |= ARM_SMMU_FEAT_MSI;
>>> 
>>> 	if (reg & IDR0_HYP)
>>> 		smmu->features |= ARM_SMMU_FEAT_HYP;
>>> -- 
>>> 2.17.1
>>> 
>>