RE: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Pavel Fedin]

 Hello!

> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 87a64e8..a667228 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
> >
> > BUG_ON(!p->is_write);
> >
> > -   val = *vcpu_reg(vcpu, p->Rt);
> > +   val = *p->val;
> 
> Why does it have to be a pointer? You could just have "val = p->val" if
> you carried the actual value instead of a pointer to the stack variable
> holding that value.

 There's only one concern for pointer approach. Actually, this refactor is 
based on my vGICv3 live migration API patch set:
http://www.spinics.net/lists/kvm/msg124205.html
http://www.spinics.net/lists/kvm/msg124202.html

 It's simply more convenient to use a pointer for exchange with userspace, see 
vgic_v3_cpu_regs_access() and callers. I wouldn't
like to refactor the code again. What's your opinion on this?
 And of course i'll fix up the rest.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


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

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

On 03/12/15 11:08, Pavel Fedin wrote:
> Hello!
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c
>>> b/arch/arm64/kvm/sys_regs.c index 87a64e8..a667228 100644 ---
>>> a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@
>>> -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu
>>> *vcpu,
>>> 
>>> BUG_ON(!p->is_write);
>>> 
>>> -   val = *vcpu_reg(vcpu, p->Rt); + val = *p->val;
>> 
>> Why does it have to be a pointer? You could just have "val =
>> p->val" if you carried the actual value instead of a pointer to the
>> stack variable holding that value.
> 
> There's only one concern for pointer approach. Actually, this
> refactor is based on my vGICv3 live migration API patch set: 
> http://www.spinics.net/lists/kvm/msg124205.html 
> http://www.spinics.net/lists/kvm/msg124202.html
> 
> It's simply more convenient to use a pointer for exchange with
> userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
> to refactor the code again. What's your opinion on this?

I still don't think this is a good idea. You can still store the value
as an integer in vgic_v3_cpu_regs_access(), and check the write property
to do the writeback on read. Which is the same thing I asked for in this
patch.

> And of course i'll fix up the rest.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Pavel Fedin]

 Hello!

> > It's simply more convenient to use a pointer for exchange with
> > userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
> > to refactor the code again. What's your opinion on this?
> 
> I still don't think this is a good idea. You can still store the value
> as an integer in vgic_v3_cpu_regs_access(), and check the write property
> to do the writeback on read. Which is the same thing I asked for in this
> patch.

 Started doing this and found one more (big) reason against. All sysreg 
handlers have 'const struct sys_reg_params' declaration, and
callers, and their callers... This 'const' is all around the code, and it would 
take a separate huge patch to un-const'ify all this.
Does it worth that?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


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

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

Pavel,

On 03/12/15 09:58, Pavel Fedin wrote:
> System register accesses also use zero register for Rt == 31, and
> therefore using it will also result in getting SP value instead. This
> patch makes them also using new accessors, introduced by the previous
> patch.
> 
> Additionally, got rid of "massive hack" in kvm_handle_cp_64().

Looks good for a first drop - some comments below.

> 
> Signed-off-by: Pavel Fedin 
> ---
>  arch/arm64/kvm/sys_regs.c| 79 
> 
>  arch/arm64/kvm/sys_regs.h|  4 +-
>  arch/arm64/kvm/sys_regs_generic_v8.c |  2 +-
>  3 files changed, 46 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87a64e8..a667228 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>  
>   BUG_ON(!p->is_write);
>  
> - val = *vcpu_reg(vcpu, p->Rt);
> + val = *p->val;

Why does it have to be a pointer? You could just have "val = p->val" if
you carried the actual value instead of a pointer to the stack variable
holding that value.

>   if (!p->is_aarch32) {
>   vcpu_sys_reg(vcpu, r->reg) = val;
>   } else {
> @@ -125,13 +125,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  const struct sys_reg_params *p,
>  const struct sys_reg_desc *r)
>  {
> - u64 val;
> -
>   if (!p->is_write)
>   return read_from_write_only(vcpu, p);
>  
> - val = *vcpu_reg(vcpu, p->Rt);
> - vgic_v3_dispatch_sgi(vcpu, val);
> + vgic_v3_dispatch_sgi(vcpu, *p->val);
>  
>   return true;
>  }
> @@ -153,7 +150,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>   if (p->is_write) {
>   return ignore_write(vcpu, p);
>   } else {
> - *vcpu_reg(vcpu, p->Rt) = (1 << 3);
> + *p->val = (1 << 3);
>   return true;
>   }
>  }
> @@ -167,7 +164,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
>   } else {
>   u32 val;
>   asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
> - *vcpu_reg(vcpu, p->Rt) = val;
> + *p->val = val;
>   return true;
>   }
>  }
> @@ -204,13 +201,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>   const struct sys_reg_desc *r)
>  {
>   if (p->is_write) {
> - vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> + vcpu_sys_reg(vcpu, r->reg) = *p->val;
>   vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>   } else {
> - *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
> + *p->val = vcpu_sys_reg(vcpu, r->reg);
>   }
>  
> - trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
> + trace_trap_reg(__func__, r->reg, p->is_write, *p->val);
>  
>   return true;
>  }
> @@ -228,7 +225,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
> const struct sys_reg_params *p,
> u64 *dbg_reg)
>  {
> - u64 val = *vcpu_reg(vcpu, p->Rt);
> + u64 val = *p->val;
>  
>   if (p->is_32bit) {
>   val &= 0xUL;
> @@ -248,7 +245,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
>   if (p->is_32bit)
>   val &= 0xUL;
>  
> - *vcpu_reg(vcpu, p->Rt) = val;
> + *p->val = val;
>  }
>  
>  static inline bool trap_bvr(struct kvm_vcpu *vcpu,
> @@ -697,10 +694,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>   u64 pfr = read_system_reg(SYS_ID_AA64PFR0_EL1);
>   u32 el3 = !!cpuid_feature_extract_field(pfr, 
> ID_AA64PFR0_EL3_SHIFT);
>  
> - *vcpu_reg(vcpu, p->Rt) = dfr >> ID_AA64DFR0_WRPS_SHIFT) & 
> 0xf) << 28) |
> -   (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 
> 0xf) << 24) |
> -   (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) 
> & 0xf) << 20) |
> -   (6 << 16) | (el3 << 14) | (el3 << 
> 12));
> + *p->val = dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
> +(((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
> +(((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20) |
> +(6 << 16) | (el3 << 14) | (el3 << 12));
>   return true;
>   }
>  }
> @@ -710,10 +707,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
>const struct sys_reg_desc *r)
>  {
>   if (p->is_write) {
> - vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> + vcpu_cp14(vcpu, r->reg) = *p->val;
>   vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>   } else {
> - *vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
> +   

Re: [PATCH 2/3] KVM: arm64: Correctly handle zero register in system register accesses

[Marc Zyngier]

On 03/12/15 11:55, Pavel Fedin wrote:
>  Hello!
> 
>>> It's simply more convenient to use a pointer for exchange with
>>> userspace, see vgic_v3_cpu_regs_access() and callers. I wouldn't like
>>> to refactor the code again. What's your opinion on this?
>>
>> I still don't think this is a good idea. You can still store the value
>> as an integer in vgic_v3_cpu_regs_access(), and check the write property
>> to do the writeback on read. Which is the same thing I asked for in this
>> patch.
> 
>  Started doing this and found one more (big) reason against. All sysreg 
> handlers have 'const struct sys_reg_params' declaration, and
> callers, and their callers... This 'const' is all around the code, and it 
> would take a separate huge patch to un-const'ify all this.
> Does it worth that?

maz@approximate:~/Work/arm-platforms$ git diff --stat
 arch/arm64/kvm/sys_regs.c | 36 ++--
 arch/arm64/kvm/sys_regs.h |  2 +-
 2 files changed, 19 insertions(+), 19 deletions(-)

Hardly a big deal... You can have that as a separate patch.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html