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

[Julien Grall <julien.grall@arm.com>]

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@arm.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel