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.

From: Marc Zyngier <[email protected]>

It is now possible to select the VGIC configuration option.

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

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

---
  arch/arm/kvm/Kconfig |    8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/kvm/Kconfig +0 -0

index 05227cb..d32e33f 100644

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...

Hello.

Quoted 21 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

It is now possible to select the VGIC configuration option.

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

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

---
  arch/arm/kvm/Kconfig |    8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/kvm/Kconfig +0 -0

index 05227cb..d32e33f 100644
@@ -51,6 +51,14 @@ config KVM_ARM_MAX_VCPUS
          large, so only choose a reasonable number that you expect to
          actually use.
 

arch/arm/kvm/Kconfig config KVM_ARM_MAX_VCPUS · line 55

Quoted 2 lines

arch/arm/kvm/Kconfig +2 -0

+config KVM_ARM_VGIC
+        bool "KVM support for Virtual GIC"

Please indent with tab, as below.

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a better generic interface.

Once we agree on a better interface the KVM/ARM code can also take advantage of that, but until then we don't want to hold up the KVM/ARM patches.

A new complete series of these patches including this change can be pulled from:
git://github.com/virtualopensystems/linux-kvm-arm.git
kvm-arm-v15-devaddr-v2

-Christoffer

---

Christoffer Dall (2):
KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl
ARM: KVM: VGIC accept vcpu and dist base addresses from user space

Documentation/virtual/kvm/api.txt   |   38 ++++++
arch/arm/include/asm/hardware/gic.h |   25 ++++
arch/arm/include/asm/kvm_host.h     |   18 +++
arch/arm/include/asm/kvm_vgic.h     |   89 +++++++++++++++
arch/arm/include/uapi/asm/kvm.h     |   16 +++
arch/arm/kvm/Makefile               |    1 
arch/arm/kvm/arm.c                  |   92 +++++++++++++++-
arch/arm/kvm/interrupts.S           |    4 +
arch/arm/kvm/mmio.c                 |    3 +
arch/arm/kvm/vgic.c                 |  206 +++++++++++++++++++++++++++++++++++
include/uapi/linux/kvm.h            |    8 +
11 files changed, 498 insertions(+), 2 deletions(-)

create mode 100644 arch/arm/include/asm/kvm_vgic.h create mode 100644 arch/arm/kvm/vgic.c

Signature · 4 lines

_______________________________________________
kvmarm mailing list
kvmarm at lists.cs.columbia.edu
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Quoted 2 lines · Christoffer Dall

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a better

generic

Quoted 4 lines

interface.

Once we agree on a better interface the KVM/ARM code can also take advantage of that, but until then we don't want to hold up the

KVM/ARM

Quoted 1 line

patches.

Works for me. Scott, are you happy with this one too?

We can start to introduce (and fix ARM) with a generic ioctl in the MPIC patches then.

Quoted 5 lines · Alexander Graf

Quoted 4 lines · Christoffer Dall

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a better generic interface.

Quoted 4 lines

Quoted 3 lines

Once we agree on a better interface the KVM/ARM code can also take advantage of that, but until then we don't want to hold up the KVM/ARM patches.

Works for me. Scott, are you happy with this one too?

Not really, given that it will stay around forever even after something new is introduced.

If you're going to change the name, why not just change it to KVM_SET_DEVICE_CONFIG? Can we change the name later if nothing else changes (so it's still binary compatible)?

Quoted 1 line

We can start to introduce (and fix ARM) with a generic ioctl in the MPIC patches then.

The ioctl is already generic, except for its name.

Quoted 4 lines · Scott Wood

Quoted 3 lines · Alexander Graf

Quoted 2 lines · Christoffer Dall

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a better

generic

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

interface.

Once we agree on a better interface the KVM/ARM code can also

take

Quoted 1 line

Quoted 1 line

Quoted 1 line

advantage of that, but until then we don't want to hold up the

KVM/ARM

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

patches.

Works for me. Scott, are you happy with this one too?

Not really, given that it will stay around forever even after

something new is introduced.

But only in ARM specific code.

Quoted 1 line

If you're going to change the name, why not just change it to

KVM_SET_DEVICE_CONFIG? Can we change the name later if nothing else changes (so it's still binary compatible)?

Because that again implies that it's generic enough. And to reach that conclusion will take more time than we should spend on this now.

Quoted 1 line

Quoted 1 line

We can start to introduce (and fix ARM) with a generic ioctl in

the MPIC patches then.

Quoted 2 lines

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:

* maximum size of value is u64

* combining device id (variable) with addr type id (const) into a
single field. It could just be split into multiple fields

* the id is 100% architecture specific. It shouldn't be. At least
the "device id" field should be generic.

I'm not sure if we can come up with more problems in that API when staring at it a bit longer and/or we would actually start using it for more things. So for the sake of not holding up the ARM code, I'm perfectly fine to clutter ARM's ioctl handling code with an ioctl that is already deprecated at its introduction, as long as we don't hold everything else up meanwhile.

Quoted 10 lines · Scott Wood

Quoted 9 lines · Alexander Graf

Quoted 8 lines · Christoffer Dall

Quoted 7 lines

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a better generic interface.

Once we agree on a better interface the KVM/ARM code can also take advantage of that, but until then we don't want to hold up the KVM/ARM patches.

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

Works for me. Scott, are you happy with this one too?

Not really, given that it will stay around forever even after something new is introduced.

But only in ARM specific code.

...which I'll probably have to deal with when Freescale's virtualization-capable ARM chips come along. I don't see "it's only in that other architecture" as "it might as well not exist".

Quoted 3 lines

Quoted 2 lines

If you're going to change the name, why not just change it to KVM_SET_DEVICE_CONFIG? Can we change the name later if nothing else changes (so it's still binary compatible)?

Because that again implies that it's generic enough. And to reach that conclusion will take more time than we should spend on this now.

If the conclusion later on is that it is good enough, can the name be changed then?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

We can start to introduce (and fix ARM) with a generic ioctl in the MPIC patches then.

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:
* maximum size of value is u64

This is tolerable IMHO.

Quoted 1 line

* combining device id (variable) with addr type id (const) into a single field. It could just be split into multiple fields

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Perhaps the threshold for an API becoming "permanent" should not be acceptance into the tree, but rather the removal of an "experimental" tag (including a way of shutting off experimental APIs to make sure you're not depending on them). Sort of like CONFIG_EXPERIMENTAL, except actually used for its intended purpose (distributions should have it *off* by default), and preferably managed at runtime. Sort of like drivers/staging, except for APIs rather than drivers. Changes at that point should require more justification than before merging, but would not have the strict compatibility requirement that non-experimental APIs have. This would make collaboration and testing easier on APIs that aren't ready to be permanent.

Quoted 2 lines

* the id is 100% architecture specific. It shouldn't be. At least the "device id" field should be generic.

That's a documentation issue that could be changed to have all architectures adopt what is currently specified for ARM, without breaking compatibility.

Quoted 2 lines

I'm not sure if we can come up with more problems in that API when staring at it a bit longer and/or we would actually start using it for more things. So for the sake of not holding up the ARM code, I'm perfectly fine to clutter ARM's ioctl handling code with an ioctl that is already deprecated at its introduction, as long as we don't hold everything else up meanwhile.

I'm not in a position to block it, and if I were I presumably would have seen this in time for feedback to matter. That's different from actually being happy. :-)

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 1 line

I get that there's a tradeoff between getting something in now,

versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Then we're the ones who have to come up with a good interface.

Quoted 1 line

Perhaps the threshold for an API becoming "permanent" should not be

acceptance into the tree, but rather the removal of an "experimental" tag (including a way of shutting off experimental APIs to make sure you're not depending on them). Sort of like CONFIG_EXPERIMENTAL, except actually used for its intended purpose (distributions should have it *off* by default), and preferably managed at runtime. Sort of like drivers/staging, except for APIs rather than drivers. Changes at that point should require more justification than before merging, but would not have the strict compatibility requirement that non-experimental APIs have. This would make collaboration and testing easier on APIs that aren't ready to be permanent.

This tag does exist. It's called "not in Linus' tree" :).

Quoted 11 lines · Alexander Graf

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 5 lines

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Quoted 1 line

Then we're the ones who have to come up with a good interface.

How about another bad one, with PPC in the name, and some pleas to hurry things up? :-)

It's not as if there haven't been last-minute requests for API changes on the PPC side in the past...

Quoted 3 lines · Alexander Graf

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 1 line

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Then we're the ones who have to come up with a good interface.

How about another bad one, with PPC in the name, and some pleas to hurry things up? :-)

It's not as if there haven't been last-minute requests for API changes on the PPC side in the past...

Quoted 3 lines

Quoted 2 lines

Perhaps the threshold for an API becoming "permanent" should not be acceptance into the tree, but rather the removal of an "experimental" tag (including a way of shutting off experimental APIs to make sure you're not depending on them). Sort of like CONFIG_EXPERIMENTAL, except actually used for its intended purpose (distributions should have it *off* by default), and preferably managed at runtime. Sort of like drivers/staging, except for APIs rather than drivers. Changes at that point should require more justification than before merging, but would not have the strict compatibility requirement that non-experimental APIs have. This would make collaboration and testing easier on APIs that aren't ready to be permanent.

This tag does exist. It's called "not in Linus' tree" :).

Which makes it a pain for multiple people to work on a new feature, especially when it spans components such as KVM and QEMU, and means that it gets less testing before the point of no return.

Quoted 12 lines · Scott Wood

Quoted 11 lines · Alexander Graf

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 5 lines

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Quoted 10 lines

Quoted 1 line

Then we're the ones who have to come up with a good interface.

How about another bad one, with PPC in the name, and some pleas to hurry things up? :-)

It's not as if there haven't been last-minute requests for API changes on the PPC side in the past...

This is getting out of hand.

Do you have another API for PPC, which was send for review and not commented on several months ago that we can unify right now?

If not, let's go with the ARM name and work on the generic API in the mean time.

The end result will be something along 5 lines in a header files and 3 lines in a switch case that return -EINVAL if the interface is completely deprecated later on, which is not a big problem.

In fact, in this particular case one could merge all of the patches except for the particular ioctl implementation and just give the respective addresses default values until there is an API to set them, similar to how we did things with PVR in the beginning.

Quoted 4 lines

In fact, in this particular case one could merge all of the patches except for the particular ioctl implementation and just give the respective addresses default values until there is an API to set them, similar to how we did things with PVR in the beginning.

That's what we started with and we got review comments saying "yuck, don't hard code this"...

-- PMM

Quoted 31 lines · Christoffer Dall

Quoted 12 lines · Scott Wood

Quoted 11 lines · Alexander Graf

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 5 lines

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Quoted 10 lines

Quoted 1 line

Then we're the ones who have to come up with a good interface.

How about another bad one, with PPC in the name, and some pleas to hurry things up? :-)

It's not as if there haven't been last-minute requests for API changes on the PPC side in the past...

This is getting out of hand.

Do you have another API for PPC, which was send for review and not commented on several months ago that we can unify right now?

If not, let's go with the ARM name and work on the generic API in the mean time.

Quoted 3 lines

The end result will be something along 5 lines in a header files and 3 lines in a switch case that return -EINVAL if the interface is completely deprecated later on, which is not a big problem.

Agreed [1].

So what exactly are we waiting for? Acks from kvm maintainers, right?

Quoted 38 lines · Alexander Graf

Quoted 34 lines · Christoffer Dall

Quoted 12 lines · Scott Wood

Quoted 11 lines · Alexander Graf

Am 09.01.2013 um 22:15 schrieb Scott Wood <[email protected]>:

Quoted 5 lines

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Quoted 10 lines

Quoted 1 line

Then we're the ones who have to come up with a good interface.

How about another bad one, with PPC in the name, and some pleas to hurry things up? :-)

It's not as if there haven't been last-minute requests for API changes on the PPC side in the past...

This is getting out of hand.

Do you have another API for PPC, which was send for review and not commented on several months ago that we can unify right now?

If not, let's go with the ARM name and work on the generic API in the mean time.

The end result will be something along 5 lines in a header files and 3 lines in a switch case that return -EINVAL if the interface is completely deprecated later on, which is not a big problem.

Quoted 3 lines

Agreed [1].

So what exactly are we waiting for? Acks from kvm maintainers, right?

In fact, we should probably CC them :)

Quoted 7 lines · Alexander Graf

In fact, in this particular case one could merge all of the patches except for the
particular ioctl implementation and just give the respective addresses default
values until there is an API to set them, similar to how we did things with PVR
in the beginning.

That's what we started with and we got review comments saying "yuck, don't hard code this"...

Quoted 9 lines · Peter Maydell

Quoted 7 lines · Alexander Graf

In fact, in this particular case one could merge all of the patches except for the
particular ioctl implementation and just give the respective addresses default
values until there is an API to set them, similar to how we did things with PVR
in the beginning.

Quoted 2 lines

That's what we started with and we got review comments saying "yuck, don't hard code this"...

Even worse: hard-coding things breaks kvmtool (we do not emulate a vexpress and have a rather different memory map) and arm64 (the requirements on GIC mappings are different).

So hard coding addresses is already unacceptable.

Quoted 17 lines · Alexander Graf

Quoted 4 lines · Scott Wood

Quoted 3 lines · Alexander Graf

Quoted 2 lines · Christoffer Dall

Renames the KVM_SET_DEVICE_ADDRESS to KVM_ARM_SET_DEVICE_ADDR to make it obvious that this is ARM specific in lack of a

better generic

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

interface.

Once we agree on a better interface the KVM/ARM code can also

take

Quoted 1 line

Quoted 1 line

Quoted 1 line

advantage of that, but until then we don't want to hold up

the KVM/ARM

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

patches.

Works for me. Scott, are you happy with this one too?

Quoted 4 lines

Quoted 1 line

Not really, given that it will stay around forever even after

something new is introduced.

But only in ARM specific code.

...which I'll probably have to deal with when Freescale's virtualization-capable ARM chips come along. I don't see "it's only in that other architecture" as "it might as well not exist".

Quoted 7 lines

Quoted 1 line

If you're going to change the name, why not just change it to

KVM_SET_DEVICE_CONFIG? Can we change the name later if nothing else changes (so it's still binary compatible)?

Because that again implies that it's generic enough. And to reach that conclusion will take more time than we should spend on this now.

If the conclusion later on is that it is good enough, can the name be changed then?

Quoted 8 lines

Quoted 1 line

Quoted 1 line

We can start to introduce (and fix ARM) with a generic ioctl in

the MPIC patches then.

Quoted 2 lines

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:

* maximum size of value is u64

This is tolerable IMHO.

Quoted 2 lines

* combining device id (variable) with addr type id (const) into
a single field. It could just be split into multiple fields

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Perhaps the threshold for an API becoming "permanent" should not be acceptance into the tree, but rather the removal of an "experimental" tag (including a way of shutting off experimental APIs to make sure you're not depending on them). Sort of like CONFIG_EXPERIMENTAL, except actually used for its intended purpose (distributions should have it *off* by default), and preferably managed at runtime. Sort of like drivers/staging, except for APIs rather than drivers. Changes at that point should require more justification than before merging, but would not have the strict compatibility requirement that non-experimental APIs have. This would make collaboration and testing easier on APIs that aren't ready to be permanent.

Quoted 2 lines

* the id is 100% architecture specific. It shouldn't be. At
least the "device id" field should be generic.

That's a documentation issue that could be changed to have all architectures adopt what is currently specified for ARM, without breaking compatibility.

Quoted 6 lines

I'm not sure if we can come up with more problems in that API when staring at it a bit longer and/or we would actually start using it for more things. So for the sake of not holding up the ARM code, I'm perfectly fine to clutter ARM's ioctl handling code with an ioctl that is already deprecated at its introduction, as long as we don't hold everything else up meanwhile.

I'm not in a position to block it, and if I were I presumably would have seen this in time for feedback to matter. That's different from actually being happy. :-)

-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

We can start to introduce (and fix ARM) with a generic ioctl in the MPIC patches then.

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:
* maximum size of value is u64

Quoted 7 lines

This is tolerable IMHO.

Quoted 1 line

* combining device id (variable) with addr type id (const) into a single field. It could just be split into multiple fields

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Then we're the ones who have to come up with a good interface.

Quoted 2 lines

Perhaps the threshold for an API becoming "permanent" should not be acceptance into the tree, but rather the removal of an "experimental" tag (including a way of shutting off experimental APIs to make sure you're not depending on them). Sort of like CONFIG_EXPERIMENTAL, except actually used for its intended purpose (distributions should have it *off* by default), and preferably managed at runtime. Sort of like drivers/staging, except for APIs rather than drivers. Changes at that point should require more justification than before merging, but would not have the strict compatibility requirement that non-experimental APIs have. This would make collaboration and testing easier on APIs that aren't ready to be permanent.

This tag does exist. It's called "not in Linus' tree" :).

Quoted 4 lines

Quoted 2 lines

* the id is 100% architecture specific. It shouldn't be. At least the "device id" field should be generic.

That's a documentation issue that could be changed to have all architectures adopt what is currently specified for ARM, without breaking compatibility.

Depends on how we want to design the final layout. I don't have the impression that we've reached final conclusion on this interface. That's all I'm saying.

Quoted 4 lines

Quoted 2 lines

I'm not sure if we can come up with more problems in that API when staring at it a bit longer and/or we would actually start using it for more things. So for the sake of not holding up the ARM code, I'm perfectly fine to clutter ARM's ioctl handling code with an ioctl that is already deprecated at its introduction, as long as we don't hold everything else up meanwhile.

I'm not in a position to block it, and if I were I presumably would have seen this in time for feedback to matter. That's different from actually being happy. :-)

For the ARM specific ioctl it has my ack. I'd say let's go with that and start to work on a good interface ASAP. But we need an ok from Marcelo or Gleb too.

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Alexander Graf

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

We can start to introduce (and fix ARM) with a generic ioctl

in the MPIC patches then.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:
* maximum size of value is u64

This is tolerable IMHO.

Quoted 1 line

* combining device id (variable) with addr type id (const) into

a single field. It could just be split into multiple fields

Quoted 4 lines

Quoted 4 lines

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in now,

versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

As mentioned, i fail to see the benefit in sharing 0.0x% of the code (the
interface), while the remaining code is not shared.

Quoted 1 line

Then we're the ones who have to come up with a good interface.

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Quoted 24 lines · Marcelo Tosatti

Quoted 3 lines · Alexander Graf

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

We can start to introduce (and fix ARM) with a generic

ioctl in the MPIC patches then.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:
* maximum size of value is u64

This is tolerable IMHO.

Quoted 1 line

* combining device id (variable) with addr type id (const)

into a single field. It could just be split into multiple fields

Quoted 4 lines

Quoted 4 lines

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in

now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

Quoted 3 lines

As mentioned, i fail to see the benefit in sharing 0.0x% of the code (the
interface), while the remaining code is not shared.

Pointlessly making things architecture-specific just makes more work for people who later need the functionality on another architecture. It might not be much in this case (unless a particular device ends up being used on multiple architectures), but the principle still applies. It's also more work for tools like strace, and could get in the way of the userspace caller using common code.

Quoted 3 lines

Quoted 1 line

Then we're the ones who have to come up with a good interface.

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

Quoted 2 lines · Scott Wood

Quoted 1 line · Marcelo Tosatti

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to

that?

Quoted 1 line

Quoted 4 lines

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

I did not really grasp that ('shouldnt be specific to addresses'), but anyway.

OK, can you write down your proposed improvements to the interface? In case you have something ready, otherwise there is time pressure to merge the ARM port.

That is, if you have interest/energy to spend in a possibly reusable interface, as long as that does not delay integration of the ARM code, i don't think the ARM people will mind that.

Quoted 10 lines · Marcelo Tosatti

Quoted 7 lines · Scott Wood

Quoted 1 line · Marcelo Tosatti

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

I did not really grasp that ('shouldnt be specific to addresses'), but anyway.

A device may have other configuration parameters that need to be set, besides addresses. PPC MPIC will require information about the vendor and version, for example.

Quoted 3 lines

OK, can you write down your proposed improvements to the interface? In case you have something ready, otherwise there is time pressure to merge the ARM port.

My original request was just to change the name to something like KVM_SET_DEVICE_CONFIG or KVM_SET_DEVICE_ATTR, and not make the id encoding architecture-specific (preferably, separate into a "device id" field and an "attribute id" field rather than using bitfields). Actual values for device id could be architecture-specific (or there could be a global enumeration), and attribute id values would be device-specific.

Alex suggested that an ideal interface might accept values larger than 64 bits, though I think it's good enough -- there are currently no proposed uses that need more than 64 bits for a single attribute (unlike ONE_REG), and if it is needed, such configuration could be split up between multiple attributes, or the attribute could specify that "value" be a userspace pointer to the actual data (as with ONE_REG).

Here's a writeup (the ARM details would go under ARM/vGIC-specific documentation):

4.80 KVM_SET_DEVICE_ATTR

Capability: KVM_CAP_SET_DEVICE_ATTR
Type: vm ioctl
Parameters: struct kvm_device_attr (in)
Returns: 0 on success, -1 on error
Errors:
ENODEV: The device id is unknown
ENXIO: Device not supported on current system
Other errors may be returned by specific devices and attributes.

struct kvm_device_attr {
   __u32 device;
   __u32 attr;
   __u64 value;
};

Specify an attribute of a device emulated or directly exposed by the kernel, which the host kernel needs to know about. The device field is an architecture-specific identifier for a specific device. The attr field is a device-specific identifier for a specific attribute. Individual attributes may have particular requirements for when they can and cannot be set.

Quoted 3 lines

That is, if you have interest/energy to spend in a possibly reusable interface, as long as that does not delay integration of the ARM code, i don't think the ARM people will mind that.

The impression I've been given is that just about any change will delay the integration at this point. If that's the case, and everyone's OK with having an interface that is deprecated on arrival, then fine.

Quoted 10 lines · Marcelo Tosatti

Quoted 7 lines · Scott Wood

Quoted 1 line · Marcelo Tosatti

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

I did not really grasp that ('shouldnt be specific to addresses'), but anyway.

A device may have other configuration parameters that need to be set, besides addresses. PPC MPIC will require information about the vendor and version, for example.

Quoted 3 lines

OK, can you write down your proposed improvements to the interface? In case you have something ready, otherwise there is time pressure to merge the ARM port.

My original request was just to change the name to something like KVM_SET_DEVICE_CONFIG or KVM_SET_DEVICE_ATTR, and not make the id encoding architecture-specific (preferably, separate into a "device id" field and an "attribute id" field rather than using bitfields). Actual values for device id could be architecture-specific (or there could be a global enumeration), and attribute id values would be device-specific.

Alex suggested that an ideal interface might accept values larger than 64 bits, though I think it's good enough -- there are currently no proposed uses that need more than 64 bits for a single attribute (unlike ONE_REG), and if it is needed, such configuration could be split up between multiple attributes, or the attribute could specify that "value" be a userspace pointer to the actual data (as with ONE_REG).

Here's a writeup (the ARM details would go under ARM/vGIC-specific documentation):

4.80 KVM_SET_DEVICE_ATTR

Capability: KVM_CAP_SET_DEVICE_ATTR
Type: vm ioctl
Parameters: struct kvm_device_attr (in)
Returns: 0 on success, -1 on error
Errors:
ENODEV: The device id is unknown
ENXIO: Device not supported on current system
Other errors may be returned by specific devices and attributes.

struct kvm_device_attr {
	__u32 device;

	__u32 attr;
	__u64 value;
};

Specify an attribute of a device emulated or directly exposed by the kernel, which the host kernel needs to know about. The device field is an architecture-specific identifier for a specific device. The attr field is a device-specific identifier for a specific attribute. Individual attributes may have particular requirements for when they can and cannot be set.

Quoted 3 lines

That is, if you have interest/energy to spend in a possibly reusable interface, as long as that does not delay integration of the ARM code, i don't think the ARM people will mind that.

The impression I've been given is that just about any change will delay the integration at this point. If that's the case, and everyone's OK with having an interface that is deprecated on arrival, then fine.

Quoted 61 lines · Scott Wood

Quoted 10 lines · Marcelo Tosatti

Quoted 7 lines · Scott Wood

Quoted 1 line · Marcelo Tosatti

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

I did not really grasp that ('shouldnt be specific to addresses'), but anyway.

A device may have other configuration parameters that need to be set, besides addresses. PPC MPIC will require information about the vendor and version, for example.

Quoted 3 lines

OK, can you write down your proposed improvements to the interface? In case you have something ready, otherwise there is time pressure to merge the ARM port.

My original request was just to change the name to something like KVM_SET_DEVICE_CONFIG or KVM_SET_DEVICE_ATTR, and not make the id encoding architecture-specific (preferably, separate into a "device id" field and an "attribute id" field rather than using bitfields). Actual values for device id could be architecture-specific (or there could be a global enumeration), and attribute id values would be device-specific.

Alex suggested that an ideal interface might accept values larger than 64 bits, though I think it's good enough -- there are currently no proposed uses that need more than 64 bits for a single attribute (unlike ONE_REG), and if it is needed, such configuration could be split up between multiple attributes, or the attribute could specify that "value" be a userspace pointer to the actual data (as with ONE_REG).

Here's a writeup (the ARM details would go under ARM/vGIC-specific documentation):

4.80 KVM_SET_DEVICE_ATTR

Capability: KVM_CAP_SET_DEVICE_ATTR
Type: vm ioctl
Parameters: struct kvm_device_attr (in)
Returns: 0 on success, -1 on error
Errors:
ENODEV: The device id is unknown
ENXIO: Device not supported on current system
Other errors may be returned by specific devices and attributes.

struct kvm_device_attr {
   __u32 device;
   __u32 attr;
   __u64 value;
};

Specify an attribute of a device emulated or directly exposed by the kernel, which the host kernel needs to know about. The device field is an architecture-specific identifier for a specific device. The attr field is a device-specific identifier for a specific attribute. Individual attributes may have particular requirements for when they can and cannot be set.

Quoted 7 lines

Quoted 3 lines

That is, if you have interest/energy to spend in a possibly reusable interface, as long as that does not delay integration of the ARM code, i don't think the ARM people will mind that.

The impression I've been given is that just about any change will delay the integration at this point. If that's the case, and everyone's OK with having an interface that is deprecated on arrival, then fine.

That is not entirely the case, but there wasn't event agreement on this revised API, and we didn't want to wait for weeks until a decision was made. Alex suggested we use a DEV_REG API similar to the ONE_REG API, but I am quite strongly against having such an API for this specific purpose as it is too semantically distant to what we do on ARM. (Having a DEV_REG API for other purposes might be fine though).

So I have no problem with your suggestion, although we could consider having a size and addr field in the struct instead and be slightly more extensible. But I don't feel strongly about it.

If we can agree on Scott's suggestion or with my modification (Alex, Gleb, Marcelo ?) then I'll change the KVM/ARM patches to use this and resend them right away. But we have to agree now!

If not, I really think we should keep things as they are now, as we're really discussing idealistic scenarios here, and the ARM patches have been out of tree for long enough. As Marcelo pointed out, the benefits of the perfect API are really minimal.

-Christoffer--

Quoted 76 lines · Christoffer Dall

Quoted 61 lines · Scott Wood

Quoted 10 lines · Marcelo Tosatti

Quoted 7 lines · Scott Wood

Quoted 1 line · Marcelo Tosatti

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Besides the above, and my original complaint that it shouldn't be specific to addresses?

-Scott

I did not really grasp that ('shouldnt be specific to addresses'), but anyway.

A device may have other configuration parameters that need to be set, besides addresses. PPC MPIC will require information about the vendor and version, for example.

Quoted 3 lines

OK, can you write down your proposed improvements to the interface? In case you have something ready, otherwise there is time pressure to merge the ARM port.

My original request was just to change the name to something like KVM_SET_DEVICE_CONFIG or KVM_SET_DEVICE_ATTR, and not make the id encoding architecture-specific (preferably, separate into a "device id" field and an "attribute id" field rather than using bitfields). Actual values for device id could be architecture-specific (or there could be a global enumeration), and attribute id values would be device-specific.

Alex suggested that an ideal interface might accept values larger than 64 bits, though I think it's good enough -- there are currently no proposed uses that need more than 64 bits for a single attribute (unlike ONE_REG), and if it is needed, such configuration could be split up between multiple attributes, or the attribute could specify that "value" be a userspace pointer to the actual data (as with ONE_REG).

Here's a writeup (the ARM details would go under ARM/vGIC-specific documentation):

4.80 KVM_SET_DEVICE_ATTR

Capability: KVM_CAP_SET_DEVICE_ATTR
Type: vm ioctl
Parameters: struct kvm_device_attr (in)
Returns: 0 on success, -1 on error
Errors:

ENODEV: The device id is unknown
ENXIO:  Device not supported on current system

Other errors may be returned by specific devices and attributes.

struct kvm_device_attr {
__u32 device;
__u32 attr;
__u64 value;
};

Specify an attribute of a device emulated or directly exposed by the kernel, which the host kernel needs to know about. The device field is an architecture-specific identifier for a specific device. The attr field is a device-specific identifier for a specific attribute. Individual attributes may have particular requirements for when they can and cannot be set.

Quoted 7 lines

Quoted 3 lines

That is, if you have interest/energy to spend in a possibly reusable interface, as long as that does not delay integration of the ARM code, i don't think the ARM people will mind that.

The impression I've been given is that just about any change will delay the integration at this point. If that's the case, and everyone's OK with having an interface that is deprecated on arrival, then fine.

That is not entirely the case, but there wasn't event agreement on this revised API, and we didn't want to wait for weeks until a decision was made. Alex suggested we use a DEV_REG API similar to the ONE_REG API, but I am quite strongly against having such an API for this specific purpose as it is too semantically distant to what we do on ARM. (Having a DEV_REG API for other purposes might be fine though).

So I have no problem with your suggestion, although we could consider having a size and addr field in the struct instead and be slightly more extensible. But I don't feel strongly about it.

Quoted 5 lines

If we can agree on Scott's suggestion or with my modification (Alex, Gleb, Marcelo ?) then I'll change the KVM/ARM patches to use this and resend them right away. But we have to agree now!

If not, I really think we should keep things as they are now, as we're really discussing idealistic scenarios here, and the ARM patches have been out of tree for long enough. As Marcelo pointed out, the benefits of the perfect API are really minimal.

-Christoffer--

Can you make KVM_SET_ARM_DEVICE address extensible? Add some reserved space and a flags field.

Quoted 10 lines · Alexander Graf

Quoted 10 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

We can start to introduce (and fix ARM) with a generic ioctl in the MPIC patches then.

The ioctl is already generic, except for its name.

It's making a few wrong assumptions:
* maximum size of value is u64

This is tolerable IMHO.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

* combining device id (variable) with addr type id (const) into a single field. It could just be split into multiple fields

I agree, but that could be lived with as well.

I get that there's a tradeoff between getting something in now, versus waiting until the API is more refined. Tagging it with a particular ISA seems like an odd way of saying "soon to be deprecated", though. What happens if we're still squabbling over the perfect replacement API when we're trying to push PPC MPIC stuff in?

As mentioned, i fail to see the benefit in sharing 0.0x% of the code (the interface), while the remaining code is not shared.

Quoted 2 lines

Then we're the ones who have to come up with a good interface.

Or just have KVM_SET_PPC_DEVICE_ADDRESS. Is there a downside to that?

Quoted 2 lines

Can you make KVM_SET_ARM_DEVICE address extensible? Add some reserved space and a flags field.

Can't we do this for the new ioctl that we all would agree on rather than the interim one that's only a short term solution for a greater problem?

Alex

Quoted 7 lines

Quoted 2 lines

Can you make KVM_SET_ARM_DEVICE address extensible? Add some reserved space and a flags field.

Can't we do this for the new ioctl that we all would agree on rather than the interim one that's only a short term solution for a greater problem?

Alex

OK.

Quoted 2 lines · Scott Wood

struct kvm_device_attr {
	__u32 device;

This needs some semantic specification. Is device a constant value? Is it the return value of CREATE_IRQCHIP?

Quoted 4 lines · Scott Wood

Quoted 2 lines

struct kvm_device_attr {
   __u32 device;

This needs some semantic specification. Is device a constant value? Is it the return value of CREATE_IRQCHIP?

As proposed, it's up to the architecture to provide that specification. In theory this could be used for things other than IRQ chips. We could still say that device creation functions return a valid device ID (if the device has any attributes), as well as have other architecture-specific ways of describing device IDs (static enumeration). Or we could have non-architecture-specific static enumeration. Or just require that all devices be explicitly created by something that returns the ID.

Do you have a preferred approach?

-Scott

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 {

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,

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 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