Re: [PATCH v3 12/20] target/arm: generate xml description of our SVE registers

From: Alan Hayward <Alan.Hayward@arm.com>
To: "Luis Machado" <luis.machado@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>
Cc: "damien.hedde@greensocs.com" <damien.hedde@greensocs.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"open list:ARM TCG CPUs" <qemu-arm@nongnu.org>, nd <nd@arm.com>
Subject: Re: [PATCH v3 12/20] target/arm: generate xml description of our SVE registers
Date: Wed, 8 Jan 2020 15:57:45 +0000	[thread overview]
Message-ID: <889CE29C-C705-4EB4-B52A-62F88A6121A2@arm.com> (raw)
In-Reply-To: <81e43d43-29f8-ec56-5270-bb90dd1bc077@linaro.org>

> On 20 Dec 2019, at 13:18, Luis Machado <luis.machado@linaro.org> wrote:
> 
> On 12/20/19 10:14 AM, Alex Bennée wrote:
>> Luis Machado <luis.machado@linaro.org> writes:
>>> On 12/19/19 4:15 PM, Alex Bennée wrote:
>>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>> 
>>>>> On 12/11/19 9:05 AM, Alex Bennée wrote:
>>>>>> +static struct TypeSize vec_lanes[] = {
>>>>> 
>>>>> const.
>>>>> 
>>>>>> +    case 51:
>>>>>> +        return gdb_get_reg64(buf, (cpu->env.vfp.zcr_el[1] & 0xf) + 1);
>>>>> 
>>>>> You need to use sve_zcr_len_for_el to get the effective vq.
>>>>> Also, I thought vg == 2 * vq.
>>>>>   > +    case 51:
>>>>>> +    {
>>>>>> +        uint64_t val = *(uint64_t *) buf;
>>>>>> +        cpu->env.vfp.zcr_el[1] = (val - 1) & 0xf;
>>>>> 
>>>>> You cannot hard-code EL1 without ifdef CONFIG_USER_ONLY.  If the effective vq
>>>>> decreases, you must call aarch64_sve_narrow_vq.  You must call arm_rebuild_hflags.
>>>> I'm just going to drop vg (and therefor the ability to set it) from
>>>> the
>>>> regset. It was only meant to be an indicator and gdb doesn't actually
>>>> look to it to size it's output. The likely dynamic extension will just
>>>> re-transmit the whole XML when a change occurs.
>>>> 

[...rebooting self after festive break]

Yes, when using a gdb stub, changes to the VG value should be silently ignored by the
stub.
In addition, if the vector length on the system does magically change whilst the program
is running, then the stub should continue to use the register sizes for the originally
transmitted XML, truncating/extending the register values as required.

Why? Because once the stub has sent the XML description to GDB on program start, then GDB
assumes the XML description will never change. GDB will error if sent packets with different
register lengths.

>>> 
>>> I'd verify with GDB first if vg isn't actually required.
>> It works with my tests but perhaps we use our own namespaced XML rather
>> than the gdbstub XML.

Yeah, if your gdb stub is providing an XML description, then essentially it can
provide whatever registers it wants to in the XML, and most stuff will just work.
When using a gdb stub, nothing (as far as I can recall) in GDB is relying on VG existing
or using its value.

>>> From looking at GDB's code, it does set vg as one of the register
>>> names, and this is regardless of any XML input. It does reference VG
>>> here and there in the code, even though it may not use it to size its
>>> output.

When using GDB a real SVE Linux box without a stub/gdbserver, then:

*Before doing anything with the target description (for example after every breakpoint),
GDB always asks the target (ie the GDB aarch64 port) for the XML description again. This
function re-reads the vector length; if the length has changed then it constructs a
new XML description and forces the registers to be re-read.

*When writing the registers back, the aarch64 target checks the VG value and if changed
writes that first. This can fail (with an error printed to the output). The rest of the
registers are then scaled and written according to the new size. There is no need to
change the xml at this stage as it’ll be done the next time GDB uses the description.

Blindly enabling the above when using a stub results in in GDB *constantly* asking the
stub for a new XML description, spamming the pipe, so this needs something more nuanced. 

I plan on sending Luis my ideas I had for VG changing when using a stub.

>> But this is all special casing for feature
>> name="org.gnu.gdb.aarch64.sve" right?
> 
> Yes, vg is only available if feature org.gnu.gdb.aarch64.sve is available.

Nod.

Alan.