KVM/ARM vGIC support

On ARM (and possibly other architectures) some bits are specific to the
model being emulated for the guest and user space needs a way to tell the kernel about those bits. An example is mmio device base addresses,
where KVM must know the base address for a given device to properly emulate mmio accesses within a certain address range or directly map a device with virtualiation extensions into the guest address space.

We try to make this API slightly more generic than for our specific use,
but so far only the VGIC uses this feature.

Signed-off-by Christoffer Dall <[email protected]>

---
 Documentation/virtual/kvm/api.txt |   37  
+++++++++++++++++++++++++++++++++++++
 arch/arm/include/uapi/asm/kvm.h   |   13 +++++++++++++
 arch/arm/kvm/arm.c                |   23 ++++++++++++++++++++++-
 include/uapi/linux/kvm.h          |    8 ++++++++
 4 files changed, 80 insertions(+), 1 deletion(-)

Documentation/virtual/kvm/api.txt +0 -0

index 38066a7a..668956f 100644
@@ -2206,6 +2206,43 @@ This ioctl returns the guest registers that  

are supported for the
KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.

+4.80 KVM_SET_DEVICE_ADDRESS

Documentation/virtual/kvm/api.txt +6 -0

+
+Capability: KVM_CAP_SET_DEVICE_ADDRESS
+Architectures: arm
+Type: vm ioctl
+Parameters: struct kvm_device_address (in)
+Returns: 0 on success, -1 on error

Quoted 37 lines · Christoffer Dall

On ARM (and possibly other architectures) some bits are specific to the model being emulated for the guest and user space needs a way to tell the kernel about those bits. An example is mmio device base addresses, where KVM must know the base address for a given device to properly emulate mmio accesses within a certain address range or directly map a device with virtualiation extensions into the guest address space.

We try to make this API slightly more generic than for our specific use, but so far only the VGIC uses this feature.

Signed-off-by Christoffer Dall <[email protected]>

---
 Documentation/virtual/kvm/api.txt |   37
+++++++++++++++++++++++++++++++++++++
 arch/arm/include/uapi/asm/kvm.h   |   13 +++++++++++++
 arch/arm/kvm/arm.c                |   23 ++++++++++++++++++++++-
 include/uapi/linux/kvm.h          |    8 ++++++++
 4 files changed, 80 insertions(+), 1 deletion(-)

Documentation/virtual/kvm/api.txt +0 -0

index 38066a7a..668956f 100644
@@ -2206,6 +2206,43 @@ This ioctl returns the guest registers that are

supported for the
KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.

+4.80 KVM_SET_DEVICE_ADDRESS

Documentation/virtual/kvm/api.txt +6 -0

+
+Capability: KVM_CAP_SET_DEVICE_ADDRESS
+Architectures: arm
+Type: vm ioctl
+Parameters: struct kvm_device_address (in)
+Returns: 0 on success, -1 on error

Documentation/virtual/kvm/api.txt This ioctl returns the guest registers that are supported for the · line 2225

Quoted 10 lines

Documentation/virtual/kvm/api.txt +10 -0

+Errors:
+  ENODEV: The device id is unknown
+  ENXIO:  Device not supported on current system
+  EEXIST: Address already set
+  E2BIG:  Address outside guest physical address space
+
+struct kvm_device_address {
+       __u64 id;
+       __u64 addr;
+};

What about this is really specific to addresses? Can't we set other device parameters this way?

Sort of like a device equivalent of PPC's one-reg interface.

Quoted 8 lines · Scott Wood

Documentation/virtual/kvm/api.txt This ioctl returns the guest registers that are supported for the · line 2225

Quoted 4 lines · Christoffer Dall

Documentation/virtual/kvm/api.txt +4 -0

+struct kvm_device_address {
+       __u64 id;
+       __u64 addr;
+};

What about this is really specific to addresses? Can't we set

other device

Quoted 4 lines

parameters this way?

Sort of like a device equivalent of PPC's one-reg interface.

This has been discussed a number of times,

and one or the other there
is a need for userspace to tell KVM to present memory-mapped devices at a given address. It was also considered to make this specific to irqchip initialization, but irqchips are different and a lot of that code is x86-specific, so that approach was discarded.

This *could* look something like this:

struct kvm_device_param {
    u64 dev_id;
    u64 param_id;
    u64 value;
};

but that has less clear, or at least less specific, semantics.

Quoted 14 lines · Christoffer Dall

Quoted 11 lines · Scott Wood

Documentation/virtual/kvm/api.txt This ioctl returns the guest registers that are supported for the · line 2225

Quoted 4 lines · Christoffer Dall

Documentation/virtual/kvm/api.txt +4 -0

+struct kvm_device_address {
+       __u64 id;
+       __u64 addr;
+};

What about this is really specific to addresses? Can't we set other device
parameters this way?

Quoted 3 lines

Quoted 2 lines

Sort of like a device equivalent of PPC's one-reg interface.

This has been discussed a number of times,

Sorry, this patch was just pointed out to me today. I googled the patch title but couldn't find this discussion.

Quoted 14 lines

and one or the other there
is a need for userspace to tell KVM to present memory-mapped devices at a given address. It was also considered to make this specific to irqchip initialization, but irqchips are different and a lot of that code is x86-specific, so that approach was discarded.

This *could* look something like this:

struct kvm_device_param {
    u64 dev_id;
    u64 param_id;
    u64 value;
};

Quoted 1 line

but that has less clear, or at least less specific, semantics.

Why is it less clear? You need to have device-specific documentation for what "id" means, so why not also an enumeration of "param"s? Or just keep it as is, and rename "address" to "value". Whether "dev" and "param" are combined is orthogonal from whether it's used for non-address things.

If you leave it as "address", either we'll have it being used for non-address things regardless of the name ("Not a typewriter!"), or there'll end up being yet more unnecessary ioctls, or device-specific things will end up getting shoved into CPU interfaces such as one-reg. For example, on MPIC we need to be able to specify the version of the chip to emulate in addition to the address at which it lives.

Also, why is it documented as an "arm" interface? Shouldn't it be a generic interface, with other architectures currently not implementing any IDs? What in the kvm_arch_vm_ioctl() wrapper is arm-specific?

Quoted 9 lines · Scott Wood

Quoted 8 lines · Christoffer Dall

This *could* look something like this:

struct kvm_device_param {
    u64 dev_id;
    u64 param_id;
    u64 value;
};

Quoted 4 lines

Quoted 1 line

but that has less clear, or at least less specific, semantics.

Why is it less clear? You need to have device-specific

documentation for

Quoted 1 line

what "id" means, so why not also an enumeration of "param"s? Or

just keep

Quoted 1 line

it as is, and rename "address" to "value". Whether "dev" and

"param" are

Quoted 1 line

combined is orthogonal from whether it's used for non-address

things.

less clear in the sense that you have to look at more code to see what it does. I'm not saying that it's too unclear, at all, I'm actually fine with it, but to make my point, we can make an ioctl that's called do_something() that takes a struct with val0, val1, val2, val3, ...

Quoted 2 lines

If you leave it as "address", either we'll have it being used for non-address things regardless of the name ("Not a typewriter!"), or

there'll

Quoted 1 line

end up being yet more unnecessary ioctls, or device-specific things

will end

Quoted 1 line

up getting shoved into CPU interfaces such as one-reg. For

example, on MPIC

Quoted 1 line

we need to be able to specify the version of the chip to emulate in

addition

Quoted 3 lines

to the address at which it lives.

Also, why is it documented as an "arm" interface? Shouldn't it be

a generic

Quoted 1 line

interface, with other architectures currently not implementing any

IDs?

Quoted 2 lines

What in the kvm_arch_vm_ioctl() wrapper is arm-specific?

As I remember the argument for keeping this the point was that there were other preferred methods for other archs to do this, and that ARM was the only platform that had this explicit need, but maybe I'm making this up.

Quoted 15 lines · Christoffer Dall

Quoted 14 lines

Quoted 11 lines · Scott Wood

Documentation/virtual/kvm/api.txt This ioctl returns the guest registers that are supported for the · line 2225

Quoted 4 lines · Christoffer Dall

Documentation/virtual/kvm/api.txt +4 -0

+struct kvm_device_address {
+       __u64 id;
+       __u64 addr;
+};

What about this is really specific to addresses? Can't we set other device
parameters this way?

Quoted 9 lines

Quoted 3 lines

Quoted 2 lines

Sort of like a device equivalent of PPC's one-reg interface.

This has been discussed a number of times,

Sorry, this patch was just pointed out to me today. I googled the patch title but couldn't find this discussion.

I believe it was mainly discussed at the KVM Forum in person.

Quoted 14 lines

Quoted 14 lines

and one or the other there
is a need for userspace to tell KVM to present memory-mapped devices at a given address. It was also considered to make this specific to irqchip initialization, but irqchips are different and a lot of that code is x86-specific, so that approach was discarded.

This *could* look something like this:

struct kvm_device_param {
   u64 dev_id;
   u64 param_id;
   u64 value;
};

Quoted 7 lines

Quoted 1 line

but that has less clear, or at least less specific, semantics.

Why is it less clear? You need to have device-specific documentation for what "id" means, so why not also an enumeration of "param"s? Or just keep it as is, and rename "address" to "value". Whether "dev" and "param" are combined is orthogonal from whether it's used for non-address things.

less clear in the sense that you have to look at more code to see what it does. I'm not saying that it's too unclear, at all, I'm actually fine with it, but to make my point, we can make an ioctl that's called do_something() that takes a struct with val0, val1, val2, val3, ...

Quoted 8 lines

If you leave it as "address", either we'll have it being used for non-address things regardless of the name ("Not a typewriter!"), or there'll end up being yet more unnecessary ioctls, or device-specific things will end up getting shoved into CPU interfaces such as one-reg. For example, on MPIC we need to be able to specify the version of the chip to emulate in addition to the address at which it lives.

Quoted 4 lines

Also, why is it documented as an "arm" interface? Shouldn't it be a generic interface, with other architectures currently not implementing any IDs? What in the kvm_arch_vm_ioctl() wrapper is arm-specific?

As I remember the argument for keeping this the point was that there were other preferred methods for other archs to do this, and that ARM was the only platform that had this explicit need, but maybe I'm making this up.

I'll let Peter and Alex respond this as well, and if they're fine with changing it to what I proposed above, then let's do that, and we can make it a non arm-specific interface.

I think we should make thus at least potentially generic. In fact, I wouldn't even mind calling it DEV_REG with the same semantics as ONE_REG, just that it also gets a unique dev id that gets created during in-kernel device creation and that it's a vm ioctl.

That way we wouldn't block our path to create two in-kernel irqchips one day.

Quoted 4 lines · Alexander Graf

I think we should make thus at least potentially generic. In fact, I wouldn't even mind calling it DEV_REG with the same semantics as ONE_REG, just that it also gets a unique dev id that gets created during in-kernel device creation and that it's a vm ioctl.

Well, we might want a DEV_REG, but you might as well just make ONE_REG OK as a VM ioctl, since there's no reason not to have not-per-cpu but not device registers. ONE_REG already supports dividing up the ID space so you can say which device or whatever is being used, because we had things like GIC registers in mind when we put that API together.

However, this shouldn't be DEV_REG, because this isn't actually setting state in the irqchip device, it's configuring the kernel's part of the system model [compare wiring up irq routing, which also has a custom ioctl rather than a generic one]. As such it definitely needs to happen only before the VM is actually started and not during VM execution, unlike a DEV_REG which would presumably be generally valid.

Quoted 2 lines

That way we wouldn't block our path to create two in-kernel irqchips one day.

Er, SET_DEVICE_ADDRESS doesn't block us doing that; that's why it has an ID parameter.

The discussion at the KVM forum, as I recall it was basically:
(a) some other ppc irqchips also want to set the base address for where their memory mapped registers live, so this isn't a totally ARM specific weirdness

(b) we didn't need to tangle up and delay the KVM ARM work with a vague and unspecified desire for general configurability

Quoted 6 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

I think we should make thus at least potentially generic. In fact, I wouldn't even mind calling it DEV_REG with the same semantics as ONE_REG, just that it also gets a unique dev id that gets created during in-kernel device creation and that it's a vm ioctl.

Quoted 5 lines

Well, we might want a DEV_REG, but you might as well just make ONE_REG OK as a VM ioctl, since there's no reason not to have not-per-cpu but not device registers. ONE_REG already supports dividing up the ID space so you can say which device or whatever is being used, because we had things like GIC registers in mind when we put that API together.

ONE_REG's address space today doesn't include a field for the target device,

because that's already predetermined by the fd you run it on. Hence my suggestion to add it as separate field, because then we keep the id space mask-free.

Quoted 6 lines

However, this shouldn't be DEV_REG, because this isn't actually setting state in the irqchip device, it's configuring the kernel's part of the system model [compare wiring up irq routing, which also has a custom ioctl rather than a generic one]. As such it definitely needs to happen only before the VM is actually started and not during VM execution, unlike a DEV_REG which would presumably be generally valid.

The same can be true for ONE_REG/SET_SREGS. On PPC we support setting the PVR (CPU type) via SREGS because ONE_REG only came later. Setting the CPU type only makes sense before you first start using a vcpu. After that point it should be forbidden.

So in a way, the irqchip memory address location is a device register, as it has all the properties that a register on that device would have. It is

* per device
* one id for one purpose
* may or may not be writable during certain times

We would also keep the gates open to get a new register id one day for a second address. Imagine an interrupt controller that splits its address mappings into "PIC base registers" and "MSI target registers", though the MSI registers logically belong to the PIC. Then the ioctl as designed here gets stuck too.

Quoted 2 lines

(b) we didn't need to tangle up and delay the KVM ARM work with a vague and unspecified desire for general configurability

Yeah, that was the basic idea. Considering that the patch set hasn't been going in for another 2 months after that discussion indicates that this isn't too much of an issue though :).

Quoted 9 lines · Alexander Graf

Quoted 6 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

I think we should make thus at least potentially generic. In fact, I wouldn't even mind calling it DEV_REG with the same semantics as ONE_REG, just that it also gets a unique dev id that gets created during in-kernel device creation and that it's a vm ioctl.

Quoted 7 lines

Quoted 5 lines

Well, we might want a DEV_REG, but you might as well just make ONE_REG OK as a VM ioctl, since there's no reason not to have not-per-cpu but not device registers. ONE_REG already supports dividing up the ID space so you can say which device or whatever is being used, because we had things like GIC registers in mind when we put that API together.

ONE_REG's address space today doesn't include a field for the target device,

There's 16 bits of "register group type".

Quoted 3 lines

because that's already predetermined by the fd you run it on. Hence my suggestion to add it as separate field, because then we keep the id space mask-free.

Er, it already has masks for the different parts of ID space. That's a feature, not a bug.

Quoted 12 lines

Quoted 6 lines

However, this shouldn't be DEV_REG, because this isn't actually setting state in the irqchip device, it's configuring the kernel's part of the system model [compare wiring up irq routing, which also has a custom ioctl rather than a generic one]. As such it definitely needs to happen only before the VM is actually started and not during VM execution, unlike a DEV_REG which would presumably be generally valid.

The same can be true for ONE_REG/SET_SREGS. On PPC we support setting the PVR (CPU type) via SREGS because ONE_REG only came later. Setting the CPU type only makes sense before you first start using a vcpu. After that point it should be forbidden.

Quoted 6 lines

So in a way, the irqchip memory address location is a device register, as it has all the properties that a register on that device would have. It is

* per device
* one id for one purpose
* may or may not be writable during certain times

There's a distinction between "things you need to do in machine setup" and "things you need to propagate for migration". It should be possible to do a migration by doing a complete copy of all ONE_REG (and DEV_REG if we have it) state from source to destination, so it needs to be always possible to write them.

Quoted 4 lines

We would also keep the gates open to get a new register id one day for a second address. Imagine an interrupt controller that splits its address mappings into "PIC base registers" and "MSI target registers", though the MSI registers logically belong to the PIC. Then the ioctl as designed here gets stuck too.

This is exactly what ARM's GIC has already. We have several memory regions (one for the distributor, one for the cpu interface), and map them with several calls to SET_DEVICE_ADDRESS.

Quoted 6 lines

Quoted 2 lines

(b) we didn't need to tangle up and delay the KVM ARM work with a vague and unspecified desire for general configurability

Yeah, that was the basic idea. Considering that the patch set hasn't been going in for another 2 months after that discussion indicates that this isn't too much of an issue though :).

Quoted 15 lines · Alexander Graf

Quoted 11 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

I think we should make thus at least potentially generic. In fact, I wouldn't even mind calling it DEV_REG with the same semantics as ONE_REG, just that it also gets a unique dev id that gets created during in-kernel device creation and that it's a vm ioctl.

Well, we might want a DEV_REG, but you might as well just make ONE_REG OK as a VM ioctl, since there's no reason not to have not-per-cpu but not device registers. ONE_REG already supports dividing up the ID space so you can say which device or whatever is being used, because we had things like GIC registers in mind when we put that API together.

Quoted 1 line

ONE_REG's address space today doesn't include a field for the target device,

There's 16 bits of "register group type".

Quoted 4 lines

because that's already predetermined by the fd you run it on. Hence my suggestion to add it as separate field, because then we keep the id space mask-free.

Er, it already has masks for the different parts of ID space. That's a feature, not a bug.

Quoted 13 lines

Quoted 7 lines

However, this shouldn't be DEV_REG, because this isn't actually setting state in the irqchip device, it's configuring the kernel's part of the system model [compare wiring up irq routing, which also has a custom ioctl rather than a generic one]. As such it definitely needs to happen only before the VM is actually started and not during VM execution, unlike a DEV_REG which would presumably be generally valid.

The same can be true for ONE_REG/SET_SREGS. On PPC we support setting the PVR (CPU type) via SREGS because ONE_REG only came later. Setting the CPU type only makes sense before you first start using a vcpu. After that point it should be forbidden.

Quoted 6 lines

So in a way, the irqchip memory address location is a device register, as it has all the properties that a register on that device would have. It is

* per device
* one id for one purpose
* may or may not be writable during certain times

There's a distinction between "things you need to do in machine setup" and "things you need to propagate for migration". It should be possible to do a migration by doing a complete copy of all ONE_REG (and DEV_REG if we have it) state from source to destination, so it needs to be always possible to write them.

Quoted 5 lines

We would also keep the gates open to get a new register id one day for a second address. Imagine an interrupt controller that splits its address mappings into "PIC base registers" and "MSI target registers", though the MSI registers logically belong to the PIC. Then the ioctl as designed here gets stuck too.

This is exactly what ARM's GIC has already. We have several memory regions (one for the distributor, one for the cpu interface), and map them with several calls to SET_DEVICE_ADDRESS.

Quoted 7 lines

Quoted 3 lines

(b) we didn't need to tangle up and delay the KVM ARM work with a vague and unspecified desire for general configurability

Yeah, that was the basic idea. Considering that the patch set hasn't been going in for another 2 months after that discussion indicates that this isn't too much of an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at the last minute :-)

Quoted 6 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this isn't too much of
an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at

last minute :-)

Quoted 8 lines · Peter Maydell

Quoted 6 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this isn't too much of
an issue though :).

Quoted 1 line

We might get there faster if people didn't keep nitpicking the APIs at

the

Quoted 1 line

last minute :-)

Exactly. We're trying hard to get the damned thing merged, and the permanent API churn quickly becomes a burden.

My understanding was that a consensus was reached 2 months ago. Has something fundamentally changed? Something that makes this API invalid? If not, can we please keep this one?

Thanks,

       M.

Signature · 1 line

Who you jivin' with that Cosmik Debris?

Quoted 8 lines · Marc Zyngier

Quoted 5 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

too much of
an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at

the

Quoted 1 line

last minute :-)

Exactly. We're trying hard to get the damned thing merged, and the permanent API churn quickly becomes a burden.

As I said earlier, we have had a lot of experience in creating really

APIs in the past.

But how about making this one specific? Call it

keep the rest as it is, resend it, and later we can come up with an actually generic interface.

Quoted 10 lines · Alexander Graf

Quoted 8 lines · Marc Zyngier

Quoted 5 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this

isn't

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

too much of
an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at

the

Quoted 1 line

last minute :-)

Exactly. We're trying hard to get the damned thing merged, and the permanent API churn quickly becomes a burden.

Quoted 1 line

As I said earlier, we have had a lot of experience in creating really

bad

Quoted 1 line

APIs in the past.

Is this one really bad? Again, what changed in the meantime that makes you think this API is wrong?

Quoted 1 line

But how about making this one specific? Call it

KVM_ARM_SET_VGIC_ADDRESS,

Quoted 2 lines

keep the rest as it is, resend it, and later we can come up with an actually generic interface.

Let's pretend you never wrote that, shall we? ;-)

Quoted 22 lines · Marc Zyngier

Quoted 10 lines · Alexander Graf

Quoted 8 lines · Marc Zyngier

Quoted 5 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this

isn't

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

too much of
an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at

the

Quoted 1 line

last minute :-)

Exactly. We're trying hard to get the damned thing merged, and the permanent API churn quickly becomes a burden.

Quoted 6 lines

Quoted 1 line

As I said earlier, we have had a lot of experience in creating really

bad

Quoted 1 line

APIs in the past.

Is this one really bad? Again, what changed in the meantime that makes you think this API is wrong?

I complained about it 2 months ago already.

Quoted 5 lines

Quoted 1 line

But how about making this one specific? Call it

KVM_ARM_SET_VGIC_ADDRESS,

Quoted 2 lines

keep the rest as it is, resend it, and later we can come up with an actually generic interface.

Quoted 1 line

Let's pretend you never wrote that, shall we? ;-)

Why? The worst thing that happened to us so far were interfaces that looked generic and extensible and turned out not to be (see SET_SREGS in the ppc code). We have 2 options to circumvent this:

1) Commonly agree on an interface that actually is extensible and use it throughout the code
2) Create a very specific interface for a single use case, keep it implemented "forever" and as soon as we need more, implement a more generic one that goes back to 1

Since the ARM patches have been out of tree for too long, I'm happy with going route 2 until we go back to square 1.

Quoted 46 lines · Alexander Graf

Quoted 22 lines · Marc Zyngier

Quoted 10 lines · Alexander Graf

Quoted 8 lines · Marc Zyngier

Quoted 5 lines · Peter Maydell

Quoted 4 lines · Alexander Graf

Yeah, that was the basic idea. Considering that the patch set hasn't been going
in for another 2 months after that discussion indicates that this

isn't

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

too much of
an issue though :).

We might get there faster if people didn't keep nitpicking the APIs at

the

Quoted 1 line

last minute :-)

Exactly. We're trying hard to get the damned thing merged, and the permanent API churn quickly becomes a burden.

Quoted 6 lines

Quoted 1 line

As I said earlier, we have had a lot of experience in creating really

bad

Quoted 1 line

APIs in the past.

Is this one really bad? Again, what changed in the meantime that makes you think this API is wrong?

I complained about it 2 months ago already.

Quoted 6 lines

Quoted 1 line

But how about making this one specific? Call it

KVM_ARM_SET_VGIC_ADDRESS,

Quoted 2 lines

keep the rest as it is, resend it, and later we can come up with an actually generic interface.

Let's pretend you never wrote that, shall we? ;-)

Why? The worst thing that happened to us so far were interfaces that looked generic and extensible and turned out not to be (see SET_SREGS in the ppc code). We have 2 options to circumvent this:

1) Commonly agree on an interface that actually is extensible and use it throughout the code
2) Create a very specific interface for a single use case, keep it implemented "forever" and as soon as we need more, implement a more generic one that goes back to 1

Quoted 1 line

Since the ARM patches have been out of tree for too long, I'm happy with going route 2 until we go back to square 1.

I really don't want to see that. Either we keep the API as it is, or we change it for something that is really better and used by other architectures. No point in turning it upside down if we're the only one doing it.

Oh, and as we're aiming for 3.9, it'd better be ready soon...

From: Marc Zyngier <[email protected]>

Wire the basic framework code for VGIC support and the initial in-kernel MMIO support code for the VGIC, used for the distributor emulation.

arch/arm/kvm/vgic.c new file +51 -0

+/**
+ * vgic_reg_access - access vgic register
+ * @mmio:   pointer to the data describing the mmio access
+ * @reg:    pointer to the virtual backing of vgic distributor data
+ * @offset: least significant 2 bits used for word offset
+ * @mode:   ACCESS_ mode (see defines above)
+ *
+ * Helper to make vgic register access easier using one of the access
+ * modes defined for vgic register access
+ * (read,raz,write-ignored,setbit,clearbit,write)
+ */
+static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
+                           phys_addr_t offset, int mode)
+{
+       int shift = (offset & 3) * 8;
+       u32 mask;
+       u32 regval;
+
+       /*
+        * Any alignment fault should have been delivered to the guest
+        * directly (ARM ARM B3.12.7 "Prioritization of aborts").
+        */
+
+       mask = (~0U) >> shift;
+       if (reg) {
+               regval = *reg;
+       } else {
+               BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
+               regval = 0;
+       }
+
+       if (mmio->is_write) {
+               u32 data = (*((u32 *)mmio->data) & mask) << shift;
+               switch (ACCESS_WRITE_MASK(mode)) {
+               case ACCESS_WRITE_IGNORED:
+                       return;
+
+               case ACCESS_WRITE_SETBIT:
+                       regval |= data;
+                       break;
+
+               case ACCESS_WRITE_CLEARBIT:
+                       regval &= ~data;
+                       break;
+
+               case ACCESS_WRITE_VALUE:
+                       regval = (regval & ~(mask << shift)) | data;
+                       break;
+               }
+               *reg = regval;
+       } else {

Quoted 4 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

Wire the basic framework code for VGIC support and the initial in-kernel MMIO support code for the VGIC, used for the distributor emulation.

[...]

Quoted 58 lines

(quoted diff) +58 -0

+/**
+ * vgic_reg_access - access vgic register
+ * @mmio:   pointer to the data describing the mmio access
+ * @reg:    pointer to the virtual backing of vgic distributor data
+ * @offset: least significant 2 bits used for word offset
+ * @mode:   ACCESS_ mode (see defines above)
+ *
+ * Helper to make vgic register access easier using one of the access
+ * modes defined for vgic register access
+ * (read,raz,write-ignored,setbit,clearbit,write)
+ */
+static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
+                           phys_addr_t offset, int mode)
+{
+       int shift = (offset & 3) * 8;
+       u32 mask;
+       u32 regval;
+
+       /*
+        * Any alignment fault should have been delivered to the guest
+        * directly (ARM ARM B3.12.7 "Prioritization of aborts").
+        */
+
+       mask = (~0U) >> shift;
+       if (reg) {
+               regval = *reg;
+       } else {
+               BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
+               regval = 0;
+       }
+
+       if (mmio->is_write) {
+               u32 data = (*((u32 *)mmio->data) & mask) << shift;
+               switch (ACCESS_WRITE_MASK(mode)) {
+               case ACCESS_WRITE_IGNORED:
+                       return;
+
+               case ACCESS_WRITE_SETBIT:
+                       regval |= data;
+                       break;
+
+               case ACCESS_WRITE_CLEARBIT:
+                       regval &= ~data;
+                       break;
+
+               case ACCESS_WRITE_VALUE:
+                       regval = (regval & ~(mask << shift)) | data;
+                       break;
+               }
+               *reg = regval;
+       } else {
+               switch (ACCESS_READ_MASK(mode)) {
+               case ACCESS_READ_RAZ:
+                       regval = 0;
+                       /* fall through */
+
+               case ACCESS_READ_VALUE:
+                       *((u32 *)mmio->data) = (regval >> shift) & mask;

Review

Quoted 3 lines

(quoted diff) +3 -0

+               }
+       }
+}

As I mentioned previously, I suspect that this doesn't work with big-endian systems. Whilst that's reasonable for the moment, a comment would be useful for the unlucky soul that decides to do that work in future (or add accessors for mmio->data as I suggested before).

Quoted 66 lines · Christoffer Dall

Quoted 4 lines

From: Marc Zyngier <[email protected]>

Wire the basic framework code for VGIC support and the initial in-kernel MMIO support code for the VGIC, used for the distributor emulation.

[...]

Quoted 58 lines

(quoted diff) +58 -0

+/**
+ * vgic_reg_access - access vgic register
+ * @mmio:   pointer to the data describing the mmio access
+ * @reg:    pointer to the virtual backing of vgic distributor data
+ * @offset: least significant 2 bits used for word offset
+ * @mode:   ACCESS_ mode (see defines above)
+ *
+ * Helper to make vgic register access easier using one of the access
+ * modes defined for vgic register access
+ * (read,raz,write-ignored,setbit,clearbit,write)
+ */
+static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
+                           phys_addr_t offset, int mode)
+{
+       int shift = (offset & 3) * 8;
+       u32 mask;
+       u32 regval;
+
+       /*
+        * Any alignment fault should have been delivered to the guest
+        * directly (ARM ARM B3.12.7 "Prioritization of aborts").
+        */
+
+       mask = (~0U) >> shift;
+       if (reg) {
+               regval = *reg;
+       } else {
+               BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
+               regval = 0;
+       }
+
+       if (mmio->is_write) {
+               u32 data = (*((u32 *)mmio->data) & mask) << shift;
+               switch (ACCESS_WRITE_MASK(mode)) {
+               case ACCESS_WRITE_IGNORED:
+                       return;
+
+               case ACCESS_WRITE_SETBIT:
+                       regval |= data;
+                       break;
+
+               case ACCESS_WRITE_CLEARBIT:
+                       regval &= ~data;
+                       break;
+
+               case ACCESS_WRITE_VALUE:
+                       regval = (regval & ~(mask << shift)) | data;
+                       break;
+               }
+               *reg = regval;
+       } else {
+               switch (ACCESS_READ_MASK(mode)) {
+               case ACCESS_READ_RAZ:
+                       regval = 0;
+                       /* fall through */
+
+               case ACCESS_READ_VALUE:
+                       *((u32 *)mmio->data) = (regval >> shift) & mask;

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+               }
+       }
+}

As I mentioned previously, I suspect that this doesn't work with big-endian systems. Whilst that's reasonable for the moment, a comment would be useful for the unlucky soul that decides to do that work in future (or add accessors for mmio->data as I suggested before).

admittedly this really hurts my brain, but I think there's actually no problem with endianness: whatever comes in mmio->data will have native endianness

and the vgic is always little-endian, so a guest would have to make sure to do its own endianness conversion before writing data, or did I get this backwards? (some nasty feeling about if the OS is compiled in another endianness than the hardware everything may break).

Anyhow, I think there's another bug in this code though. Please take a look and see if you agree:

commit 3cab2b93a6f6acd3c043e584f23b94ab8f1bbd66

Author: Christoffer Dall <[email protected]>
Date:   Mon Jan 14 15:55:18 2013 -0500

KVM: ARM: Limit vgic read/writes to load/store length

The vgic read/write operations did not consider ldrb/strb masks, and would therefore unintentionally overwrite parts of a register.

Consider for example a store of a single byte to a word-aligned address of one of the priority registers, that would cause the 3 most significant bytes to be overwritten with zeros.

Cc Marc Zyniger <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

arch/arm/kvm/vgic.c +10 -0

index 25daa07..5c1bcf5 100644
@@ -233,6 +233,16 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
 *vcpu, int irq)
             vcpu->arch.vgic_cpu.pending_shared);
 }
 
+static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
+{
+    return *((u32 *)mmio->data) & mask;
+}
+
+static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value)
+{
+    *((u32 *)mmio->data) = value & mask;
+}
+
 /**
 * vgic_reg_access - access vgic register

* @mmio: pointer to the data describing the mmio access

arch/arm/kvm/vgic.c +2 -2

@@ -247,8 +257,8 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
 *vcpu, int irq)
 static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
               phys_addr_t offset, int mode)
 {
-    int shift = (offset & 3) * 8;
-    u32 mask;
+    int word_offset = (offset & 3) * 8;
+    u32 mask = (1UL << (mmio->len * 8)) - 1;
   u32 regval;
 

   /*

arch/arm/kvm/vgic.c +0 -1

@@ -256,7 +266,6 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
    * directly (ARM ARM B3.12.7 "Prioritization of aborts").
    */
 
-    mask = (~0U) >> shift;
   if (reg) {
       regval = *reg;

   } else {

arch/arm/kvm/vgic.c +1 -1

@@ -265,7 +274,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
   }
 
   if (mmio->is_write) {
-        u32 data = (*((u32 *)mmio->data) & mask) << shift;
+        u32 data = mmio_data_read(mmio, mask) << word_offset;
       switch (ACCESS_WRITE_MASK(mode)) {
       case ACCESS_WRITE_IGNORED:

           return;

arch/arm/kvm/vgic.c +1 -1

@@ -279,7 +288,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
           break;
 
       case ACCESS_WRITE_VALUE:
-            regval = (regval & ~(mask << shift)) | data;
+            regval = (regval & ~(mask << word_offset)) | data;
           break;
       }

       *reg = regval;

arch/arm/kvm/vgic.c +1 -1

@@ -290,7 +299,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
           /* fall through */
 
       case ACCESS_READ_VALUE:
-            *((u32 *)mmio->data) = (regval >> shift) & mask;
+            mmio_data_write(mmio, mask, regval >> word_offset);
       }
   }

}

arch/arm/kvm/vgic.c +6 -0

@@ -702,6 +711,12 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu,
 struct kvm_run *run,
       (mmio->phys_addr + mmio->len) > (base + KVM_VGIC_V2_DIST_SIZE))
       return false;
 
+    /* We don't support ldrd / strd or ldm / stm to the emulated vgic */
+    if (mmio->len > 4) {
+        kvm_inject_dabt(vcpu, mmio->phys_addr);
+        return true;
+    }
+
   range = find_matching_range(vgic_ranges, mmio, base);
   if (unlikely(!range || !range->handle_mmio)) {

       pr_warn("Unhandled access %d %08llx %d\n",

--

Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm at lists.cs.columbia.edu
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Quoted 7 lines · Will Deacon

Review

Quoted 5 lines · Christoffer Dall

(quoted diff) +5 -0

+               case ACCESS_READ_VALUE:
+                       *((u32 *)mmio->data) = (regval >> shift) & mask;
+               }
+       }
+}

Quoted 5 lines

As I mentioned previously, I suspect that this doesn't work with big-endian systems. Whilst that's reasonable for the moment, a comment would be useful for the unlucky soul that decides to do that work in future (or add accessors for mmio->data as I suggested before).

admittedly this really hurts my brain, but I think there's actually no problem with endianness: whatever comes in mmio->data will have native endianness and the vgic is always little-endian, so a guest would have to make sure to do its own endianness conversion before writing data, or did I get this backwards? (some nasty feeling about if the OS is compiled in another endianness than the hardware everything may break).

Quoted 5 lines · Will Deacon

As I mentioned previously, I suspect that this doesn't work with big-endian systems. Whilst that's reasonable for the moment, a comment would be useful for the unlucky soul that decides to do that work in future (or add accessors for mmio->data as I suggested before).

admittedly this really hurts my brain, but I think there's actually no problem with endianness: whatever comes in mmio->data will have native endianness and the vgic is always little-endian, so a guest would have to make sure to do its own endianness conversion before writing data, or did I get this backwards? (some nasty feeling about if the OS is compiled in another endianness than the hardware everything may break).

Anyhow, I think there's another bug in this code though. Please take a look and see if you agree:

commit 3cab2b93a6f6acd3c043e584f23b94ab8f1bbd66

Author: Christoffer Dall <[email protected]>
Date:   Mon Jan 14 15:55:18 2013 -0500

KVM: ARM: Limit vgic read/writes to load/store length

The vgic read/write operations did not consider ldrb/strb masks, and would therefore unintentionally overwrite parts of a register.

Consider for example a store of a single byte to a word-aligned address of one of the priority registers, that would cause the 3 most significant bytes to be overwritten with zeros.

Cc Marc Zyniger <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

arch/arm/kvm/vgic.c +10 -0

index 25daa07..5c1bcf5 100644
@@ -233,6 +233,16 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
 *vcpu, int irq)
 			  vcpu->arch.vgic_cpu.pending_shared);
 }
 
+static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
+{
+	return *((u32 *)mmio->data) & mask;
+}
+
+static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value)
+{
+	*((u32 *)mmio->data) = value & mask;
+}
+
 /**
  * vgic_reg_access - access vgic register

* @mmio: pointer to the data describing the mmio access

arch/arm/kvm/vgic.c +2 -2

@@ -247,8 +257,8 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu
 *vcpu, int irq)
 static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg,
 			    phys_addr_t offset, int mode)
 {
-	int shift = (offset & 3) * 8;
-	u32 mask;
+	int word_offset = (offset & 3) * 8;
+	u32 mask = (1UL << (mmio->len * 8)) - 1;
 	u32 regval;
 

arch/arm/kvm/vgic.c +0 -1

 	/*

@@ -256,7 +266,6 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
 	 * directly (ARM ARM B3.12.7 "Prioritization of aborts").
 	 */
 
-	mask = (~0U) >> shift;
 	if (reg) {
 		regval = *reg;

arch/arm/kvm/vgic.c +1 -1

 	} else {

@@ -265,7 +274,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
 	}
 
 	if (mmio->is_write) {
-		u32 data = (*((u32 *)mmio->data) & mask) << shift;
+		u32 data = mmio_data_read(mmio, mask) << word_offset;
 		switch (ACCESS_WRITE_MASK(mode)) {
 		case ACCESS_WRITE_IGNORED:

arch/arm/kvm/vgic.c +1 -1

 			return;

@@ -279,7 +288,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
 			break;
 
 		case ACCESS_WRITE_VALUE:
-			regval = (regval & ~(mask << shift)) | data;
+			regval = (regval & ~(mask << word_offset)) | data;
 			break;
 		}

arch/arm/kvm/vgic.c +1 -1

 		*reg = regval;

@@ -290,7 +299,7 @@ static void vgic_reg_access(struct kvm_exit_mmio
 *mmio, u32 *reg,
 			/* fall through */
 
 		case ACCESS_READ_VALUE:
-			*((u32 *)mmio->data) = (regval >> shift) & mask;
+			mmio_data_write(mmio, mask, regval >> word_offset);
 		}
 	}

}

arch/arm/kvm/vgic.c +6 -0

@@ -702,6 +711,12 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu,
 struct kvm_run *run,
 	    (mmio->phys_addr + mmio->len) > (base + KVM_VGIC_V2_DIST_SIZE))
 		return false;
 
+	/* We don't support ldrd / strd or ldm / stm to the emulated vgic */
+	if (mmio->len > 4) {
+		kvm_inject_dabt(vcpu, mmio->phys_addr);
+		return true;
+	}
+
 	range = find_matching_range(vgic_ranges, mmio, base);
 	if (unlikely(!range || !range->handle_mmio)) {

pr_warn("Unhandled access %d %08llx %d\n",
--

Thanks,
-Christoffer

From: Marc Zyngier <[email protected]>

Add the GIC distributor emulation code. A number of the GIC features are simply ignored as they are not required to boot a Linux guest.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   82 +++++
 arch/arm/kvm/vgic.c             |  593 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 674 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +0 -0

index 270dcd2..9ff0d9c 100644
@@ -19,12 +19,94 @@
 #ifndef __ASM_ARM_KVM_VGIC_H
 #define __ASM_ARM_KVM_VGIC_H
 

arch/arm/include/asm/kvm_vgic.h +56 -0

+#define VGIC_NR_PPIS           16
+#define VGIC_NR_PRIVATE_IRQS   (VGIC_NR_SGIS + VGIC_NR_PPIS)
+#define VGIC_NR_SHARED_IRQS    (VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
+#define VGIC_MAX_CPUS          KVM_MAX_VCPUS
+
+/* Sanity checks... */
+#if (VGIC_MAX_CPUS > 8)
+#error Invalid number of CPU interfaces
+#endif
+
+#if (VGIC_NR_IRQS & 31)
+#error "VGIC_NR_IRQS must be a multiple of 32"
+#endif
+
+#if (VGIC_NR_IRQS > 1024)
+#error "VGIC_NR_IRQS must be <= 1024"
+#endif
+
+/*
+ * The GIC distributor registers describing interrupts have two parts:
+ * - 32 per-CPU interrupts (SGI + PPI)
+ * - a bunch of shared interrupts (SPI)
+ */
+struct vgic_bitmap {
+       union {
+               u32 reg[VGIC_NR_PRIVATE_IRQS / 32];
+               DECLARE_BITMAP(reg_ul, VGIC_NR_PRIVATE_IRQS);
+       } percpu[VGIC_MAX_CPUS];
+       union {
+               u32 reg[VGIC_NR_SHARED_IRQS / 32];
+               DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
+       } shared;
+};
+
+struct vgic_bytemap {
+       u32 percpu[VGIC_MAX_CPUS][VGIC_NR_PRIVATE_IRQS / 4];
+       u32 shared[VGIC_NR_SHARED_IRQS  / 4];
+};
+
 struct vgic_dist {
+#ifdef CONFIG_KVM_ARM_VGIC
+       spinlock_t              lock;
+
+       /* Virtual control interface mapping */
+       void __iomem            *vctrl_base;
+
        /* Distributor and vcpu interface mapping in the guest */
        phys_addr_t             vgic_dist_base;
        phys_addr_t             vgic_cpu_base;
+
+       /* Distributor enabled */
+       u32                     enabled;
+
+       /* Interrupt enabled (one bit per IRQ) */
+       struct vgic_bitmap      irq_enabled;
+
+       /* Interrupt 'pin' level */
+       struct vgic_bitmap      irq_state;
+
+       /* Level-triggered interrupt in progress */

arch/arm/include/asm/kvm_vgic.h +20 -0

+static u32 vgic_get_target_reg(struct kvm *kvm, int irq)
+{
+       struct vgic_dist *dist = &kvm->arch.vgic;
+       struct kvm_vcpu *vcpu;
+       int i, c;
+       unsigned long *bmap;
+       u32 val = 0;
+
+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       kvm_for_each_vcpu(c, vcpu, kvm) {
+               bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+               for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++)
+                       if (test_bit(irq + i, bmap))
+                               val |= 1 << (c + i * 8);
+       }
+
+       return val;
+}
+

arch/arm/include/asm/kvm_vgic.h +21 -0

+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       /*
+        * Pick the LSB in each byte. This ensures we target exactly
+        * one vcpu per IRQ. If the byte is null, assume we target
+        * CPU0.
+        */
+       for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++) {
+               int shift = i * GICD_CPUTARGETS_BITS;
+               target = ffs((val >> shift) & 0xffU);
+               target = target ? (target - 1) : 0;
+               dist->irq_spi_cpu[irq + i] = target;
+               kvm_for_each_vcpu(c, vcpu, kvm) {
+                       bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+                       if (c == target)
+                               set_bit(irq + i, bmap);
+                       else
+                               clear_bit(irq + i, bmap);
+               }
+       }
+}

static const struct mmio_range vgic_ranges[] = {

arch/arm/include/asm/kvm_vgic.h +52 -0

+       {                       /* CTRL, TYPER, IIDR */
+               .base           = 0,
+               .len            = 12,
+               .handle_mmio    = handle_mmio_misc,
+       },
+       {                       /* IGROUPRn */
+               .base           = 0x80,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ISENABLERn */
+               .base           = 0x100,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_enable_reg,
+       },
+       {                       /* ICENABLERn */
+               .base           = 0x180,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_enable_reg,
+       },
+       {                       /* ISPENDRn */
+               .base           = 0x200,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_pending_reg,
+       },
+       {                       /* ICPENDRn */
+               .base           = 0x280,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_pending_reg,
+       },
+       {                       /* ISACTIVERn */
+               .base           = 0x300,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ICACTIVERn */
+               .base           = 0x380,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* IPRIORITYRn */
+               .base           = 0x400,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_priority_reg,
+       },
+       {                       /* ITARGETSRn */
+               .base           = 0x800,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_target_reg,
+       },
+       {                       /* ICFGRn */
+               .base           = 0xC00,

Quoted 20 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

Add the GIC distributor emulation code. A number of the GIC features are simply ignored as they are not required to boot a Linux guest.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   82 +++++
 arch/arm/kvm/vgic.c             |  593 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 674 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +0 -0

index 270dcd2..9ff0d9c 100644
@@ -19,12 +19,94 @@
 #ifndef __ASM_ARM_KVM_VGIC_H
 #define __ASM_ARM_KVM_VGIC_H
 

arch/arm/include/asm/kvm_vgic.h line 31

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +8 -0

+#include <linux/kernel.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <linux/irqreturn.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
 #include <asm/hardware/gic.h>
 
+#define VGIC_NR_IRQS           128
+#define VGIC_NR_SGIS           16

Now that you have this, you can use it in a few places (see also the BUG_ONs in vgic_queue_irq).

Quoted 60 lines

arch/arm/include/asm/kvm_vgic.h +56 -0

+#define VGIC_NR_PPIS           16
+#define VGIC_NR_PRIVATE_IRQS   (VGIC_NR_SGIS + VGIC_NR_PPIS)
+#define VGIC_NR_SHARED_IRQS    (VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
+#define VGIC_MAX_CPUS          KVM_MAX_VCPUS
+
+/* Sanity checks... */
+#if (VGIC_MAX_CPUS > 8)
+#error Invalid number of CPU interfaces
+#endif
+
+#if (VGIC_NR_IRQS & 31)
+#error "VGIC_NR_IRQS must be a multiple of 32"
+#endif
+
+#if (VGIC_NR_IRQS > 1024)
+#error "VGIC_NR_IRQS must be <= 1024"
+#endif
+
+/*
+ * The GIC distributor registers describing interrupts have two parts:
+ * - 32 per-CPU interrupts (SGI + PPI)
+ * - a bunch of shared interrupts (SPI)
+ */
+struct vgic_bitmap {
+       union {
+               u32 reg[VGIC_NR_PRIVATE_IRQS / 32];
+               DECLARE_BITMAP(reg_ul, VGIC_NR_PRIVATE_IRQS);
+       } percpu[VGIC_MAX_CPUS];
+       union {
+               u32 reg[VGIC_NR_SHARED_IRQS / 32];
+               DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
+       } shared;
+};
+
+struct vgic_bytemap {
+       u32 percpu[VGIC_MAX_CPUS][VGIC_NR_PRIVATE_IRQS / 4];
+       u32 shared[VGIC_NR_SHARED_IRQS  / 4];
+};
+
 struct vgic_dist {
+#ifdef CONFIG_KVM_ARM_VGIC
+       spinlock_t              lock;
+
+       /* Virtual control interface mapping */
+       void __iomem            *vctrl_base;
+
        /* Distributor and vcpu interface mapping in the guest */
        phys_addr_t             vgic_dist_base;
        phys_addr_t             vgic_cpu_base;
+
+       /* Distributor enabled */
+       u32                     enabled;
+
+       /* Interrupt enabled (one bit per IRQ) */
+       struct vgic_bitmap      irq_enabled;
+
+       /* Interrupt 'pin' level */
+       struct vgic_bitmap      irq_state;
+
+       /* Level-triggered interrupt in progress */

arch/arm/include/asm/kvm_vgic.h line 101

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +10 -0

+       struct vgic_bitmap      irq_active;
+
+       /* Interrupt priority. Not used yet. */
+       struct vgic_bytemap     irq_priority;
+
+       /* Level/edge triggered */
+       struct vgic_bitmap      irq_cfg;
+
+       /* Source CPU per SGI and target CPU */
+       u8                      irq_sgi_sources[VGIC_MAX_CPUS][16];

VGIC_NR_SGIS

Quoted 20 lines

arch/arm/include/asm/kvm_vgic.h +20 -0

+static u32 vgic_get_target_reg(struct kvm *kvm, int irq)
+{
+       struct vgic_dist *dist = &kvm->arch.vgic;
+       struct kvm_vcpu *vcpu;
+       int i, c;
+       unsigned long *bmap;
+       u32 val = 0;
+
+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       kvm_for_each_vcpu(c, vcpu, kvm) {
+               bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+               for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++)
+                       if (test_bit(irq + i, bmap))
+                               val |= 1 << (c + i * 8);
+       }
+
+       return val;
+}
+

arch/arm/include/asm/kvm_vgic.h

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +10 -0

+static void vgic_set_target_reg(struct kvm *kvm, u32 val, int irq)
+{
+       struct vgic_dist *dist = &kvm->arch.vgic;
+       struct kvm_vcpu *vcpu;
+       int i, c;
+       unsigned long *bmap;
+       u32 target;
+
+       BUG_ON(irq & 3);
+       BUG_ON(irq < VGIC_NR_PRIVATE_IRQS);

This is now different to vgic_Get_target_reg, which doesn't have the BUG_ONs. Can we remove these ones too?

Quoted 21 lines

arch/arm/include/asm/kvm_vgic.h +21 -0

+       irq -= VGIC_NR_PRIVATE_IRQS;
+
+       /*
+        * Pick the LSB in each byte. This ensures we target exactly
+        * one vcpu per IRQ. If the byte is null, assume we target
+        * CPU0.
+        */
+       for (i = 0; i < GICD_IRQS_PER_ITARGETSR; i++) {
+               int shift = i * GICD_CPUTARGETS_BITS;
+               target = ffs((val >> shift) & 0xffU);
+               target = target ? (target - 1) : 0;
+               dist->irq_spi_cpu[irq + i] = target;
+               kvm_for_each_vcpu(c, vcpu, kvm) {
+                       bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
+                       if (c == target)
+                               set_bit(irq + i, bmap);
+                       else
+                               clear_bit(irq + i, bmap);
+               }
+       }
+}

[...]

Quoted 53 lines

static const struct mmio_range vgic_ranges[] = {

arch/arm/include/asm/kvm_vgic.h +52 -0

+       {                       /* CTRL, TYPER, IIDR */
+               .base           = 0,
+               .len            = 12,
+               .handle_mmio    = handle_mmio_misc,
+       },
+       {                       /* IGROUPRn */
+               .base           = 0x80,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ISENABLERn */
+               .base           = 0x100,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_enable_reg,
+       },
+       {                       /* ICENABLERn */
+               .base           = 0x180,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_enable_reg,
+       },
+       {                       /* ISPENDRn */
+               .base           = 0x200,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_set_pending_reg,
+       },
+       {                       /* ICPENDRn */
+               .base           = 0x280,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_clear_pending_reg,
+       },
+       {                       /* ISACTIVERn */
+               .base           = 0x300,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* ICACTIVERn */
+               .base           = 0x380,
+               .len            = VGIC_NR_IRQS / 8,
+               .handle_mmio    = handle_mmio_raz_wi,
+       },
+       {                       /* IPRIORITYRn */
+               .base           = 0x400,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_priority_reg,
+       },
+       {                       /* ITARGETSRn */
+               .base           = 0x800,
+               .len            = VGIC_NR_IRQS,
+               .handle_mmio    = handle_mmio_target_reg,
+       },
+       {                       /* ICFGRn */
+               .base           = 0xC00,

arch/arm/include/asm/kvm_vgic.h

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +8 -0

+               .len            = VGIC_NR_IRQS / 4,
+               .handle_mmio    = handle_mmio_cfg_reg,
+       },
+       {                       /* SGIRn */
+               .base           = 0xF00,
+               .len            = 4,
+               .handle_mmio    = handle_mmio_sgi_reg,
+       },
        {}
 };

You've added named definitions for these constants to the GIC header file, so please replace these immediates with those and delete the comments.

From: Marc Zyngier <[email protected]>

Add VGIC virtual CPU interface code, picking pending interrupts from the distributor and stashing them in the VGIC control interface list registers.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   30 ++++
 arch/arm/kvm/vgic.c             |  327 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 356 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +12 -0

index 9ff0d9c..b3133c4 100644
@@ -110,8 +110,33 @@ struct vgic_dist {
 };
 
 struct vgic_cpu {
+#ifdef CONFIG_KVM_ARM_VGIC
+       /* per IRQ to LR mapping */
+       u8              vgic_irq_lr_map[VGIC_NR_IRQS];
+
+       /* Pending interrupts on this VCPU */
+       DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
+       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
+
+       /* Bitmap of used/free list registers */
+       DECLARE_BITMAP( lr_used, 64);
+
+       /* Number of list registers on this CPU */

arch/arm/include/asm/kvm_vgic.h +3 -0

+#endif
 };
 
+#define LR_EMPTY       0xff
+
 struct kvm;
 struct kvm_vcpu;
 struct kvm_run;

arch/arm/include/asm/kvm_vgic.h +3 -0

+int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
                      struct kvm_exit_mmio *mmio);
 
+#define irqchip_in_kernel(k)   (!!((k)->arch.vgic.vctrl_base))
+
 #else
 static inline int kvm_vgic_hyp_init(void)
 {

arch/arm/kvm/vgic.c +58 -1

index bd2bd7f..58237d5 100644
@@ -152,6 +152,34 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
        return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
 }
 
+static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
+}
+
+static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
+}
+
+static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
+}
+
+static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
+}
+
 static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
 {
        struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

@@ -711,7 +739,30 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 
 static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 {
-       return 0;
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
+       unsigned long pending_private, pending_shared;
+       int vcpu_id;
+
+       vcpu_id = vcpu->vcpu_id;
+       pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
+       pend_shared = vcpu->arch.vgic_cpu.pending_shared;
+
+       pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
+       enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
+       bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
+
+       pending = vgic_bitmap_get_shared_map(&dist->irq_state);
+       enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
+       bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
+       bitmap_and(pend_shared, pend_shared,
+                  vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
+                  VGIC_NR_SHARED_IRQS);
+
+       pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
+       pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
+       return (pending_private < VGIC_NR_PRIVATE_IRQS ||
+               pending_shared < VGIC_NR_SHARED_IRQS);
 }
 
 /*

@@ -737,6 +788,280 @@ static void vgic_update_state(struct kvm *kvm)
        }
 }
 
+#define LR_CPUID(lr)   \
+       (((lr) & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT)
+#define MK_LR_PEND(src, irq)   \
+       (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq))
+/*
+ * Queue an interrupt to a CPU virtual interface. Return true on success,

Quoted 33 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

Add VGIC virtual CPU interface code, picking pending interrupts from the distributor and stashing them in the VGIC control interface list registers.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   30 ++++
 arch/arm/kvm/vgic.c             |  327 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 356 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +12 -0

index 9ff0d9c..b3133c4 100644
@@ -110,8 +110,33 @@ struct vgic_dist {
 };
 
 struct vgic_cpu {
+#ifdef CONFIG_KVM_ARM_VGIC
+       /* per IRQ to LR mapping */
+       u8              vgic_irq_lr_map[VGIC_NR_IRQS];
+
+       /* Pending interrupts on this VCPU */
+       DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
+       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
+
+       /* Bitmap of used/free list registers */
+       DECLARE_BITMAP( lr_used, 64);
+
+       /* Number of list registers on this CPU */

arch/arm/include/asm/kvm_vgic.h struct vgic_dist { · line 134

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +10 -0

+       int             nr_lr;
+
+       /* CPU vif control registers for world switch */
+       u32             vgic_hcr;
+       u32             vgic_vmcr;
+       u32             vgic_misr;      /* Saved only */
+       u32             vgic_eisr[2];   /* Saved only */
+       u32             vgic_elrsr[2];  /* Saved only */
+       u32             vgic_apr;
+       u32             vgic_lr[64];    /* A15 has only 4... */

Have a #define for the maximum number of list registers.

Quoted 9 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+#endif
 };
 
+#define LR_EMPTY       0xff
+
 struct kvm;
 struct kvm_vcpu;
 struct kvm_run;

arch/arm/include/asm/kvm_vgic.h struct kvm_exit_mmio; · line 148

Quoted 5 lines

arch/arm/include/asm/kvm_vgic.h +2 -0

@@ -119,9 +144,14 @@ struct kvm_exit_mmio;
 
 #ifdef CONFIG_KVM_ARM_VGIC
 int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
+void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
+void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);

Same comment as for the arch timer (flush/sync).

Quoted 94 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
                      struct kvm_exit_mmio *mmio);
 
+#define irqchip_in_kernel(k)   (!!((k)->arch.vgic.vctrl_base))
+
 #else
 static inline int kvm_vgic_hyp_init(void)
 {

arch/arm/kvm/vgic.c +62 -1

index bd2bd7f..58237d5 100644
@@ -152,6 +152,34 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
        return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
 }
 
+static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
+}
+
+static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
+}
+
+static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
+}
+
+static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
+}
+
 static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
 {
        struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

@@ -711,7 +739,30 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 
 static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 {
-       return 0;
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
+       unsigned long pending_private, pending_shared;
+       int vcpu_id;
+
+       vcpu_id = vcpu->vcpu_id;
+       pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
+       pend_shared = vcpu->arch.vgic_cpu.pending_shared;
+
+       pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
+       enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
+       bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
+
+       pending = vgic_bitmap_get_shared_map(&dist->irq_state);
+       enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
+       bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
+       bitmap_and(pend_shared, pend_shared,
+                  vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
+                  VGIC_NR_SHARED_IRQS);
+
+       pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
+       pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
+       return (pending_private < VGIC_NR_PRIVATE_IRQS ||
+               pending_shared < VGIC_NR_SHARED_IRQS);
 }
 
 /*

@@ -737,6 +788,280 @@ static void vgic_update_state(struct kvm *kvm)
        }
 }
 
+#define LR_CPUID(lr)   \
+       (((lr) & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT)
+#define MK_LR_PEND(src, irq)   \
+       (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq))
+/*
+ * Queue an interrupt to a CPU virtual interface. Return true on success,
+ * or false if it wasn't possible to queue it.
+ */
+static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
+{

arch/arm/kvm/vgic.c static void vgic_update_state(struct kvm *kvm) · line 806

Quoted 6 lines

arch/arm/kvm/vgic.c +6 -0

+       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+       int lr;
+
+       /* Sanitize the input... */
+       BUG_ON(sgi_source_id & ~7);
+       BUG_ON(sgi_source_id && irq > 15);

You can use your new NR_SGIS definition here.

Quoted 158 lines · Will Deacon

Quoted 33 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

Add VGIC virtual CPU interface code, picking pending interrupts from the distributor and stashing them in the VGIC control interface list registers.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   30 ++++
 arch/arm/kvm/vgic.c             |  327 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 356 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +12 -0

index 9ff0d9c..b3133c4 100644
@@ -110,8 +110,33 @@ struct vgic_dist {
 };
 
 struct vgic_cpu {
+#ifdef CONFIG_KVM_ARM_VGIC
+       /* per IRQ to LR mapping */
+       u8              vgic_irq_lr_map[VGIC_NR_IRQS];
+
+       /* Pending interrupts on this VCPU */
+       DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
+       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
+
+       /* Bitmap of used/free list registers */
+       DECLARE_BITMAP( lr_used, 64);
+
+       /* Number of list registers on this CPU */

arch/arm/include/asm/kvm_vgic.h struct vgic_dist { · line 134

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +10 -0

+       int             nr_lr;
+
+       /* CPU vif control registers for world switch */
+       u32             vgic_hcr;
+       u32             vgic_vmcr;
+       u32             vgic_misr;      /* Saved only */
+       u32             vgic_eisr[2];   /* Saved only */
+       u32             vgic_elrsr[2];  /* Saved only */
+       u32             vgic_apr;
+       u32             vgic_lr[64];    /* A15 has only 4... */

Have a #define for the maximum number of list registers.

Quoted 9 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+#endif
 };
 
+#define LR_EMPTY       0xff
+
 struct kvm;
 struct kvm_vcpu;
 struct kvm_run;

arch/arm/include/asm/kvm_vgic.h struct kvm_exit_mmio; · line 148

Quoted 5 lines

arch/arm/include/asm/kvm_vgic.h +2 -0

@@ -119,9 +144,14 @@ struct kvm_exit_mmio;
 
 #ifdef CONFIG_KVM_ARM_VGIC
 int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
+void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
+void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);

Same comment as for the arch timer (flush/sync).

Quoted 94 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
                      struct kvm_exit_mmio *mmio);
 
+#define irqchip_in_kernel(k)   (!!((k)->arch.vgic.vctrl_base))
+
 #else
 static inline int kvm_vgic_hyp_init(void)
 {

arch/arm/kvm/vgic.c +62 -1

index bd2bd7f..58237d5 100644
@@ -152,6 +152,34 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
        return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
 }
 
+static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
+}
+
+static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
+}
+
+static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
+}
+
+static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
+}
+
 static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
 {
        struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

@@ -711,7 +739,30 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 
 static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 {
-       return 0;
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
+       unsigned long pending_private, pending_shared;
+       int vcpu_id;
+
+       vcpu_id = vcpu->vcpu_id;
+       pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
+       pend_shared = vcpu->arch.vgic_cpu.pending_shared;
+
+       pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
+       enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
+       bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
+
+       pending = vgic_bitmap_get_shared_map(&dist->irq_state);
+       enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
+       bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
+       bitmap_and(pend_shared, pend_shared,
+                  vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
+                  VGIC_NR_SHARED_IRQS);
+
+       pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
+       pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
+       return (pending_private < VGIC_NR_PRIVATE_IRQS ||
+               pending_shared < VGIC_NR_SHARED_IRQS);
 }
 
 /*

@@ -737,6 +788,280 @@ static void vgic_update_state(struct kvm *kvm)
        }
 }
 
+#define LR_CPUID(lr)   \
+       (((lr) & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT)
+#define MK_LR_PEND(src, irq)   \
+       (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq))
+/*
+ * Queue an interrupt to a CPU virtual interface. Return true on success,
+ * or false if it wasn't possible to queue it.
+ */
+static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
+{

Quoted 9 lines

arch/arm/kvm/vgic.c static void vgic_update_state(struct kvm *kvm) · line 806

Quoted 6 lines

arch/arm/kvm/vgic.c +6 -0

+       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+       int lr;
+
+       /* Sanitize the input... */
+       BUG_ON(sgi_source_id & ~7);
+       BUG_ON(sgi_source_id && irq > 15);

You can use your new NR_SGIS definition here.

This should address the remaining comments:

commit 43957095ec5476beb198f4c4630dfc3e2f3951db

Author: Christoffer Dall <[email protected]>
Date:   Mon Jan 14 16:59:38 2013 -0500

KVM: ARM: vgic: Define VGIC_MAX_LRS

Define maximum number of link registers we can handle instead of using literals in the code. If an architecture reports more link registers than we support, only use the number we can support.

Signed-off-by Christoffer Dall <[email protected]>

arch/arm/include/asm/kvm_vgic.h +0 -0

index 1ace491..f9d1977 100644

arch/arm/include/asm/kvm_vgic.h +2 -2

 /* Sanity checks... */
 #if (VGIC_MAX_CPUS > 8)

@@ -120,7 +121,7 @@ struct vgic_cpu {
 	DECLARE_BITMAP(	pending_shared, VGIC_NR_SHARED_IRQS);
 
 	/* Bitmap of used/free list registers */
-	DECLARE_BITMAP(	lr_used, 64);
+	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
 
 	/* Number of list registers on this CPU */
 	int		nr_lr;

@@ -132,7 +133,7 @@ struct vgic_cpu {
 	u32		vgic_eisr[2];	/* Saved only */
 	u32		vgic_elrsr[2];	/* Saved only */
 	u32		vgic_apr;
-	u32		vgic_lr[64];	/* A15 has only 4... */
+	u32		vgic_lr[VGIC_MAX_LRS];
 #endif
 };
 

arch/arm/kvm/vgic.c +0 -0

index a0d283c..90a99fd 100644

arch/arm/kvm/vgic.c +0 -1

 	ret = create_hyp_io_mappings(vgic_vctrl_base,
 				     vgic_vctrl_base + resource_size(&vctrl_res),
--

Thanks,
-Christoffer

Quoted 188 lines · Christoffer Dall

Quoted 158 lines · Will Deacon

Quoted 33 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

Add VGIC virtual CPU interface code, picking pending interrupts from the distributor and stashing them in the VGIC control interface list registers.

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 arch/arm/include/asm/kvm_vgic.h |   30 ++++
 arch/arm/kvm/vgic.c             |  327 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 356 insertions(+), 1 deletion(-)

arch/arm/include/asm/kvm_vgic.h +12 -0

index 9ff0d9c..b3133c4 100644
@@ -110,8 +110,33 @@ struct vgic_dist {
 };
 
 struct vgic_cpu {
+#ifdef CONFIG_KVM_ARM_VGIC
+       /* per IRQ to LR mapping */
+       u8              vgic_irq_lr_map[VGIC_NR_IRQS];
+
+       /* Pending interrupts on this VCPU */
+       DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
+       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
+
+       /* Bitmap of used/free list registers */
+       DECLARE_BITMAP( lr_used, 64);
+
+       /* Number of list registers on this CPU */

arch/arm/include/asm/kvm_vgic.h struct vgic_dist { · line 134

Quoted 10 lines

arch/arm/include/asm/kvm_vgic.h +10 -0

+       int             nr_lr;
+
+       /* CPU vif control registers for world switch */
+       u32             vgic_hcr;
+       u32             vgic_vmcr;
+       u32             vgic_misr;      /* Saved only */
+       u32             vgic_eisr[2];   /* Saved only */
+       u32             vgic_elrsr[2];  /* Saved only */
+       u32             vgic_apr;
+       u32             vgic_lr[64];    /* A15 has only 4... */

Have a #define for the maximum number of list registers.

Quoted 9 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+#endif
 };
 
+#define LR_EMPTY       0xff
+
 struct kvm;
 struct kvm_vcpu;
 struct kvm_run;

arch/arm/include/asm/kvm_vgic.h struct kvm_exit_mmio; · line 148

Quoted 5 lines

arch/arm/include/asm/kvm_vgic.h +2 -0

@@ -119,9 +144,14 @@ struct kvm_exit_mmio;
 
 #ifdef CONFIG_KVM_ARM_VGIC
 int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
+void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
+void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);

Same comment as for the arch timer (flush/sync).

Quoted 94 lines

arch/arm/include/asm/kvm_vgic.h +3 -0

+int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
                      struct kvm_exit_mmio *mmio);
 
+#define irqchip_in_kernel(k)   (!!((k)->arch.vgic.vctrl_base))
+
 #else
 static inline int kvm_vgic_hyp_init(void)
 {

arch/arm/kvm/vgic.c +62 -1

index bd2bd7f..58237d5 100644
@@ -152,6 +152,34 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
        return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
 }
 
+static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
+}
+
+static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
+}
+
+static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
+}
+
+static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
+{
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+       return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
+}
+
 static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
 {
        struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

@@ -711,7 +739,30 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 
 static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 {
-       return 0;
+       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
+       unsigned long pending_private, pending_shared;
+       int vcpu_id;
+
+       vcpu_id = vcpu->vcpu_id;
+       pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
+       pend_shared = vcpu->arch.vgic_cpu.pending_shared;
+
+       pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
+       enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
+       bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
+
+       pending = vgic_bitmap_get_shared_map(&dist->irq_state);
+       enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
+       bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
+       bitmap_and(pend_shared, pend_shared,
+                  vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
+                  VGIC_NR_SHARED_IRQS);
+
+       pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
+       pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
+       return (pending_private < VGIC_NR_PRIVATE_IRQS ||
+               pending_shared < VGIC_NR_SHARED_IRQS);
 }
 
 /*

@@ -737,6 +788,280 @@ static void vgic_update_state(struct kvm *kvm)
        }
 }
 
+#define LR_CPUID(lr)   \
+       (((lr) & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT)
+#define MK_LR_PEND(src, irq)   \
+       (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq))
+/*
+ * Queue an interrupt to a CPU virtual interface. Return true on success,
+ * or false if it wasn't possible to queue it.
+ */
+static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
+{

Quoted 9 lines

arch/arm/kvm/vgic.c static void vgic_update_state(struct kvm *kvm) · line 806

Quoted 6 lines

arch/arm/kvm/vgic.c +6 -0

+       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+       int lr;
+
+       /* Sanitize the input... */
+       BUG_ON(sgi_source_id & ~7);
+       BUG_ON(sgi_source_id && irq > 15);

You can use your new NR_SGIS definition here.

This should address the remaining comments:

commit 43957095ec5476beb198f4c4630dfc3e2f3951db

Author: Christoffer Dall <[email protected]>
Date:   Mon Jan 14 16:59:38 2013 -0500

KVM: ARM: vgic: Define VGIC_MAX_LRS

Define maximum number of link registers we can handle instead of using literals in the code. If an architecture reports more link registers than we support, only use the number we can support.

Signed-off-by Christoffer Dall <[email protected]>

arch/arm/include/asm/kvm_vgic.h +0 -0

index 1ace491..f9d1977 100644

arch/arm/include/asm/kvm_vgic.h line 36

Quoted 4 lines

arch/arm/include/asm/kvm_vgic.h +1 -0

@@ -33,6 +33,7 @@
 #define VGIC_NR_PRIVATE_IRQS (VGIC_NR_SGIS + VGIC_NR_PPIS)
 #define VGIC_NR_SHARED_IRQS  (VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
 #define VGIC_MAX_CPUS                KVM_MAX_VCPUS
+#define VGIC_MAX_LRS         64

Consider this instead (for the reason below)
#define VGIC_MAX_LRS (1 << 7)

Quoted 25 lines

arch/arm/include/asm/kvm_vgic.h +2 -2

 /* Sanity checks... */
 #if (VGIC_MAX_CPUS > 8)

@@ -120,7 +121,7 @@ struct vgic_cpu {
      DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
 
      /* Bitmap of used/free list registers */
-     DECLARE_BITMAP( lr_used, 64);
+     DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
 
      /* Number of list registers on this CPU */
      int             nr_lr;

@@ -132,7 +133,7 @@ struct vgic_cpu {
      u32             vgic_eisr[2];   /* Saved only */
      u32             vgic_elrsr[2];  /* Saved only */
      u32             vgic_apr;
-     u32             vgic_lr[64];    /* A15 has only 4... */
+     u32             vgic_lr[VGIC_MAX_LRS];
 #endif
 };
 

arch/arm/kvm/vgic.c +0 -0

index a0d283c..90a99fd 100644

arch/arm/kvm/vgic.c int kvm_vgic_hyp_init(void) · line 1347

Quoted 3 lines

arch/arm/kvm/vgic.c +0 -0

@@ -1345,6 +1345,8 @@ int kvm_vgic_hyp_init(void)
 
      vgic_nr_lr = readl_relaxed(vgic_vctrl_base + GICH_VTR);
      vgic_nr_lr = (vgic_nr_lr & 0x1f) + 1;

There is a bug here. It should be:

        vgic_nr_lr = (vgic_nr_lr & 0x2f) + 1;

arch/arm/kvm/vgic.c int kvm_vgic_hyp_init(void) · line 1349

Quoted 2 lines

arch/arm/kvm/vgic.c +2 -0

+     if (vgic_nr_lr > VGIC_MAX_LRS)
+             vgic_nr_lr = VGIC_MAX_LRS; /* TODO: Clear remaining LRs */

Why? VGIC_MAX_LRS isn't a configurable value, but a maximum value defined by the specification. This is the maximum you can fit in a 6 bit field, plus one (1 << 7, exactly).

Quoted 4 lines

arch/arm/include/asm/kvm_vgic.h +0 -0

index 1ace491..f9d1977 100644

arch/arm/include/asm/kvm_vgic.h line 36

Quoted 5 lines

arch/arm/include/asm/kvm_vgic.h +1 -0

@@ -33,6 +33,7 @@
 #define VGIC_NR_PRIVATE_IRQS (VGIC_NR_SGIS + VGIC_NR_PPIS)
 #define VGIC_NR_SHARED_IRQS  (VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
 #define VGIC_MAX_CPUS                KVM_MAX_VCPUS
+#define VGIC_MAX_LRS         64

Consider this instead (for the reason below)
#define VGIC_MAX_LRS (1 << 7)

Quoted 26 lines

arch/arm/include/asm/kvm_vgic.h +2 -2

 /* Sanity checks... */
 #if (VGIC_MAX_CPUS > 8)

@@ -120,7 +121,7 @@ struct vgic_cpu {
      DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
 
      /* Bitmap of used/free list registers */
-     DECLARE_BITMAP( lr_used, 64);
+     DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
 
      /* Number of list registers on this CPU */
      int             nr_lr;

@@ -132,7 +133,7 @@ struct vgic_cpu {
      u32             vgic_eisr[2];   /* Saved only */
      u32             vgic_elrsr[2];  /* Saved only */
      u32             vgic_apr;
-     u32             vgic_lr[64];    /* A15 has only 4... */
+     u32             vgic_lr[VGIC_MAX_LRS];
 #endif
 };
 

arch/arm/kvm/vgic.c +0 -0

index a0d283c..90a99fd 100644
@@ -1345,6 +1345,8 @@ int kvm_vgic_hyp_init(void)