On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell at linaro.org> wrote:
> On 9 August 2016 at 11:58, <vijay.kilari at gmail.com> wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar at cavium.com>
>>
>> This patchset adds API for saving and restoring
>> of VGICv3 registers to support live migration with new vgic feature.
>> This API definition is as per version of VGICv3 specification
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>
>> To test live migration with QEMU, use below patch series
>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>
>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>> http://www.spinics.net/lists/kvm/msg122040.html
>>
>> v1 => v2:
>> - The init sequence change patch is no more required.
>> Fixed in patch 2 by using static vgic_io_dev regions structure instead
>> of using dynamic allocation pointer.
>> - Updated commit message of patch 4.
>> - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>> Used local variable for 32-bit access.
>> - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>> arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>
> I only scanned briefly through this patchset, but I didn't
> see any code implementing:
> * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
If irq->pending is updated by kernel based on irq->line_level when interrupt
is asserted by device or guest. Do we still need to extract
irq->line_level using
this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
+(OR) GIC{D|R}_ISPENDR?
> * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
QEMU is saving and restoring this register, but kernel implementation
is missing. Kernel is silently returning zero. So could not catch. I
will fix it.
However, Specification says as below for STATUSR.
" The GICD_STATUSR and GICR_STATUSR registers are architecturally
defined such
that a write of a clear bit has no effect, whereas a write with a set bit
clears that value. To allow userspace to freely set the values
of these two
registers, setting the attributes with the register offsets for these two
registers simply sets the non-reserved bits to the value written."
Question is during restore, the set bit will clear the value STATUSR.
So it will reset the STATUSR after migrating the VM.
> GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
> don't act the same via this API as for a guest access to the register
>
> Did I miss something?
In kernel (as shown in below code snippet),
GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
is accessing irq->pending state.
unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
u32 value = 0;
int i;
/* Loop over all IRQs affected by this read */
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
if (irq->pending)
value |= (1U << i);
}
...
}
>
> thanks
> -- PMM