| Message ID | 1480576187-5012-8-git-send-email-vijay.kilari@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Vijaya, On 01/12/2016 08:09, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > Userspace requires to store and restore of line_level for > level triggered interrupts using ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > --- > arch/arm/include/uapi/asm/kvm.h | 7 ++++++ > arch/arm64/include/uapi/asm/kvm.h | 6 +++++ > virt/kvm/arm/vgic/vgic-kvm-device.c | 45 +++++++++++++++++++++++++++++++++++- > virt/kvm/arm/vgic/vgic-mmio-v3.c | 11 +++++++++ > virt/kvm/arm/vgic/vgic-mmio.c | 46 +++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic-mmio.h | 5 ++++ > virt/kvm/arm/vgic/vgic.h | 2 ++ > 7 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h > index 98658d9d..f347779 100644 > --- a/arch/arm/include/uapi/asm/kvm.h > +++ b/arch/arm/include/uapi/asm/kvm.h > @@ -191,6 +191,13 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 > #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 > #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 > +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ > + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 > + > #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > > /* KVM_IRQ_LINE irq field index values */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 91c7137..4100f8c 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -211,6 +211,12 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 > #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 > #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 > +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ > + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > index b6266fe..d5f7197 100644 > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > @@ -510,6 +510,21 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev, > regid, reg); > break; > } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + unsigned int info, intid; > + > + info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT; > + if (info == VGIC_LEVEL_INFO_LINE_LEVEL) { > + intid = attr->attr & > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK; > + ret = vgic_v3_line_level_info_uaccess(vcpu, is_write, > + intid, reg); > + } else { > + ret = -EINVAL; > + } > + break; > + } > default: > ret = -EINVAL; > break; > @@ -552,6 +567,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev, > > return vgic_v3_attr_regs_access(dev, attr, ®, true); > } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + u32 __user *uaddr = (u32 __user *)(long)attr->addr; > + u64 reg; > + u32 tmp32; > + > + if (get_user(tmp32, uaddr)) > + return -EFAULT; > + > + reg = tmp32; > + return vgic_v3_attr_regs_access(dev, attr, ®, true); > + } > } > return -ENXIO; > } > @@ -587,8 +613,18 @@ static int vgic_v3_get_attr(struct kvm_device *dev, > return ret; > return put_user(reg, uaddr); > } > - } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + u32 __user *uaddr = (u32 __user *)(long)attr->addr; > + u64 reg; > + u32 tmp32; > > + ret = vgic_v3_attr_regs_access(dev, attr, ®, false); > + if (ret) > + return ret; > + tmp32 = reg; > + return put_user(tmp32, uaddr); > + } > + } > return -ENXIO; > } > > @@ -609,6 +645,13 @@ static int vgic_v3_has_attr(struct kvm_device *dev, > return vgic_v3_has_attr_regs(dev, attr); > case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: > return 0; > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) == > + VGIC_LEVEL_INFO_LINE_LEVEL) > + return 0; > + break; > + } > case KVM_DEV_ARM_VGIC_GRP_CTRL: > switch (attr->attr) { > case KVM_DEV_ARM_VGIC_CTRL_INIT: > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 51439c9..9491d72 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -809,3 +809,14 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > return vgic_uaccess(vcpu, &rd_dev, is_write, > offset, val); > } > + > +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + u32 intid, u64 *val) > +{ I don't see anyone checking vINTID is multiple of 32 as emphasized in Documentation/virtual/kvm/devices/arm-vgic-v3.txt > + if (is_write) > + vgic_write_irq_line_level_info(vcpu, intid, *val); > + else > + *val = vgic_read_irq_line_level_info(vcpu, intid); > + > + return 0; > +} > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > index f81e0e5..28fef92b 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.c > +++ b/virt/kvm/arm/vgic/vgic-mmio.c > @@ -371,6 +371,52 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu, > } > } > > +u64 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid) > +{ > + int i; > + u64 val = 0; > + > + for (i = 0; i < 32; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + if (irq->line_level) > + val |= (1U << i); what about SGIs and higher ID than the number of interrupts supported. Spec says RAZ/WI. > + > + vgic_put_irq(vcpu->kvm, irq); > + } > + > + return val; > +} > + > +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, > + const u64 val) > +{ > + int i; > + > + for (i = 0; i < 32; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + same as above. > + spin_lock(&irq->irq_lock); > + if (val & (1U << i)) { > + if (irq->config == VGIC_CONFIG_LEVEL) { > + irq->line_level = true; > + irq->pending = true; > + vgic_queue_irq_unlock(vcpu->kvm, irq); > + } else { > + spin_unlock(&irq->irq_lock); > + } > + } else { > + if (irq->config == VGIC_CONFIG_EDGE || can't we just ignore VGIC_CONFIG_EDGE case for which line_level is not modeled? > + (irq->config == VGIC_CONFIG_LEVEL && > + !irq->soft_pending)) > + irq->line_level = false; To me the line level does not depend on the soft_pending bit. The pending state depends on both. Thanks Eric > + spin_unlock(&irq->irq_lock); > + } > + > + vgic_put_irq(vcpu->kvm, irq); > + } > +} > + > static int match_region(const void *key, const void *elt) > { > const unsigned int offset = (unsigned long)key; > diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h > index 1cc7faf..b9423b4 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.h > +++ b/virt/kvm/arm/vgic/vgic-mmio.h > @@ -181,6 +181,11 @@ int vgic_validate_mmio_region_addr(struct kvm_device *dev, > const struct vgic_register_region *regions, > int nr_regions, gpa_t addr); > > +u64 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid); > + > +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, > + const u64 val); > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); > > unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev); > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index eac272c..60d4350 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -144,6 +144,8 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, > u64 id, u64 *val); > int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id, > u64 *reg); > +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + u32 intid, u64 *val); > int kvm_register_vgic_device(unsigned long type); > void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); > void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 16 December 2016 at 12:07, Auger Eric <eric.auger@redhat.com> wrote: > Hi Vijaya, > > On 01/12/2016 08:09, vijay.kilari@gmail.com wrote: >> +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, >> + const u64 val) >> +{ >> + int i; >> + >> + for (i = 0; i < 32; i++) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> + > same as above. >> + spin_lock(&irq->irq_lock); >> + if (val & (1U << i)) { >> + if (irq->config == VGIC_CONFIG_LEVEL) { >> + irq->line_level = true; >> + irq->pending = true; >> + vgic_queue_irq_unlock(vcpu->kvm, irq); >> + } else { >> + spin_unlock(&irq->irq_lock); >> + } >> + } else { >> + if (irq->config == VGIC_CONFIG_EDGE || > can't we just ignore VGIC_CONFIG_EDGE case for which line_level is not > modeled? >> + (irq->config == VGIC_CONFIG_LEVEL && >> + !irq->soft_pending)) >> + irq->line_level = false; > To me the line level does not depend on the soft_pending bit. The > pending state depends on both. Without having looked at the details, it seems surprising to me that the implementation of "set line level to X" is not "set irq->line_level to X; figure out consequences and update other state as necessary"... thanks -- PMM _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Dec 01, 2016 at 12:39:46PM +0530, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > Userspace requires to store and restore of line_level for > level triggered interrupts using ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > --- > arch/arm/include/uapi/asm/kvm.h | 7 ++++++ > arch/arm64/include/uapi/asm/kvm.h | 6 +++++ > virt/kvm/arm/vgic/vgic-kvm-device.c | 45 +++++++++++++++++++++++++++++++++++- > virt/kvm/arm/vgic/vgic-mmio-v3.c | 11 +++++++++ > virt/kvm/arm/vgic/vgic-mmio.c | 46 +++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic-mmio.h | 5 ++++ > virt/kvm/arm/vgic/vgic.h | 2 ++ > 7 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h > index 98658d9d..f347779 100644 > --- a/arch/arm/include/uapi/asm/kvm.h > +++ b/arch/arm/include/uapi/asm/kvm.h > @@ -191,6 +191,13 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 > #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 > #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 > +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ > + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 > + > #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > > /* KVM_IRQ_LINE irq field index values */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 91c7137..4100f8c 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -211,6 +211,12 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 > #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 > #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 > +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ > + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > index b6266fe..d5f7197 100644 > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > @@ -510,6 +510,21 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev, > regid, reg); > break; > } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + unsigned int info, intid; > + > + info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT; > + if (info == VGIC_LEVEL_INFO_LINE_LEVEL) { > + intid = attr->attr & > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK; > + ret = vgic_v3_line_level_info_uaccess(vcpu, is_write, > + intid, reg); > + } else { > + ret = -EINVAL; > + } > + break; > + } > default: > ret = -EINVAL; > break; > @@ -552,6 +567,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev, > > return vgic_v3_attr_regs_access(dev, attr, ®, true); > } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + u32 __user *uaddr = (u32 __user *)(long)attr->addr; > + u64 reg; > + u32 tmp32; > + > + if (get_user(tmp32, uaddr)) > + return -EFAULT; > + > + reg = tmp32; > + return vgic_v3_attr_regs_access(dev, attr, ®, true); > + } > } > return -ENXIO; > } > @@ -587,8 +613,18 @@ static int vgic_v3_get_attr(struct kvm_device *dev, > return ret; > return put_user(reg, uaddr); > } > - } > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + u32 __user *uaddr = (u32 __user *)(long)attr->addr; > + u64 reg; > + u32 tmp32; > > + ret = vgic_v3_attr_regs_access(dev, attr, ®, false); > + if (ret) > + return ret; > + tmp32 = reg; > + return put_user(tmp32, uaddr); > + } > + } > return -ENXIO; > } > > @@ -609,6 +645,13 @@ static int vgic_v3_has_attr(struct kvm_device *dev, > return vgic_v3_has_attr_regs(dev, attr); > case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: > return 0; > + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { > + if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> > + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) == > + VGIC_LEVEL_INFO_LINE_LEVEL) > + return 0; > + break; > + } > case KVM_DEV_ARM_VGIC_GRP_CTRL: > switch (attr->attr) { > case KVM_DEV_ARM_VGIC_CTRL_INIT: > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 51439c9..9491d72 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -809,3 +809,14 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > return vgic_uaccess(vcpu, &rd_dev, is_write, > offset, val); > } > + > +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + u32 intid, u64 *val) > +{ > + if (is_write) > + vgic_write_irq_line_level_info(vcpu, intid, *val); > + else > + *val = vgic_read_irq_line_level_info(vcpu, intid); > + > + return 0; > +} > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > index f81e0e5..28fef92b 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.c > +++ b/virt/kvm/arm/vgic/vgic-mmio.c > @@ -371,6 +371,52 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu, > } > } > > +u64 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid) > +{ > + int i; > + u64 val = 0; > + > + for (i = 0; i < 32; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + if (irq->line_level) > + val |= (1U << i); > + > + vgic_put_irq(vcpu->kvm, irq); > + } > + > + return val; > +} > + > +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, > + const u64 val) > +{ > + int i; > + > + for (i = 0; i < 32; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + if (val & (1U << i)) { > + if (irq->config == VGIC_CONFIG_LEVEL) { > + irq->line_level = true; > + irq->pending = true; > + vgic_queue_irq_unlock(vcpu->kvm, irq); > + } else { > + spin_unlock(&irq->irq_lock); > + } > + } else { > + if (irq->config == VGIC_CONFIG_EDGE || > + (irq->config == VGIC_CONFIG_LEVEL && > + !irq->soft_pending)) > + irq->line_level = false; > + spin_unlock(&irq->irq_lock); > + } So last time we had a discussion about whether or not the API should support any random order of restoring the registers, but I cannot see how we can support that, because how can we tell the difference between the following two scenarios without knowing if an interrupt is edge-triggered or level triggered: (1) Clearing the line_level for an edge-triggered interrupt after having set it to pending, which means it should stay pending. (2) Clearing the line_level for a level-triggered interrupt when the state is already pending for some reason, but the soft_pending (latch) state is not set, in which case the pending state should be removed. This leads me to conclude that userspace must restore the configuration state of the interrupt before at least the line_level, in which case the implementation becomes: for (i = 0; i < 32; i++) { struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); bool new_level = !!(val & (1U << i)); if (irq->config == VGIC_CONFIG_EDGE) continue; spin_lock(&irq->irq_lock); irq->line_level = new_level; if (new_level) { irq->pending = true; vgic_queue_irq_unlock(vcpu->kvm, irq); } else { if (!irq->soft_pending) irq->pending = false; spin_unlock(&irq->irq_lock); } vgic_put_irq(vcpu->kvm, irq); } Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Dec 16, 2016 at 12:44:18PM +0000, Peter Maydell wrote: > On 16 December 2016 at 12:07, Auger Eric <eric.auger@redhat.com> wrote: > > Hi Vijaya, > > > > On 01/12/2016 08:09, vijay.kilari@gmail.com wrote: > >> +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, > >> + const u64 val) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < 32; i++) { > >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > >> + > > same as above. > >> + spin_lock(&irq->irq_lock); > >> + if (val & (1U << i)) { > >> + if (irq->config == VGIC_CONFIG_LEVEL) { > >> + irq->line_level = true; > >> + irq->pending = true; > >> + vgic_queue_irq_unlock(vcpu->kvm, irq); > >> + } else { > >> + spin_unlock(&irq->irq_lock); > >> + } > >> + } else { > >> + if (irq->config == VGIC_CONFIG_EDGE || > > can't we just ignore VGIC_CONFIG_EDGE case for which line_level is not > > modeled? > >> + (irq->config == VGIC_CONFIG_LEVEL && > >> + !irq->soft_pending)) > >> + irq->line_level = false; > > To me the line level does not depend on the soft_pending bit. The > > pending state depends on both. > > Without having looked at the details, it seems surprising to > me that the implementation of "set line level to X" is not > "set irq->line_level to X; figure out consequences and update > other state as necessary"... > Indeed. Assuming you're ok with the API requiring config state to be restored before the line level state, I think my suggestion in the separate reply should work. Of course, we need to clarify the ordering requirement in the API. Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 20 January 2017 at 19:53, Christoffer Dall <christoffer.dall@linaro.org> wrote: > So last time we had a discussion about whether or not the API should > support any random order of restoring the registers, but I cannot see > how we can support that, because how can we tell the difference between > the following two scenarios without knowing if an interrupt is > edge-triggered or level triggered: > > (1) Clearing the line_level for an edge-triggered interrupt after > having set it to pending, which means it should stay pending. This is userspace doing: * set pending-latch to 1 * set linelevel to 0 right? The pending state is pending-latch | (linelevel & ~edge_trigger), and you can recalculate that when userspace updates either of the pending-latch state or linelevel. (It will be temporarily wrong before userspace has told the kernel about all the state, but that's fine because we won't try to run the VM until we've finished loading everything.) > (2) Clearing the line_level for a level-triggered interrupt when the > state is already pending for some reason, but the soft_pending > (latch) state is not set, in which case the pending state should > be removed. This is userspace doing * set pending-latch to 0 * set line_level to 0 and thus the pending state goes to 0 (same calculation as above). pending is always a pure logical function of pending-latch, linelevel and edge-trigger bits, so as long as you recalculate it at the right time then it shouldn't matter which order userspace tells you about the three things in. thanks -- PMM _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Jan 23, 2017 at 10:16:04AM +0000, Peter Maydell wrote: > On 20 January 2017 at 19:53, Christoffer Dall > <christoffer.dall@linaro.org> wrote: > > So last time we had a discussion about whether or not the API should > > support any random order of restoring the registers, but I cannot see > > how we can support that, because how can we tell the difference between > > the following two scenarios without knowing if an interrupt is > > edge-triggered or level triggered: > > > > (1) Clearing the line_level for an edge-triggered interrupt after > > having set it to pending, which means it should stay pending. > > This is userspace doing: > * set pending-latch to 1 > * set linelevel to 0 > > right? The pending state is pending-latch | (linelevel & ~edge_trigger), > and you can recalculate that when userspace updates either of the > pending-latch state or linelevel. (It will be temporarily wrong > before userspace has told the kernel about all the state, but > that's fine because we won't try to run the VM until we've finished > loading everything.) > > > (2) Clearing the line_level for a level-triggered interrupt when the > > state is already pending for some reason, but the soft_pending > > (latch) state is not set, in which case the pending state should > > be removed. > > This is userspace doing > * set pending-latch to 0 > * set line_level to 0 > > and thus the pending state goes to 0 (same calculation as above). > > pending is always a pure logical function of pending-latch, > linelevel and edge-trigger bits, so as long as you recalculate > it at the right time then it shouldn't matter which order userspace > tells you about the three things in. > ok, I think you're right that it can be done this way, but it has the unfortunate consequence of having to change a lot of the implementation. The reason is that we store the latch state in two different variables, depening on whether it's an edge- or level-triggered interrupts. We use the irq->pending field for the computed result (using above calculaiton) for level interrupts based on irq->line_level and irq->soft_pending. soft_pending is the latch state for level interrupts only. For edge triggered interrupts the computed result and the latch state are alway the same (right?) so we we only use the irq->pending field for that. But unless I didn't have enough coffee this morning, this only works if you have a-priori knowledge of which interrupts are level and which are edge. When this is not the case, as in the case of order-free save/restore, then I think the only solution is to rework the logic so that the soft_pending field is used for the latch state for both edge and level interrupts, and the pending field is just the internal computed value. Thoughts? Andre, Marc, Eric, do you agree with the above? (I wonder how we could go through an entire redesign of the vgic and still have issues like this, but I do remember we all felt tired when we finally got to design the soft_pending stuff.) Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 23 January 2017 at 11:06, Christoffer Dall <christoffer.dall@linaro.org> wrote: > ok, I think you're right that it can be done this way, but it has the > unfortunate consequence of having to change a lot of the implementation. > > The reason is that we store the latch state in two different variables, > depening on whether it's an edge- or level-triggered interrupts. > > We use the irq->pending field for the computed result (using above > calculaiton) for level interrupts based on irq->line_level and > irq->soft_pending. soft_pending is the latch state for level interrupts > only. > > For edge triggered interrupts the computed result and the latch state > are alway the same (right?) so we we only use the irq->pending field for > that. > > But unless I didn't have enough coffee this morning, this only works if > you have a-priori knowledge of which interrupts are level and which are > edge. When this is not the case, as in the case of order-free > save/restore, then I think the only solution is to rework the logic so > that the soft_pending field is used for the latch state for both edge > and level interrupts, and the pending field is just the internal > computed value. I *think* you could fudge it by saying "when the config changes from edge to level, copy the current irq->pending into irq->soft_pending". Then you can always read the latch state (it's soft_pending if level, otherwise pending), and writing it is "set both if level, just irq->pending if edge". That in turn gives you enough information I think to cope with restores of all of (config, level, pending-latch) in any order. It feels a bit fragile, though. thanks -- PMM _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Jan 23, 2017 at 11:41:41AM +0000, Peter Maydell wrote: > On 23 January 2017 at 11:06, Christoffer Dall > <christoffer.dall@linaro.org> wrote: > > ok, I think you're right that it can be done this way, but it has the > > unfortunate consequence of having to change a lot of the implementation. > > > > The reason is that we store the latch state in two different variables, > > depening on whether it's an edge- or level-triggered interrupts. > > > > We use the irq->pending field for the computed result (using above > > calculaiton) for level interrupts based on irq->line_level and > > irq->soft_pending. soft_pending is the latch state for level interrupts > > only. > > > > For edge triggered interrupts the computed result and the latch state > > are alway the same (right?) so we we only use the irq->pending field for > > that. > > > > But unless I didn't have enough coffee this morning, this only works if > > you have a-priori knowledge of which interrupts are level and which are > > edge. When this is not the case, as in the case of order-free > > save/restore, then I think the only solution is to rework the logic so > > that the soft_pending field is used for the latch state for both edge > > and level interrupts, and the pending field is just the internal > > computed value. > > I *think* you could fudge it by saying "when the config changes > from edge to level, copy the current irq->pending into irq->soft_pending". > Then you can always read the latch state (it's soft_pending > if level, otherwise pending), and writing it is "set both if > level, just irq->pending if edge". That in turn gives you enough > information I think to cope with restores of all of (config, > level, pending-latch) in any order. It feels a bit fragile, though. Right, thanks for working this out. I agree it's fragile and I cannot seem to easily convince myself it would be correct, so I sent out a patch to get rid of the pending cached state which should simplify the save/restore patches as well. Assuming the other VGIC suspects don't object to that, I suggest we base these patches on top of that one and see if we can convince ourselves that it's correct then. Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index 98658d9d..f347779 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -191,6 +191,13 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 + #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 /* KVM_IRQ_LINE irq field index values */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 91c7137..4100f8c 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -211,6 +211,12 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6 +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7 +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10 +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \ + (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff +#define VGIC_LEVEL_INFO_LINE_LEVEL 0 #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c index b6266fe..d5f7197 100644 --- a/virt/kvm/arm/vgic/vgic-kvm-device.c +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c @@ -510,6 +510,21 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev, regid, reg); break; } + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { + unsigned int info, intid; + + info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT; + if (info == VGIC_LEVEL_INFO_LINE_LEVEL) { + intid = attr->attr & + KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK; + ret = vgic_v3_line_level_info_uaccess(vcpu, is_write, + intid, reg); + } else { + ret = -EINVAL; + } + break; + } default: ret = -EINVAL; break; @@ -552,6 +567,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev, return vgic_v3_attr_regs_access(dev, attr, ®, true); } + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { + u32 __user *uaddr = (u32 __user *)(long)attr->addr; + u64 reg; + u32 tmp32; + + if (get_user(tmp32, uaddr)) + return -EFAULT; + + reg = tmp32; + return vgic_v3_attr_regs_access(dev, attr, ®, true); + } } return -ENXIO; } @@ -587,8 +613,18 @@ static int vgic_v3_get_attr(struct kvm_device *dev, return ret; return put_user(reg, uaddr); } - } + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { + u32 __user *uaddr = (u32 __user *)(long)attr->addr; + u64 reg; + u32 tmp32; + ret = vgic_v3_attr_regs_access(dev, attr, ®, false); + if (ret) + return ret; + tmp32 = reg; + return put_user(tmp32, uaddr); + } + } return -ENXIO; } @@ -609,6 +645,13 @@ static int vgic_v3_has_attr(struct kvm_device *dev, return vgic_v3_has_attr_regs(dev, attr); case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: return 0; + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: { + if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >> + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT) == + VGIC_LEVEL_INFO_LINE_LEVEL) + return 0; + break; + } case KVM_DEV_ARM_VGIC_GRP_CTRL: switch (attr->attr) { case KVM_DEV_ARM_VGIC_CTRL_INIT: diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c index 51439c9..9491d72 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c @@ -809,3 +809,14 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write, return vgic_uaccess(vcpu, &rd_dev, is_write, offset, val); } + +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write, + u32 intid, u64 *val) +{ + if (is_write) + vgic_write_irq_line_level_info(vcpu, intid, *val); + else + *val = vgic_read_irq_line_level_info(vcpu, intid); + + return 0; +} diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c index f81e0e5..28fef92b 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.c +++ b/virt/kvm/arm/vgic/vgic-mmio.c @@ -371,6 +371,52 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu, } } +u64 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid) +{ + int i; + u64 val = 0; + + for (i = 0; i < 32; i++) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + if (irq->line_level) + val |= (1U << i); + + vgic_put_irq(vcpu->kvm, irq); + } + + return val; +} + +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, + const u64 val) +{ + int i; + + for (i = 0; i < 32; i++) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + spin_lock(&irq->irq_lock); + if (val & (1U << i)) { + if (irq->config == VGIC_CONFIG_LEVEL) { + irq->line_level = true; + irq->pending = true; + vgic_queue_irq_unlock(vcpu->kvm, irq); + } else { + spin_unlock(&irq->irq_lock); + } + } else { + if (irq->config == VGIC_CONFIG_EDGE || + (irq->config == VGIC_CONFIG_LEVEL && + !irq->soft_pending)) + irq->line_level = false; + spin_unlock(&irq->irq_lock); + } + + vgic_put_irq(vcpu->kvm, irq); + } +} + static int match_region(const void *key, const void *elt) { const unsigned int offset = (unsigned long)key; diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h index 1cc7faf..b9423b4 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.h +++ b/virt/kvm/arm/vgic/vgic-mmio.h @@ -181,6 +181,11 @@ int vgic_validate_mmio_region_addr(struct kvm_device *dev, const struct vgic_register_region *regions, int nr_regions, gpa_t addr); +u64 vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid); + +void vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid, + const u64 val); + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev); diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index eac272c..60d4350 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -144,6 +144,8 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id, u64 *val); int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id, u64 *reg); +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write, + u32 intid, u64 *val); int kvm_register_vgic_device(unsigned long type); void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);