Re: [Qemu-devel] [PATCH 4/9] mips/kvm: Implement Config CP0 registers

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 7595 bytes --]

On 12/03/15 16:41, Leon Alrae wrote:
> On 11/03/2015 15:22, James Hogan wrote:
>> Implement saving and restoring to KVM state of the Config CP0 registers
>> (namely Config, Config1, Config2, Config3, Config4, and Config5). These
>> control the features available to a guest, and a few of the fields will
>> soon be writeable by a guest so QEMU needs to know about them so as not
>> to clobber them on migration/savevm.
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Leon Alrae <leon.alrae@imgtec.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  target-mips/kvm.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 108 insertions(+)
>>
>> diff --git a/target-mips/kvm.c b/target-mips/kvm.c
>> index 730c67e247d8..b8813a2722a3 100644
>> --- a/target-mips/kvm.c
>> +++ b/target-mips/kvm.c
>> @@ -223,6 +223,12 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level)
>>  #define KVM_REG_MIPS_CP0_CAUSE          MIPS_CP0_32(13, 0)
>>  #define KVM_REG_MIPS_CP0_EPC            MIPS_CP0_64(14, 0)
>>  #define KVM_REG_MIPS_CP0_PRID           MIPS_CP0_32(15, 0)
>> +#define KVM_REG_MIPS_CP0_CONFIG         MIPS_CP0_32(16, 0)
>> +#define KVM_REG_MIPS_CP0_CONFIG1        MIPS_CP0_32(16, 1)
>> +#define KVM_REG_MIPS_CP0_CONFIG2        MIPS_CP0_32(16, 2)
>> +#define KVM_REG_MIPS_CP0_CONFIG3        MIPS_CP0_32(16, 3)
>> +#define KVM_REG_MIPS_CP0_CONFIG4        MIPS_CP0_32(16, 4)
>> +#define KVM_REG_MIPS_CP0_CONFIG5        MIPS_CP0_32(16, 5)
>>  #define KVM_REG_MIPS_CP0_ERROREPC       MIPS_CP0_64(30, 0)
>>  
>>  static inline int kvm_mips_put_one_reg(CPUState *cs, uint64_t reg_id,
>> @@ -305,6 +311,34 @@ static inline int kvm_mips_get_one_reg64(CPUState *cs, uint64 reg_id,
>>      return kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &cp0reg);
>>  }
>>  
>> +#define KVM_REG_MIPS_CP0_CONFIG_MASK    (1 << CP0C0_M)
>> +#define KVM_REG_MIPS_CP0_CONFIG1_MASK   (1 << CP0C1_M)
>> +#define KVM_REG_MIPS_CP0_CONFIG2_MASK   (1 << CP0C2_M)
>> +#define KVM_REG_MIPS_CP0_CONFIG3_MASK   (1 << CP0C3_M)
>> +#define KVM_REG_MIPS_CP0_CONFIG4_MASK   (1 << CP0C4_M)
> 
> CP0Cx_M is 31, thus without "U" suffix 1 is left shifted into sign bit
> which is undefined behaviour.

Well spotted, I'll fix that.

> 
>> +#define KVM_REG_MIPS_CP0_CONFIG5_MASK   0
>> +
>> +static inline int kvm_mips_change_one_reg(CPUState *cs, uint64_t reg_id,
>> +                                          int32_t *addr, int32_t mask)
>> +{
>> +    int err;
>> +    int32_t tmp, change;
>> +
>> +    err = kvm_mips_get_one_reg(cs, reg_id, &tmp);
>> +    if (err < 0) {
>> +        return err;
>> +    }
>> +
>> +    /* only change bits in mask */
>> +    change = (*addr ^ tmp) & mask;
>> +    if (!change) {
>> +        return 0;
>> +    }
>> +
>> +    tmp = tmp ^ change;
>> +    return kvm_mips_put_one_reg(cs, reg_id, &tmp);
>> +}
>> +
>>  /*
>>   * We freeze the KVM timer when either the VM clock is stopped or the state is
>>   * saved (the state is dirty).
>> @@ -527,6 +561,48 @@ static int kvm_mips_put_cp0_registers(CPUState *cs, int level)
>>          DPRINTF("%s: Failed to put CP0_PRID (%d)\n", __func__, err);
>>          ret = err;
>>      }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG,
>> +                                  &env->CP0_Config0,
>> +                                  KVM_REG_MIPS_CP0_CONFIG_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG1,
>> +                                  &env->CP0_Config1,
>> +                                  KVM_REG_MIPS_CP0_CONFIG1_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG1 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG2,
>> +                                  &env->CP0_Config2,
>> +                                  KVM_REG_MIPS_CP0_CONFIG2_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG2 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG3,
>> +                                  &env->CP0_Config3,
>> +                                  KVM_REG_MIPS_CP0_CONFIG3_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG3 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG4,
>> +                                  &env->CP0_Config4,
>> +                                  KVM_REG_MIPS_CP0_CONFIG4_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG4 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_change_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG5,
>> +                                  &env->CP0_Config5,
>> +                                  KVM_REG_MIPS_CP0_CONFIG5_MASK);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to change CP0_CONFIG5 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>>      err = kvm_mips_put_one_ulreg(cs, KVM_REG_MIPS_CP0_ERROREPC,
>>                                   &env->CP0_ErrorEPC);
>>      if (err < 0) {
>> @@ -618,6 +694,38 @@ static int kvm_mips_get_cp0_registers(CPUState *cs)
>>          DPRINTF("%s: Failed to get CP0_PRID (%d)\n", __func__, err);
>>          ret = err;
>>      }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG, &env->CP0_Config0);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG1, &env->CP0_Config1);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG1 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG2, &env->CP0_Config2);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG2 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG3, &env->CP0_Config3);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG3 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG4,
>> +                               &env->CP0_Config4);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG4 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
>> +    err = kvm_mips_get_one_reg(cs, KVM_REG_MIPS_CP0_CONFIG5,
>> +                               &env->CP0_Config5);
>> +    if (err < 0) {
>> +        DPRINTF("%s: Failed to get CP0_CONFIG5 (%d)\n", __func__, err);
>> +        ret = err;
>> +    }
> 
> There's a minor style inconsistency here - for Config4 and Config5 the
> last argument has been moved to a new line whereas for Config{0,1,2,3}
> all arguments are in the same line.

Yes, Config4 and Config5 used to be unsigned prior to Maciej's patches,
so it used get_one_ureg, just enough to make it wrap. I'll fix.

Thanks for reviewing,

Cheers
James

> 
>>      err = kvm_mips_get_one_ulreg(cs, KVM_REG_MIPS_CP0_ERROREPC,
>>                                   &env->CP0_ErrorEPC);
>>      if (err < 0) {
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]