Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers

[Gavin Shan <gshan@redhat.com>]

Hi Drew,

On 4/11/22 8:02 PM, Andrew Jones wrote:
> On Mon, Apr 11, 2022 at 10:22:59AM +0100, Peter Maydell wrote:
>> On Mon, 11 Apr 2022 at 07:59, Gavin Shan <gshan@redhat.com> wrote:
>>>
>>> There are two arrays for each CPU, to store the indexes and values of the
>>> coprocessor registers. Currently, 8 bytes fixed storage space is reserved
>>> for each coprocessor register. However, larger coprocessor registers have
>>> been defined and exposed by KVM. Except SVE registers, no coprocessor
>>> register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
>>> won't be exploited in future. For example, I'm looking into SDEI virtualization
>>> support, which isn't merged into Linux upstream yet. I have plan to add
>>> several coprocessor ("firmware pseudo") registers to assist the migration.
>>
>> So, can you give an example of coprocessor registers which are
>> not 8 bytes in size? How are they accessed by the guest?
>> If we need to support them then we need to support them, but this
>> cover letter/series doesn't seem to me to provide enough detail
>> to make the case that they really are necessary.
>>
>> Also, we support SVE today, and we don't have variable size
>> coprocessor registers. Is there a bug here that we would be
>> fixing ?
> 
> SVE registers are KVM_REG_SIZE_U2048 and KVM_REG_SIZE_U256 sized
> registers. They work fine (just like the VFP registers which are
> KVM_REG_SIZE_U128 sized). They work because they don't get stored in the
> cpreg list. SVE and CORE (which includes VFP) registers are filtered
> out by kvm_arm_reg_syncs_via_cpreg_list(). Since they're filtered
> out they need to be handled specifically by kvm_arch_get/put_registers()
> 
> I asked Gavin to check if following the SVE pattern made sense for his
> use case prior to sending this series, but I don't see the rationale for
> not following the SVE pattern in this cover letter. Gavin, can you please
> explain why following the SVE pattern doesn't work? Or, are you trying to
> avoid adding an additional special case?
> 

Yes, SVE registers are special case. They're not synchronized through
coprocessor register list as you mentioned. For SDEI, we mimic PSCI
because both of them are firmware interfaces.

PSCI's pseudo-registers are synchronized and migrated through the
coprocessor register list. Besides, treating SDEI as additional
special case should work, but more maintaining load will be introduced.
We need separate functions to get/set SDEI's pseudo registers, like
what we did for SVE.

Thanks,
Gavin