From: Jens Wiklander <jens.wiklander@linaro.org>
To: Bertrand Marquis <Bertrand.Marquis@arm.com>
Cc: Julien Grall <julien@xen.org>,
Xen-devel <xen-devel@lists.xenproject.org>,
"patches@linaro.org" <patches@linaro.org>,
Stefano Stabellini <sstabellini@kernel.org>,
Michal Orzel <michal.orzel@amd.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
Date: Mon, 29 Apr 2024 11:49:54 +0200 [thread overview]
Message-ID: <CAHUa44HdmxV4oBeYu4rX89sFm7_NQcwza3Eotme9Z5g7EDRQew@mail.gmail.com> (raw)
In-Reply-To: <1A23083C-9605-4291-AF2F-5891AF270333@arm.com>
Hi Bertrand,
On Mon, Apr 29, 2024 at 9:20 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
[...]
> >> +static void notif_irq_handler(int irq, void *data)
> >> +{
> >> + const struct arm_smccc_1_2_regs arg = {
> >> + .a0 = FFA_NOTIFICATION_INFO_GET_64,
> >> + };
> >> + struct arm_smccc_1_2_regs resp;
> >> + unsigned int id_pos;
> >> + unsigned int list_count;
> >> + uint64_t ids_count;
> >> + unsigned int n;
> >> + int32_t res;
> >> +
> >> + do {
> >> + arm_smccc_1_2_smc(&arg, &resp);
> >> + res = ffa_get_ret_code(&resp);
> >> + if ( res )
> >> + {
> >> + if ( res != FFA_RET_NO_DATA )
> >> + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> >> + res);
> >> + return;
> >> + }
> >> +
> >> + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> >> + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> >> + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> >> +
> >> + id_pos = 0;
> >> + for ( n = 0; n < list_count; n++ )
> >> + {
> >> + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> >> + struct domain *d;
> >> +
> >> + d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> >
> > Thinking a bit more about the question from Bertrand about get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok here.
> >
> > If I am not mistaken, d->arch.tee will be freed as part of the domain teardown process. This means you can have the following scenario:
> >
> > CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
> >
> > CPU1: call domain_kill()
> > CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
> >
> > d->arch.tee is now a dangling pointer
> >
> > CPU0: access d->arch.tee
> >
> > This implies you may need to gain a global lock (I don't have a better idea so far) to protect the IRQ handler against domains teardown.
> >
> > Did I miss anything?
>
> I think you are right which is why I was thinking to use rcu_lock_live_remote_domain_by_id to only get a reference
> to the domain if it is not dying.
>
> From the comment in sched.h:
> /*
> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
> * This is the preferred function if the returned domain reference
> * is short lived, but it cannot be used if the domain reference needs
> * to be kept beyond the current scope (e.g., across a softirq).
> * The returned domain reference must be discarded using rcu_unlock_domain().
> */
>
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".
>
> It would be a good idea to start a mailing list thread discussion with other
> maintainers on the subject to confirm.
> @Jens: as i will be off for some time, would you mind doing it ?
Sure, first I'll send out a new patch set with the current comments
addressed. I'll update to use rcu_lock_live_remote_domain_by_id() both
because it makes more sense than the current code, and also to have a
good base for the discussion.
Thanks,
Jens
next prev parent reply other threads:[~2024-04-29 9:50 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-26 9:20 ` Bertrand Marquis
2024-04-26 12:11 ` Jens Wiklander
2024-04-26 12:19 ` Bertrand Marquis
2024-04-26 12:32 ` Jens Wiklander
2024-04-26 12:41 ` Bertrand Marquis
2024-04-26 13:02 ` Jens Wiklander
2024-04-26 15:12 ` Bertrand Marquis
2024-04-26 18:31 ` Julien Grall
2024-04-26 17:58 ` Julien Grall
2024-04-29 9:55 ` Jens Wiklander
2024-04-26 19:07 ` Julien Grall
2024-04-29 7:20 ` Bertrand Marquis
2024-04-29 9:49 ` Jens Wiklander [this message]
2024-04-29 20:55 ` Julien Grall
2024-04-29 8:43 ` Jens Wiklander
2024-04-26 9:23 ` [XEN PATCH v3 0/5] FF-A notifications Bertrand Marquis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHUa44HdmxV4oBeYu4rX89sFm7_NQcwza3Eotme9Z5g7EDRQew@mail.gmail.com \
--to=jens.wiklander@linaro.org \
--cc=Bertrand.Marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=julien@xen.org \
--cc=michal.orzel@amd.com \
--cc=patches@linaro.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.