On 06/11/2018 03:38 PM, Peter Maydell wrote:
> On 6 June 2018 at 10:30, <luc.mic...@greensocs.com> wrote:
>> From: Luc MICHEL <luc.mic...@greensocs.com>
>>
>> Add the necessary parts of the virtualization extensions state to the
>> GIC state. We choose to increase the size of the CPU interfaces state to
>> add space for the vCPU interfaces (the GIC_NCPU_VCPU macro). This way,
>> we'll be able to reuse most of the CPU interface code for the vCPUs.
>>
>> The vCPUs are numbered from GIC_NCPU to (GIC_NCPU * 2) - 1. The
>> `gic_is_vcpu` function help to determine if a given CPU id correspond to
>> a physical CPU or a virtual one.
>>
>> The GIC VMState is updated to account for this modification. We add a
>> subsection for the virtual interface state. The virtual CPU interfaces
>> state however cannot be put in the subsection because of some 2D arrays
>> in the GIC state. This implies an increase in the VMState version id.
>>
>> For the in-kernel KVM VGIC, since the exposed VGIC does not implement
>> the virtualization extensions, we report an error if the corresponding
>> property is set to true.
>>
>> Signed-off-by: Luc MICHEL <luc.mic...@greensocs.com>
>> ---
>
>> +/* Note: We cannot put the vCPUs state in this subsection because of some 2D
>> + * arrays that mix CPU and vCPU states. */
>> +static const VMStateDescription vmstate_gic_virt_state = {
>> + .name = "arm_gic_virt_state",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = gic_virt_state_needed,
>> + .fields = (VMStateField[]) {
>> + /* Virtual interface */
>> + VMSTATE_UINT32_ARRAY(h_hcr, GICState, GIC_NCPU),
>> + VMSTATE_UINT32_ARRAY(h_misr, GICState, GIC_NCPU),
>> + VMSTATE_UINT32_2DARRAY(h_lr, GICState, GIC_NR_LR, GIC_NCPU),
>> + VMSTATE_UINT64_ARRAY(h_eisr, GICState, GIC_NCPU),
>> + VMSTATE_UINT64_ARRAY(h_elrsr, GICState, GIC_NCPU),
>> + VMSTATE_UINT32_ARRAY(h_apr, GICState, GIC_NCPU),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> static const VMStateDescription vmstate_gic = {
>> .name = "arm_gic",
>> - .version_id = 12,
>> - .minimum_version_id = 12,
>> + .version_id = 13,
>> + .minimum_version_id = 13,
>
> This breaks migration compatibility, which we can't do for the GIC
> (it is used by some KVM VM configs, where we strongly care about
> compatibility).
>
>> .pre_save = gic_pre_save,
>> .post_load = gic_post_load,
>> .fields = (VMStateField[]) {
>> VMSTATE_UINT32(ctlr, GICState),
>> - VMSTATE_UINT32_ARRAY(cpu_ctlr, GICState, GIC_NCPU),
>> + VMSTATE_UINT32_ARRAY(cpu_ctlr, GICState, GIC_NCPU_VCPU),
>
> If you want to put the VCPU state in the same array as the CPU state
> like this, you need to use subarrays, so here
> VMSTATE_UINT32_SUB_ARRAY(cpu_ctlr, GICState, 0, GIC_NCPU),
> and in the vmstate_gic_virt_state
> VMSTATE_UINT32_SUB_ARRAY(cpu_ctlr, GICState, GIC_NCPU, GIC_NCPU)
>
> Similarly for the other arrays. You'll need a patch adding
> VMSTATE_UINT16_SUB_ARRAY to vmstate.h:
>
> #define VMSTATE_UINT16_SUB_ARRAY(_f, _s, _start, _num) \
> VMSTATE_SUB_ARRAY(_f, _s, _start, _num, 0, vmstate_info_uint16, uint16_t)
>
>> VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>> vmstate_gic_irq_state, gic_irq_state),
>> VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
>> VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
>> VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
>> VMSTATE_UINT8_2DARRAY(sgi_pending, GICState, GIC_NR_SGIS, GIC_NCPU),
>> - VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
>> - VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
>> - VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
>> - VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
>> - VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
>> - VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU),
>> + VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU_VCPU),
>> + VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU_VCPU),
>> + VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU_VCPU),
>> + VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU_VCPU),
>> + VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU_VCPU),
>> + VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU_VCPU),
>
> The 2D array is more painful. You need to describe it as a set of
> 1D slices, something like
> VMSTATE_UINT32_SUB_ARRAY(apr, GICState, 0, GIC_NCPU),
> VMSTATE_UINT32_SUB_ARRAY(apr, GICState, GIC_NCPU_VCPU, GIC_NCPU),
> VMSTATE_UINT32_SUB_ARRAY(apr, GICState, GIC_NCPU_VCPU * 2,
> GIC_NCPU),
> [...]
> VMSTATE_UINT32_SUB_ARRAY(apr, GICState, GIC_NCPU_VCPU * n,
> GIC_NCPU),
> where n is GIC_NR_APRS - 1
>
> and then the other slices in the vmstate_gic_virt_state.
>
> But conveniently the only 2D array is for the APR registers, which
> you don't actually want to have state for for the virtualization
> extensions anyway. The only state here is in the GICH_APR, and
> (as per the recommendation in the spec in the GICV_APRn documentation)
> the GICV_APRn should just be either RAZ/WI or aliases of GICH_APR,
> with no state of their own to migrate.
>
> More generally, there's something odd going on here. The way the
> GIC virtualization extensions are defined, there are registers
> which expose the state to the guest (the GIC virtual CPU interface),
> and there are registers which expose the state to the hypervisor
> (the GIC virtual interface), but the underlying state is identical.
> In the spec all the various fields in the virtual CPU interface
> are defined as aliases to register fields in the virtual interface
> (for instance GICV_PMR is an alias of GICH_VMCR.VMPriMask).
>
> So you don't want to be migrating the data twice -- depending on
> the implementation we either hold the state in struct fields that
> look like the CPU interface, and the virtual interface register
> read and write code does the mapping to access the right parts of
> those, or we can do it the other way round and store the state in
> struct fields matching the virtual interface registers, with the
> read and write functions for the virtual CPU interface doing the
> mapping. But we don't want struct fields and migration state
> descriptions for both.
Indeed you are right, h_apr is a leftover that is not used in my
implementation. I use apr[0] of the vCPU interface to store the APR
value. If it's ok like this, I can simply remove h_apr from the struct
and from the virt VMState.
The same goes for h_eisr and h_elrsr, which are in the struct as a
cached data, but that can be recomputed from the LRs, and thus don't
need to be stored in the VMState. In my implementation they are
recomputed in the VMState post_load function.
If you agree with that, I'm going to publish a v2 with migration
compatibility and without those three registers in the VMState.
Thanks,
--
Luc
>
>> VMSTATE_UINT32_2DARRAY(nsapr, GICState, GIC_NR_APRS, GIC_NCPU),
>> VMSTATE_END_OF_LIST()
>> + },
>> + .subsections = (const VMStateDescription * []) {
>> + &vmstate_gic_virt_state,
>> + NULL
>> }
>> };
>
> thanks
> -- PMM
>
signature.asc
Description: OpenPGP digital signature
