[PATCH v1 2/4] KVM: arm64: Document KVM_ARM_GET_REG_WRITABLE_MASKS

[Eric Auger]

Hi Marc,

On 2/13/24 15:53, Marc Zyngier wrote:
> Hey Eric,
> 
> On Tue, 13 Feb 2024 13:59:31 +0000,
> Eric Auger <eauger at redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/19/23 19:50, Jing Zhang wrote:
>>> Add some basic documentation on how to get feature ID register writable
>>> masks from userspace.
>>>
>>> Signed-off-by: Jing Zhang <jingzhangos at google.com>
>>> ---
>>>  Documentation/virt/kvm/api.rst | 42 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>>> index 21a7578142a1..2defb5e198ce 100644
>>> --- a/Documentation/virt/kvm/api.rst
>>> +++ b/Documentation/virt/kvm/api.rst
>>> @@ -6070,6 +6070,48 @@ writes to the CNTVCT_EL0 and CNTPCT_EL0 registers using the SET_ONE_REG
>>>  interface. No error will be returned, but the resulting offset will not be
>>>  applied.
>>>  
>>> +4.139 KVM_ARM_GET_REG_WRITABLE_MASKS
>>> +-------------------------------------------
>>> +
>>> +:Capability: KVM_CAP_ARM_SUPPORTED_FEATURE_ID_RANGES
>>> +:Architectures: arm64
>>> +:Type: vm ioctl
>>> +:Parameters: struct reg_mask_range (in/out)
>>> +:Returns: 0 on success, < 0 on error
>>> +
>>> +
>>> +::
>>> +
>>> +        #define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
>>> +        #define ARM64_FEATURE_ID_RANGE_IDREGS	BIT(0)
>>> +
>>> +        struct reg_mask_range {
>>> +                __u64 addr;             /* Pointer to mask array */
>>> +                __u32 range;            /* Requested range */
>>> +                __u32 reserved[13];
>>> +        };
>>> +
>>> +This ioctl copies the writable masks for Feature ID registers to userspace.
>>> +The Feature ID space is defined as the AArch64 System register space with
>>> +op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7}, op2=={0-7}.
>> when attempting a migration between Ampere Altra and ThunderXv2 the
>> first hurdle is to handle a failure when writing ICC_CTLR_EL1
>> (3.0.12.12.4) on dest. This reg is outside of the scope of the above
>> single range (BIT(0)).
> 
> Indeed. But more importantly, this isn't really an ID register. Plenty
> of variable bits in there.
> 
>> This may be questionable if we want to migrate between those types of
>> machines but the goal is to exercise different scenarios to have a
>> gloval view of the problems.
> 
> I think this is a valuable experiment, and we should definitely
> explore this sort of things (as I cannot see the diversity of ARM
> system slowing down any time soon).
> 
>>
>> This reg exposes some RO capabilities such as ExtRange, A3V, SEIS,hyp/nvhe/hyp-main.c
>> IDBits, ...
>> So to get the migration going further I would need to tweek this on the
>> source - for instance I guess SEIS could be reset despite the host HW
>> cap - without making too much trouble.
> 
> I'm not sure SEIS is such an easy one: if you promised the guest that
> it would never get an SError doing the most stupid things (SEIS=0), it
> really shouldn't get one after migration. If you advertised it on the
> source HW (Altra), a migration to TX2 would be fine.
I see. Indeed for sure we must make sure KVM cannot inject SEIs in the
guest if SEIS is not advertised on guest side.

In this case SEIS is 0 on src Altra. On dst TX2 ich_vtr_el2 advertises
it and host seis =1 causing set_gic_ctlr to fail (vgic-sys-reg-v3.c).

> 
> The other bits are possible to change depending on the requirements of
> the VM (aff3, IDBits), and ExtRange should always be set to 0 (because
> our GIC implementation doesn't support EPPI/ESPI).
> 
> The really ugly part here is that if you want to affect these bits,
> you will need to trap and emulate the access. Not a big deal, but in
> the absence of FGT, you will need to handle the full Common trap
> group, which is going to slow things down (you will have to trap
> ICV_PMR_EL1, for example).
Would it be sensible to simplify things and only support the new range
if FGT is supported?
> 
>> What would you recommend, adding a new range? But I guess we need to
>> design ranges carefully otherwise we may be quickly limited by the
>> number of flag bits.
> 
> I can see a need to adding a range that would cover non-ID registers
> that have RO fields. But we also need to consider the case of EL2
> registers that take part in this.
Yes I need to better understand the consistency with ICH_VTR_EL2
> 
> For example, ICV_CTLR_EL1 and ICH_VTR_EL2 and deeply linked, and share
> some fields. Without NV, no need to expose HCR_VTR_EL2. With NV, this
> register actually drives ICV_CTLR_EL1.
understood.
> 
> So careful planning is required here, and the impact of this measured.

Understood. More time needed to study the code ;-)

Thanks!

Eric
> 
> Thanks,
> 
> 	M.
>