Re: [PATCH 11/16] x86: add a boot option to enable and disable the direct map

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/05/2020 14:11, Wei Liu wrote:
> On Fri, May 01, 2020 at 01:59:24PM +0100, Hongyan Xia wrote:
> > On Fri, 2020-05-01 at 12:11 +0000, Wei Liu wrote:
> > > On Thu, Apr 30, 2020 at 09:44:20PM +0100, Hongyan Xia wrote:
> > > > From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> > > >
> > > > Also add a helper function to retrieve it. Change
> > > > arch_mfn_in_direct_map
> > > > to check this option before returning.
> > > >
> > > > This is added as a boot command line option, not a Kconfig. We do
> > > > not
> > > > produce different builds for EC2 so this is not introduced as a
> > > > compile-time configuration.
> > > Having a Kconfig will probably allow the compiler to eliminate dead
> > > code.
> > >
> > > This is not asking you to do the work, someone can come along and
> > > adjust
> > > arch_has_directmap easily.
> > My original code added this as a CONFIG option, but I converted it into
> > a boot-time switch, so I can just dig out history and convert it back.
> > I wonder if we should get more opinions on this to make a decision.
> Form my perspective, you as a contributor has done the work to scratch
> your own itch, hence I said "not asking you to do the work". I don't
> want to turn every comment into a formal ask and eventually lead to
> feature creep.
>
> > I would love Xen to have static key support though so that a boot-time
> > switch costs no run-time performance.
> Yes that would be great.
From my understanding static key is very powerful as you could modify 
the value even at runtime.
On Arm, I wrote a version that I would call static key for the poor. We 
are using alternative to select between 0 and 1 as an immediate value.
#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
static inline bool check_workaround_##erratum(void)             \
{                                                               \
    if ( !IS_ENABLED(arch) )                                    \
        return false;                                           \
    else                                                        \
    {                                                           \
        register_t ret;                                         \
                                                                \
        asm volatile (ALTERNATIVE("mov %0, #0",                 \
                                  "mov %0, #1",                 \
                                  feature)                      \
                      : "=r" (ret));                            \
                                                                \
        return unlikely(ret);                                   \
    }                                                           \
}

The code generated will still be using a conditional branch, but the 
processor should be able to always infer correctly whether the condition 
is true or not.
The implementation is also very tailored to Arm as we consider 
workaround are not enabled by default. But I think this can be improved 
and made more generic.
Cheers,

--
Julien Grall