From: Luca Fancellu <luca.fancellu@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Bertrand Marquis <bertrand.marquis@arm.com>,
wei.chen@arm.com, Stefano Stabellini <sstabellini@kernel.org>,
Julien Grall <julien@xen.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
Rahul Singh <rahul.singh@arm.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL
Date: Thu, 8 Apr 2021 14:11:39 +0100 [thread overview]
Message-ID: <4F221E20-0ABA-499D-A810-4A2A4F20F17D@arm.com> (raw)
In-Reply-To: <b79f363c-2220-f187-6840-23205ef37e1e@suse.com>
> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.04.2021 11:48, Luca Fancellu wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>> struct domain *dom0;
>> int rv;
>>
>> - if ( d != hardware_domain || d->domain_id == 0 )
>> + if ( !is_hardware_domain(d) || d->domain_id == 0 )
>> return 0;
>>
>> rv = xsm_init_hardware_domain(XSM_HOOK, d);
>> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>> err = err ?: -EILSEQ; /* Release build safety. */
>>
>> d->is_dying = DOMDYING_dead;
>> - if ( hardware_domain == d )
>> + if ( is_hardware_domain(d) )
>> hardware_domain = old_hwdom;
>> atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>
> While these may seem like open-coding of is_hardware_domain(), I
> think it would be better to leave them alone. In neither of the two
> cases is it possible for d to be NULL afaics, and hence your
> addition to is_hardware_domain() doesn't matter here.
Yes that is right, the only thing is that we have a nice function
“Is_hardware_domain” and we and up comparing “manually”.
It looks weird to me, but I can change it back if you don’t agree.
>
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -30,7 +30,7 @@ enum domain_type {
>> #endif
>>
>> /* The hardware domain has always its memory direct mapped. */
>> -#define is_domain_direct_mapped(d) ((d) == hardware_domain)
>> +#define is_domain_direct_mapped(d) (is_hardware_domain(d))
>
> Nit: If this was code I'm a maintainer of, I'd ask for the unneeded
> parentheses to be dropped.
Sure I can do that on the next version of the patch
>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1022,7 +1022,7 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>> if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>> return false;
>>
>> - return evaluate_nospec(d == hardware_domain);
>> + return evaluate_nospec((hardware_domain != NULL) && (d == hardware_domain));
>> }
>
> This would be the first instance in the tree of an && expression
> inside evaluate_nospec(). I think the generated code will still be
> okay, but I wonder whether this is really needed. Can you point
> out code paths where d may actually be NULL, and where
>
> static always_inline bool is_hardware_domain(const struct domain *d)
> {
> if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
> return false;
>
> if ( !d )
> return false;
>
> return evaluate_nospec(d == hardware_domain);
> }
>
> would not behave as intended (i.e. where bad speculation would
> result)? (In any event I think checking d against NULL is preferable
> over checking hardware_domain.)
I agree with you, I will change the code checking if d is NULL the
way it’s written above
Cheers,
Luca
>
> Jan
next prev parent reply other threads:[~2021-04-08 13:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-08 9:48 [PATCH v2 0/4] xen/arm: Prevent Dom0 to be loaded when using dom0less Luca Fancellu
2021-04-08 9:48 ` [PATCH v2 1/4] xen/arm: Move dom0 creation in domain_build.c Luca Fancellu
2021-04-09 8:30 ` Julien Grall
2021-04-09 9:51 ` Luca Fancellu
2021-04-08 9:48 ` [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL Luca Fancellu
2021-04-08 10:17 ` Jan Beulich
2021-04-08 13:11 ` Luca Fancellu [this message]
2021-04-08 14:36 ` Jan Beulich
2021-04-08 14:58 ` Luca Fancellu
2021-04-08 9:48 ` [PATCH v2 3/4] xen/arm: Reserve domid 0 for Dom0 Luca Fancellu
2021-04-08 10:46 ` Jan Beulich
2021-04-08 13:12 ` Luca Fancellu
2021-04-08 9:48 ` [PATCH v2 4/4] xen/arm: Prevent Dom0 to be loaded when using dom0less Luca Fancellu
2021-04-09 9:12 ` Julien Grall
2021-04-09 9:56 ` Luca Fancellu
2021-04-09 10:04 ` Julien Grall
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=4F221E20-0ABA-499D-A810-4A2A4F20F17D@arm.com \
--to=luca.fancellu@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=rahul.singh@arm.com \
--cc=sstabellini@kernel.org \
--cc=wei.chen@arm.com \
--cc=wl@xen.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).