linux-kernel - Re: [PATCH v2 2/3] mailbox: Add support for Hi3660 mailbox

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 Oct 2017 12:45:06 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Kaihua Zhong <zhongkaihua@...wei.com>, robh+dt@...nel.org,
        xuwei5@...ilicon.com, catalin.marinas@....com, will.deacon@....com,
        jassisinghbrar@...il.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        guodong.xu@...aro.org, haojian.zhuang@...aro.org,
        suzhuangluan@...ilicon.com, xuezhiliang@...ilicon.com,
        kevin.wangtao@...ilicon.com
Subject: Re: [PATCH v2 2/3] mailbox: Add support for Hi3660 mailbox

Hi Mark,

On Fri, Oct 27, 2017 at 11:46:00AM +0100, Mark Rutland wrote:
> On Fri, Oct 27, 2017 at 02:15:03PM +0800, Kaihua Zhong wrote:
> > Hi3660 mailbox controller is used to send message within multiple
> > processors, MCU, HIFI, etc.  It supports 32 mailbox channels and every
> > channel can only be used for single transferring direction.  Once the
> > channel is enabled, it needs to specify the destination interrupt and
> > acknowledge interrupt, these two interrupt vectors are used to create
> > the connection between the mailbox and interrupt controllers.
> > 
> > The application processor (or from point of view of kernel) is not the
> > only one master which can launch the data transferring, other
> > processors or MCU/DSP also can kick off the data transferring.  So this
> > driver implements a locking mechanism to support exclusive accessing.
> 
> ... and that locking mechanism is what precisely?
> 
> Where is the protocol defined?
> 
> > +static int hi3660_mbox_check_state(struct mbox_chan *chan)
> > +{
> > +	unsigned long ch = (unsigned long)chan->con_priv;
> > +	struct hi3660_mbox *mbox = to_hi3660_mbox(chan);
> > +	struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> > +	void __iomem *base = MBOX_BASE(mbox, ch);
> > +	unsigned long val;
> > +	unsigned int state, ret;
> > +
> > +	/* Mailbox is idle so directly bail out */
> > +	state = readl_relaxed(base + MBOX_MODE_REG);
> > +	if (state & MBOX_STATE_IDLE)
> > +		return 0;
> > +
> > +	/* Wait for acknowledge from remote */
> > +	ret = readx_poll_timeout_atomic(readl_relaxed, base + MBOX_MODE_REG,
> > +			val, (val & MBOX_STATE_ACK), 1000, 300000);
> > +	if (ret) {
> > +		dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
> > +		return ret;
> > +	}
> > +
> > +	/* Ensure channel is released */
> > +	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> > +	writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG);
> > +	__asm__ volatile ("sev");
> > +	return 0;
> > +}
> 
> Drivers really shouldn't be using SEV directly (even if via the sev() macro)...
> 
> This SEV isn't ordered w.r.t. anything, and it's unclear what ordering you
> need, so this simply does not work.

I will leave your questions for Hisilicon colleagues, essentially
your questions are related with mailbox mechanism.

But I'd like to firstly get clear your question for "This SEV isn't
ordered w.r.t. anything". From my understanding, ARMv8 architecture
natually adds DMB before SEV so all previous register writing
opreations should be ensured to endpoint before SEV?

[...]

Thanks,
Leo Yan