Re: [PATCH 07/50] KVM: PPC: Add generic single register ioctls

[Scott Wood <scottwood@freescale.com>]

On 01/03/2012 07:10 PM, Alexander Graf wrote:
> Right now we transfer a static struct every time we want to get or set
> registers. Unfortunately, over time we realize that there are more of
> these than we thought of before and the extensibility and flexibility of
> transferring a full struct every time is limited.
> 
> So this is a new approach to the problem. With these new ioctls, we can
> get and set a single register that is identified by an ID. This allows for
> very precise and limited transmittal of data. When we later realize that
> it's a better idea to shove over multiple registers at once, we can reuse
> most of the infrastructure and simply implement a GET_MANY_REGS / SET_MANY_REGS
> interface.

If we end up replacing get/set sregs with this (either totally or just
for new registers), we should consider a runtime facility for userspace
to extract from the kernel a list of supported registers (with sizes)
that are supported, so that userspace doesn't have to know about every
register in order to migrate (this could also be useful if there is
non-architecturally-readable state that needs to be preserved, such as a
pending book3e doorbell exception, or the MCAR to be set when machine
checks are unmasked).  It could also avoid the need for explicit
capabilities in some situations when new registers are added (like
KVM_CAP_PPC_HIOR).

> The only downpoint I see to this one is that it needs to pad to 1024 bits
> (hardware is already on 512 bit registers, so I wanted to leave some room)
> which is slightly too much for transmitting only 64 bits. But if that's all
> the tradeoff we have to do for getting an extensible interface, I'd say go
> for it nevertheless.

Does it really make sense to consider these large things as single
registers (even if that's how hw documents it)?  Do they need to be set
atomically?  How do you get/set them in hardware if GPRs aren't that large?

> +4.65 KVM_SET_ONE_REG
> +
> +Capability: KVM_CAP_ONE_REG
> +Architectures: all
> +Type: vcpu ioctl
> +Parameters: struct kvm_one_reg (in)
> +Returns: 0 on success, negative value on failure
> +
> +struct kvm_one_reg {
> +       __u64 id;
> +       union {
> +               __u8 reg8;
> +               __u16 reg16;
> +               __u32 reg32;
> +               __u64 reg64;
> +               __u8 reg128[16];
> +               __u8 reg256[32];
> +               __u8 reg512[64];
> +               __u8 reg1024[128];
> +       } u;
> +};

Do we need reg8/16/32, or can we simplify (and reduce potential size
mismatch errors) by using __u64 for everything that doesn't need to be
an array?

> +/* Available with KVM_CAP_ONE_REG */
> +
> +#define KVM_ONE_REG_GENERIC		0x0000000000000000ULL

Generic registers?  Is the idea to use this in place of dedicated ioctls
for certain KVM knobs?

> +/*
> + * Architecture specific registers are to be defined in arch headers and
> + * ORed with the arch identifier.
> + */
> +#define KVM_ONE_REG_PPC			0x1000000000000000ULL
> +#define KVM_ONE_REG_X86			0x2000000000000000ULL
> +#define KVM_ONE_REG_IA64		0x3000000000000000ULL
> +#define KVM_ONE_REG_ARM			0x4000000000000000ULL
> +#define KVM_ONE_REG_S390		0x5000000000000000ULL

Might want to allow space for more than 16 architectures -- there's room
to spare.

-Scott