Re: [PATCH v7 11/12] xen/arm: add sve property for dom0less domUs

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 25 May 2023, at 09:52, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> Sorry for jumping into this but I just wanted to read the dt binding doc and 
> spotted one thing by accident.
> 
> On 24/05/2023 17:20, Bertrand Marquis wrote:
>> 
>> 
>> Hi Luca,
>> 
>>> On 23 May 2023, at 09:43, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>>> 
>>> Add a device tree property in the dom0less domU configuration
>>> to enable the guest to use SVE.
>>> 
>>> Update documentation.
>>> 
>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> 
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> (...)
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 9202a96d9c28..ba4fe9e165ee 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -4008,6 +4008,34 @@ void __init create_domUs(void)
>>>            d_cfg.max_maptrack_frames = val;
>>>        }
>>> 
>>> +        if ( dt_get_property(node, "sve", &val) )
>>> +        {
>>> +#ifdef CONFIG_ARM64_SVE
>>> +            unsigned int sve_vl_bits;
>>> +            bool ret = false;
>>> +
>>> +            if ( !val )
>>> +            {
>>> +                /* Property found with no value, means max HW VL supported 
>>> */
>>> +                ret = sve_domctl_vl_param(-1, &sve_vl_bits);
>>> +            }
>>> +            else
>>> +            {
>>> +                if ( dt_property_read_u32(node, "sve", &val) )
>>> +                    ret = sve_domctl_vl_param(val, &sve_vl_bits);
>>> +                else
>>> +                    panic("Error reading 'sve' property");
> Both here and ...
> 
>>> +            }
>>> +
>>> +            if ( ret )
>>> +                d_cfg.arch.sve_vl = sve_encode_vl(sve_vl_bits);
>>> +            else
>>> +                panic("SVE vector length error\n");
>>> +#else
>>> +            panic("'sve' property found, but CONFIG_ARM64_SVE not 
>>> selected");
> here you are missing \n at the end of string. If you take a look at panic() 
> implementation,
> new line char is not added so in your case it would result in an ugly 
> formatted panic message.

Hi Michal,

Thank you for pointing that out! Indeed there might be some issues, I will fix 
in the next push.

@Bertrand, can I retain your R-by with this fix?

> 
> ~Michal