On Thu, May 12, 2016 at 08:10:21PM +0100, Andre Przywara wrote:

Hi,

On 12/05/16 19:41, Christoffer Dall wrote:

quoted

On Fri, May 06, 2016 at 11:45:58AM +0100, Andre Przywara wrote:

quoted

Userland may want to save and restore the state of the in-kernel VGIC,
so we provide the code which takes a userland request and translate
that into calls to our MMIO framework.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 50 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index c952f6f..bb33af8 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c

@@ -264,7 +264,55 @@ static int vgic_attr_regs_access(struct kvm_device *dev,

come to think of it, this is GICv2 specific right?

why don't we call it vgic_v2_attr_regs_access then?

and should it really live in this file?

Mmmh, at the moment every userland access is v2 specific, but
potentially this function should cover GICv3 as well, I think. We might
need some adjustments once this will be implemented, but in general I
don't see anything too v2 specific in here?

the defines being used below are v2 specific and it's going to stay that
way.  But ok, if you think it will be reworked to cater for both v2 and
v3, fine.

quoted

quoted

 				 struct kvm_device_attr *attr,
 				 u32 *reg, bool is_write)
 {
-	return -ENXIO;
+	gpa_t addr;
+	int cpuid, ret, c;
+	struct kvm_vcpu *vcpu, *tmp_vcpu;
+
+	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
+		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+
+	mutex_lock(&dev->kvm->lock);
+
+	ret = vgic_init(dev->kvm);
+	if (ret)
+		goto out;
+
+	if (cpuid >= atomic_read(&dev->kvm->online_vcpus)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Ensure that no other VCPU is running by checking the vcpu->cpu
+	 * field.  If no other VPCUs are running we can safely access the VGIC
+	 * state, because even if another VPU is run after this point, that
+	 * VCPU will not touch the vgic state, because it will block on
+	 * getting the vgic->lock in kvm_vgic_sync_hwstate().
+	 */

eh, I don't see us grabbing any vgic->lock (does that still exist?)
here?

Yeah, that scared me the other day too.
In fact I think we cannot guarantee this requirement anymore with our
existing locks - but maybe we can pause the guest explicitly as we do
now for the clear-active handler?

I think that should work, yes.

Even if you're doing a restore of the state before actually running your
VCPUs, the make all requests wouldn't do anything and your VCPUs would
block in the early part of the run loop.  They will call
first_run_init() though, but I can't easily make that out to be a
problem.

An alternative is to do something similar to kvm_vgic_create(), perhaps
factor out that logic, which grabs the mutexes of all vcpus.

-Christoffer