subject:"Re\: \[PATCH 5\/6\] arm\/arm64\: vgic\-new\: Implement VGICv3 CPU interface access"

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

2016-09-19 Thread Peter Maydell

On 19 September 2016 at 08:36, Vijay Kilari  wrote:
> In order to track the PRI and ID bits written by guest,
> VGIC needs to store these values when ICC_CTRL_EL1 is updated.
> However,  QEMU is reseting VGIC by writing 0's to all the
> registers after VGIC initialization and hence the back up values are
> always reset to 0 and hence when guest read back, VGIC returns wrong value.
>
> One option is to drop VGIC reset from QEMU which is not doing much.

No, if QEMU's reset is not resetting registers to the right value
the correct thing to do is fix the reset code. The way device
reset is supposed to work is that QEMU knows the right values
to use and it writes them to the kernel.

QEMU doesn't reset ICC_CTLR_EL1 to 0 for TCG:
cs->icc_ctlr_el1[GICV3_NS] = ICC_CTLR_EL1_A3V |
(1 << ICC_CTLR_EL1_IDBITS_SHIFT) |
(7 << ICC_CTLR_EL1_PRIBITS_SHIFT);

but I don't think that code gets run for the KVM VGIC.

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

2016-09-19 Thread Vijay Kilari

Hi Marc , Peter

On Sat, Sep 17, 2016 at 5:07 PM, Marc Zyngier  wrote:
> On Sat, 17 Sep 2016 11:58:48 +0530
> Vijay Kilari  wrote:
>
>> On Fri, Sep 16, 2016 at 10:37 PM, Marc Zyngier  wrote:
>> > On 16/09/16 17:57, Vijay Kilari wrote:
>> >> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier  
>> >> wrote:
>> >>> On 16/09/16 13:20, vijay.kil...@gmail.com wrote:
>>  From: Vijaya Kumar K 
>> 
>>  VGICv3 CPU interface registers are accessed using
>>  KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>>  as 64-bit. The cpu MPIDR value is passed along with register id.
>>  is used to identify the cpu for registers access.
>> 
>>  The version of VGIC v3 specification is define here
>>  http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> 
>>  Signed-off-by: Pavel Fedin 
>>  Signed-off-by: Vijaya Kumar K 
>>  ---
>>   arch/arm64/include/uapi/asm/kvm.h   |   3 +
>>   arch/arm64/kvm/Makefile |   1 +
>>   include/linux/irqchip/arm-gic-v3.h  |  30 
>>   virt/kvm/arm/vgic/vgic-kvm-device.c |  27 
>>   virt/kvm/arm/vgic/vgic-mmio-v3.c|  18 +++
>>   virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 
>>  
>>   virt/kvm/arm/vgic/vgic.h|  10 ++
>>   7 files changed, 385 insertions(+)
>> >
>> > [...]
>> >
>>  diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c 
>>  b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>>  new file mode 100644
>>  index 000..8e4f403
>>  --- /dev/null
>>  +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>>  @@ -0,0 +1,296 @@
>>  +#include 
>>  +#include 
>>  +#include 
>>  +#include 
>>  +#include 
>>  +#include 
>>  +#include 
>>  +#include 
>>  +
>>  +#include "vgic.h"
>>  +#include "vgic-mmio.h"
>>  +#include "sys_regs.h"
>>  +
>>  +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct 
>>  sys_reg_params *p,
>>  + const struct sys_reg_desc *r)
>>  +{
>>  + struct vgic_vmcr vmcr;
>>  + u64 val;
>>  + u32 ich_vtr;
>>  +
>>  + vgic_get_vmcr(vcpu, );
>>  + if (p->is_write) {
>>  + val = p->regval;
>>  + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>>  +   ICC_CTLR_EL1_CBPR_SHIFT) << 
>>  ICH_VMCR_CBPR_SHIFT;
>>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>>  +  ICC_CTLR_EL1_EOImode_SHIFT) << 
>>  ICH_VMCR_EOIM_SHIFT;
>>  + vgic_set_vmcr(vcpu, );
>> >>>
>> >>> You've ignored my comments again: "What if userspace writes something
>> >>> that is incompatible with the current configuration? Wrong number of ID
>> >>> bits, or number of priorities?"
>> >>
>> >> IMO, In case of incompatibility,
>> >> If ID bits and PRI bits are less than HW supported, it is ok.
>> >
>> > Yes. But you also need to track of what the guest has programmed in
>> > order to be able to migrate it back to its original configuration.
>>
>> You mean the vgic has to track/store the ID and PRI bits that guest
>> has programmed
>> and return the same when guest reads back instead of
>> returning HW supported value for ICC_CTLR_EL1 reg access?.
>
> If you have two hosts (A and B), A having 5 bits of priority and B
> having 7 bits, you should be able to migrate from A to B, and then from
> B to A. Which means you have to preserve what the guest knows to be its
> configuration, even if you run on a more capable system. Otherwise,
> you're a bit stuck.
>
> You probably won't be able to hide the discrepancy from inside the
> guest though (the guest will be able to observe the change), but this
> is better than nothing.

In order to track the PRI and ID bits written by guest,
VGIC needs to store these values when ICC_CTRL_EL1 is updated.
However,  QEMU is reseting VGIC by writing 0's to all the
registers after VGIC initialization and hence the back up values are
always reset to 0 and hence when guest read back, VGIC returns wrong value.

One option is to drop VGIC reset from QEMU which is not doing much.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

2016-09-18 Thread Marc Zyngier

On Sun, 18 Sep 2016 12:00:01 +0530
Vijay Kilari  wrote:

> Hi Marc,
> 
> On Fri, Sep 16, 2016 at 10:37 PM, Marc Zyngier  wrote:
> > On 16/09/16 17:57, Vijay Kilari wrote:  
> >> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier  
> >> wrote:  
> >>> On 16/09/16 13:20, vijay.kil...@gmail.com wrote:  
>  From: Vijaya Kumar K 
> 
>  VGICv3 CPU interface registers are accessed using
>  KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>  as 64-bit. The cpu MPIDR value is passed along with register id.
>  is used to identify the cpu for registers access.
> 
>  The version of VGIC v3 specification is define here
>  http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
>  Signed-off-by: Pavel Fedin 
>  Signed-off-by: Vijaya Kumar K 
>  ---
>   arch/arm64/include/uapi/asm/kvm.h   |   3 +
>   arch/arm64/kvm/Makefile |   1 +
>   include/linux/irqchip/arm-gic-v3.h  |  30 
>   virt/kvm/arm/vgic/vgic-kvm-device.c |  27 
>   virt/kvm/arm/vgic/vgic-mmio-v3.c|  18 +++
>   virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 
>  
>   virt/kvm/arm/vgic/vgic.h|  10 ++
>   7 files changed, 385 insertions(+)  
> >
> > [...]
> >  
>  diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c 
>  b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>  new file mode 100644
>  index 000..8e4f403
>  --- /dev/null
>  +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>  @@ -0,0 +1,296 @@
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +
>  +#include "vgic.h"
>  +#include "vgic-mmio.h"
>  +#include "sys_regs.h"
>  +
>  +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct 
>  sys_reg_params *p,
>  + const struct sys_reg_desc *r)
>  +{
>  + struct vgic_vmcr vmcr;
>  + u64 val;
>  + u32 ich_vtr;
>  +
>  + vgic_get_vmcr(vcpu, );
>  + if (p->is_write) {
>  + val = p->regval;
>  + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>  +   ICC_CTLR_EL1_CBPR_SHIFT) << 
>  ICH_VMCR_CBPR_SHIFT;
>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>  +  ICC_CTLR_EL1_EOImode_SHIFT) << 
>  ICH_VMCR_EOIM_SHIFT;
>  + vgic_set_vmcr(vcpu, );  
> >>>
> >>> You've ignored my comments again: "What if userspace writes something
> >>> that is incompatible with the current configuration? Wrong number of ID
> >>> bits, or number of priorities?"  
> >>
> >> IMO, In case of incompatibility,
> >> If ID bits and PRI bits are less than HW supported, it is ok.  
> >
> > Yes. But you also need to track of what the guest has programmed in
> > order to be able to migrate it back to its original configuration.
> >  
> >> If ID bits and PRI bits are greater than HW supported, then warn would be 
> >> good
> >> enough. Please suggest the behaviour that you think it should be.  
> >
> > No, it is an error, plain and simple. You cannot run in this condition.
> >  
> >>  
> >>>  
>  + } else {
>  + ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>  +
>  + val = 0;
>  + val |= ((ich_vtr & ICH_VTR_PRI_BITS_MASK) >>
>  + ICH_VTR_PRI_BITS_SHIFT) << 
>  ICC_CTLR_EL1_PRI_BITS_SHIFT;
>  + val |= ((ich_vtr & ICH_VTR_ID_BITS_MASK) >>
>  + ICH_VTR_ID_BITS_SHIFT) << 
>  ICC_CTLR_EL1_ID_BITS_SHIFT;
>  + val |= ((ich_vtr & ICH_VTR_SEIS_MASK) >> 
>  ICH_VTR_SEIS_SHIFT)
>  + << ICC_CTLR_EL1_SEIS_SHIFT;
>  + val |= ((ich_vtr & ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT)
>  + << ICC_CTLR_EL1_A3V_SHIFT;
>  + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
>  + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
>  + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
>  + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
>  +
>  + p->regval = val;
>  + }
>  +
>  + return true;
>  +}
>  +
>  +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params 
>  *p,
>  +const struct sys_reg_desc *r)
>  +{
>  + struct vgic_vmcr vmcr;
>  +
>  + vgic_get_vmcr(vcpu, );
>  + if (p->is_write) {
>  + vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> 
>

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

2016-09-18 Thread Vijay Kilari

Hi Marc,

On Fri, Sep 16, 2016 at 10:37 PM, Marc Zyngier  wrote:
> On 16/09/16 17:57, Vijay Kilari wrote:
>> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier  wrote:
>>> On 16/09/16 13:20, vijay.kil...@gmail.com wrote:
 From: Vijaya Kumar K 

 VGICv3 CPU interface registers are accessed using
 KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
 as 64-bit. The cpu MPIDR value is passed along with register id.
 is used to identify the cpu for registers access.

 The version of VGIC v3 specification is define here
 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

 Signed-off-by: Pavel Fedin 
 Signed-off-by: Vijaya Kumar K 
 ---
  arch/arm64/include/uapi/asm/kvm.h   |   3 +
  arch/arm64/kvm/Makefile |   1 +
  include/linux/irqchip/arm-gic-v3.h  |  30 
  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 
  virt/kvm/arm/vgic/vgic-mmio-v3.c|  18 +++
  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 
 
  virt/kvm/arm/vgic/vgic.h|  10 ++
  7 files changed, 385 insertions(+)
>
> [...]
>
 diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c 
 b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
 new file mode 100644
 index 000..8e4f403
 --- /dev/null
 +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
 @@ -0,0 +1,296 @@
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#include "vgic.h"
 +#include "vgic-mmio.h"
 +#include "sys_regs.h"
 +
 +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params 
 *p,
 + const struct sys_reg_desc *r)
 +{
 + struct vgic_vmcr vmcr;
 + u64 val;
 + u32 ich_vtr;
 +
 + vgic_get_vmcr(vcpu, );
 + if (p->is_write) {
 + val = p->regval;
 + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
 + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
 +   ICC_CTLR_EL1_CBPR_SHIFT) << 
 ICH_VMCR_CBPR_SHIFT;
 + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
 +  ICC_CTLR_EL1_EOImode_SHIFT) << 
 ICH_VMCR_EOIM_SHIFT;
 + vgic_set_vmcr(vcpu, );
>>>
>>> You've ignored my comments again: "What if userspace writes something
>>> that is incompatible with the current configuration? Wrong number of ID
>>> bits, or number of priorities?"
>>
>> IMO, In case of incompatibility,
>> If ID bits and PRI bits are less than HW supported, it is ok.
>
> Yes. But you also need to track of what the guest has programmed in
> order to be able to migrate it back to its original configuration.
>
>> If ID bits and PRI bits are greater than HW supported, then warn would be 
>> good
>> enough. Please suggest the behaviour that you think it should be.
>
> No, it is an error, plain and simple. You cannot run in this condition.
>
>>
>>>
 + } else {
 + ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
 +
 + val = 0;
 + val |= ((ich_vtr & ICH_VTR_PRI_BITS_MASK) >>
 + ICH_VTR_PRI_BITS_SHIFT) << 
 ICC_CTLR_EL1_PRI_BITS_SHIFT;
 + val |= ((ich_vtr & ICH_VTR_ID_BITS_MASK) >>
 + ICH_VTR_ID_BITS_SHIFT) << ICC_CTLR_EL1_ID_BITS_SHIFT;
 + val |= ((ich_vtr & ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT)
 + << ICC_CTLR_EL1_SEIS_SHIFT;
 + val |= ((ich_vtr & ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT)
 + << ICC_CTLR_EL1_A3V_SHIFT;
 + val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
 + ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
 + val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
 + ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
 +
 + p->regval = val;
 + }
 +
 + return true;
 +}
 +
 +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params 
 *p,
 +const struct sys_reg_desc *r)
 +{
 + struct vgic_vmcr vmcr;
 +
 + vgic_get_vmcr(vcpu, );
 + if (p->is_write) {
 + vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> 
 ICC_PMR_EL1_SHIFT;
 + vgic_set_vmcr(vcpu, );
 + } else {
 + p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & 
 ICC_PMR_EL1_MASK;
 + }
 +
 + return true;
 +}
 +
 +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params 
 *p,
 + const struct sys_reg_desc *r)
 +{

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

2016-09-17 Thread Marc Zyngier

On Sat, 17 Sep 2016 11:58:48 +0530
Vijay Kilari  wrote:

> On Fri, Sep 16, 2016 at 10:37 PM, Marc Zyngier  wrote:
> > On 16/09/16 17:57, Vijay Kilari wrote:  
> >> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier  
> >> wrote:  
> >>> On 16/09/16 13:20, vijay.kil...@gmail.com wrote:  
>  From: Vijaya Kumar K 
> 
>  VGICv3 CPU interface registers are accessed using
>  KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>  as 64-bit. The cpu MPIDR value is passed along with register id.
>  is used to identify the cpu for registers access.
> 
>  The version of VGIC v3 specification is define here
>  http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
>  Signed-off-by: Pavel Fedin 
>  Signed-off-by: Vijaya Kumar K 
>  ---
>   arch/arm64/include/uapi/asm/kvm.h   |   3 +
>   arch/arm64/kvm/Makefile |   1 +
>   include/linux/irqchip/arm-gic-v3.h  |  30 
>   virt/kvm/arm/vgic/vgic-kvm-device.c |  27 
>   virt/kvm/arm/vgic/vgic-mmio-v3.c|  18 +++
>   virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 
>  
>   virt/kvm/arm/vgic/vgic.h|  10 ++
>   7 files changed, 385 insertions(+)  
> >
> > [...]
> >  
>  diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c 
>  b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>  new file mode 100644
>  index 000..8e4f403
>  --- /dev/null
>  +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
>  @@ -0,0 +1,296 @@
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +
>  +#include "vgic.h"
>  +#include "vgic-mmio.h"
>  +#include "sys_regs.h"
>  +
>  +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct 
>  sys_reg_params *p,
>  + const struct sys_reg_desc *r)
>  +{
>  + struct vgic_vmcr vmcr;
>  + u64 val;
>  + u32 ich_vtr;
>  +
>  + vgic_get_vmcr(vcpu, );
>  + if (p->is_write) {
>  + val = p->regval;
>  + vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
>  +   ICC_CTLR_EL1_CBPR_SHIFT) << 
>  ICH_VMCR_CBPR_SHIFT;
>  + vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
>  +  ICC_CTLR_EL1_EOImode_SHIFT) << 
>  ICH_VMCR_EOIM_SHIFT;
>  + vgic_set_vmcr(vcpu, );  
> >>>
> >>> You've ignored my comments again: "What if userspace writes something
> >>> that is incompatible with the current configuration? Wrong number of ID
> >>> bits, or number of priorities?"  
> >>
> >> IMO, In case of incompatibility,
> >> If ID bits and PRI bits are less than HW supported, it is ok.  
> >
> > Yes. But you also need to track of what the guest has programmed in
> > order to be able to migrate it back to its original configuration.  
> 
> You mean the vgic has to track/store the ID and PRI bits that guest
> has programmed
> and return the same when guest reads back instead of
> returning HW supported value for ICC_CTLR_EL1 reg access?.

If you have two hosts (A and B), A having 5 bits of priority and B
having 7 bits, you should be able to migrate from A to B, and then from
B to A. Which means you have to preserve what the guest knows to be its
configuration, even if you run on a more capable system. Otherwise,
you're a bit stuck.

You probably won't be able to hide the discrepancy from inside the
guest though (the guest will be able to observe the change), but this
is better than nothing.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

Re: [PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

5 matches

Site Navigation

Mail list logo

Footer information