Re: [Xen-devel] [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume

[Julien Grall <julien.grall@xxxxxxx>]

On 04/24/2018 03:50 PM, Mirela Simonovic wrote:
> Hi Julien,
Hi,

Sorry I forgot to answer to some part of the e-mail.

> On Mon, Apr 23, 2018 at 1:28 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Mirela,
> > On 20/04/18 13:25, Mirela Simonovic wrote:
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index d43c3aa896..9bb62c13cd 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1451,13 +1451,21 @@ err:
> > >        return page;
> > >    }
> > >    -static void __init setup_virt_paging_one(void *data)
> > > +static void setup_virt_paging_one(void *data)
> > >    {
> > >        unsigned long val = (unsigned long)data;
> > >        WRITE_SYSREG32(val, VTCR_EL2);
> > >        isb();
> > >    }
> > >    +/* VTCR value to be configured by all CPUs. Set only once by the boot
> > > CPU */
> > > +static unsigned long __read_mostly vtcr_value;
> >
> > VTCR is a register, so the type should be represented in term of bits (i.e
> > uint*_t).
> I followed the type used in setup_virt_paging() and it's unsigned
> long. However, the spec says the VTCR_EL2 is 32-bit register, meaning
> that in the current implementation the type is not correct.
> If I want the type to be correct in this patch Xen will not compile.
> Are you suggesting to fix the type in existing implementation?
The unsigned long is just a workaround to avoid an extra variable for 
the cast. As you introduce a static variable, then the cast become 
unnecessary.
> > > +
> > > +void setup_virt_paging_secondary(void)
> > > +{
> > > +    setup_virt_paging_one((void *)vtcr_value);
> >
> > That's fairly ugly. Is there any way to rework the interface? For instance,
> > because you have a static variable which contain the VTCR, you could just
> > use the variable in setup_virt_paging one.
>  I 
> If the argument provided to setup_virt_paging_one() is NULL within the
> setup_virt_paging_one() I configure saved static vtcr_value? If that
> is what you meant it was submitted in previous version of this patch
> :)
> Are you suggesting to revert the change to v1?
I would suggest a mix between v1 and v2. Something like:

static uint64_t vtcr;

static setup_init_paging_one(void *data)
{
   WRITE_SYSREG(vtcr, VTRC_EL2);
   [...]
}
r

void __init setup_virt_paging(...)
{
    vtcr = val;
}

Potentially, you could drop val and use vtcr everywhere in 
setup_virt_paging().
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel