| Message ID | 1462531568-9799-48-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/05/16 11:46, Andre Przywara wrote: > Using the VMCR accessors we provide access to GIC CPU interface state > to userland by wiring it up to the existing userland interface. > [Marc: move and make VMCR accessors static, streamline MMIO handlers] > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > Changelog v2 .. v3: > - total rework, moving into vgic-mmio-v2.c > - move vmcr accessor wrapper functions into this file > - use the register description table for CPU i/f registers as well > - add RAZ/WI handling for the active priority registers > - streamline MMIO handler functions > > virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- > virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic.h | 2 + > 3 files changed, 107 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > index bb33af8..2122ff2 100644 > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > > switch (attr->group) { > case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > - ret = -EINVAL; > + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); > break; > case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index c453e6f..0060539 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, > } > } > > +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > +{ > + if (kvm_vgic_global_state.type == VGIC_V2) > + vgic_v2_set_vmcr(vcpu, vmcr); > + else > + vgic_v3_set_vmcr(vcpu, vmcr); > +} > + > +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > +{ > + if (kvm_vgic_global_state.type == VGIC_V2) > + vgic_v2_get_vmcr(vcpu, vmcr); > + else > + vgic_v3_get_vmcr(vcpu, vmcr); > +} > + > +#define GICC_ARCH_VERSION_V2 0x2 > + > +/* These are for userland accesses only, there is no guest-facing emulation. */ > +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len) > +{ > + struct vgic_vmcr vmcr; > + u32 val; > + > + vgic_get_vmcr(vcpu, &vmcr); > + > + switch (addr & 0xff) { > + case GIC_CPU_CTRL: > + val = vmcr.ctlr; > + break; > + case GIC_CPU_PRIMASK: > + val = vmcr.pmr; > + break; > + case GIC_CPU_BINPOINT: > + val = vmcr.bpr; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + val = vmcr.abpr; > + break; > + case GIC_CPU_IDENT: > + val = ((PRODUCT_ID_KVM << 20) | > + (GICC_ARCH_VERSION_V2 << 16) | > + IMPLEMENTER_ARM); > + break; > + default: > + return 0; > + } > + > + return extract_bytes(val, addr & 3, len); > +} > + > +static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + struct vgic_vmcr vmcr; > + > + vgic_get_vmcr(vcpu, &vmcr); > + > + switch (addr & 0xff) { > + case GIC_CPU_CTRL: > + vmcr.ctlr = val; > + break; > + case GIC_CPU_PRIMASK: > + vmcr.pmr = val; > + break; > + case GIC_CPU_BINPOINT: > + vmcr.bpr = val; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + vmcr.abpr = val; > + break; > + } > + > + vgic_set_vmcr(vcpu, &vmcr); > +} > + > static const struct vgic_register_region vgic_v2_dist_registers[] = { > REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL, > vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12), > @@ -237,6 +315,21 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16), > }; > > +static const struct vgic_register_region vgic_v2_cpu_registers[] = { > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_CTRL, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_PRIMASK, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_BINPOINT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO, > + vgic_mmio_read_raz, vgic_mmio_write_wi, 16), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > +}; > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev) > { > dev->regions = vgic_v2_dist_registers; > @@ -306,6 +399,17 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, > return ret; > } > > +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + int offset, u32 *val) > +{ > + struct vgic_io_device dev = { > + .regions = vgic_v2_cpu_registers, > + .nr_regions = ARRAY_SIZE(vgic_v2_cpu_registers), > + }; > + > + return vgic_uaccess(vcpu, &dev, is_write, offset, val); > +} > + > int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > int offset, u32 *val) > { And what about this: +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS; + const struct vgic_register_region *regions; + gpa_t addr; + int nr_regions, i, len; + + addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; + + switch (attr->group) { + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: + regions = vgic_v2_dist_registers; + nr_regions = ARRAY_SIZE(vgic_v2_dist_registers); + break; + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: + return -ENXIO; /* TODO: describe CPU i/f regs also */ + default: + return -ENXIO; + } This TODO was already in the previous version. Can you please wire it and give save/restore a chance to work? Thanks, M.
Hi, On 09/05/16 18:27, Marc Zyngier wrote: > On 06/05/16 11:46, Andre Przywara wrote: >> Using the VMCR accessors we provide access to GIC CPU interface state >> to userland by wiring it up to the existing userland interface. >> [Marc: move and make VMCR accessors static, streamline MMIO handlers] >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> Changelog v2 .. v3: >> - total rework, moving into vgic-mmio-v2.c >> - move vmcr accessor wrapper functions into this file >> - use the register description table for CPU i/f registers as well >> - add RAZ/WI handling for the active priority registers >> - streamline MMIO handler functions >> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic.h | 2 + >> 3 files changed, 107 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c >> index bb33af8..2122ff2 100644 >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c >> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, >> >> switch (attr->group) { >> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: >> - ret = -EINVAL; >> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); >> break; >> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: >> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> index c453e6f..0060539 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, >> } >> } >> >> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >> +{ >> + if (kvm_vgic_global_state.type == VGIC_V2) >> + vgic_v2_set_vmcr(vcpu, vmcr); >> + else >> + vgic_v3_set_vmcr(vcpu, vmcr); >> +} >> + >> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >> +{ >> + if (kvm_vgic_global_state.type == VGIC_V2) >> + vgic_v2_get_vmcr(vcpu, vmcr); >> + else >> + vgic_v3_get_vmcr(vcpu, vmcr); >> +} >> + >> +#define GICC_ARCH_VERSION_V2 0x2 >> + >> +/* These are for userland accesses only, there is no guest-facing emulation. */ >> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len) >> +{ >> + struct vgic_vmcr vmcr; >> + u32 val; >> + >> + vgic_get_vmcr(vcpu, &vmcr); >> + >> + switch (addr & 0xff) { >> + case GIC_CPU_CTRL: >> + val = vmcr.ctlr; >> + break; >> + case GIC_CPU_PRIMASK: >> + val = vmcr.pmr; >> + break; >> + case GIC_CPU_BINPOINT: >> + val = vmcr.bpr; >> + break; >> + case GIC_CPU_ALIAS_BINPOINT: >> + val = vmcr.abpr; >> + break; >> + case GIC_CPU_IDENT: >> + val = ((PRODUCT_ID_KVM << 20) | >> + (GICC_ARCH_VERSION_V2 << 16) | >> + IMPLEMENTER_ARM); >> + break; >> + default: >> + return 0; >> + } >> + >> + return extract_bytes(val, addr & 3, len); >> +} >> + >> +static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + struct vgic_vmcr vmcr; >> + >> + vgic_get_vmcr(vcpu, &vmcr); >> + >> + switch (addr & 0xff) { >> + case GIC_CPU_CTRL: >> + vmcr.ctlr = val; >> + break; >> + case GIC_CPU_PRIMASK: >> + vmcr.pmr = val; >> + break; >> + case GIC_CPU_BINPOINT: >> + vmcr.bpr = val; >> + break; >> + case GIC_CPU_ALIAS_BINPOINT: >> + vmcr.abpr = val; >> + break; >> + } >> + >> + vgic_set_vmcr(vcpu, &vmcr); >> +} >> + >> static const struct vgic_register_region vgic_v2_dist_registers[] = { >> REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL, >> vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12), >> @@ -237,6 +315,21 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { >> vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16), >> }; >> >> +static const struct vgic_register_region vgic_v2_cpu_registers[] = { >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_CTRL, >> + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_PRIMASK, >> + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_BINPOINT, >> + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT, >> + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO, >> + vgic_mmio_read_raz, vgic_mmio_write_wi, 16), >> + REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT, >> + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), >> +}; >> + >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev) >> { >> dev->regions = vgic_v2_dist_registers; >> @@ -306,6 +399,17 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, >> return ret; >> } >> >> +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, >> + int offset, u32 *val) >> +{ >> + struct vgic_io_device dev = { >> + .regions = vgic_v2_cpu_registers, >> + .nr_regions = ARRAY_SIZE(vgic_v2_cpu_registers), >> + }; >> + >> + return vgic_uaccess(vcpu, &dev, is_write, offset, val); >> +} >> + >> int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, >> int offset, u32 *val) >> { > > And what about this: > > +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr) > +{ > + int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS; > + const struct vgic_register_region *regions; > + gpa_t addr; > + int nr_regions, i, len; > + > + addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; > + > + switch (attr->group) { > + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > + regions = vgic_v2_dist_registers; > + nr_regions = ARRAY_SIZE(vgic_v2_dist_registers); > + break; > + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > + return -ENXIO; /* TODO: describe CPU i/f regs also */ > + default: > + return -ENXIO; > + } > > This TODO was already in the previous version. Can you please wire it > and give save/restore a chance to work? Oh dear, thanks for spotting this. It _was_ in my fix patch, but got lost during the rebase. Will fix it. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: > Using the VMCR accessors we provide access to GIC CPU interface state > to userland by wiring it up to the existing userland interface. > [Marc: move and make VMCR accessors static, streamline MMIO handlers] does this mean Marc did this and serves as credit or is it a lost reminder? > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > Changelog v2 .. v3: > - total rework, moving into vgic-mmio-v2.c > - move vmcr accessor wrapper functions into this file > - use the register description table for CPU i/f registers as well > - add RAZ/WI handling for the active priority registers > - streamline MMIO handler functions > > virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- > virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic.h | 2 + > 3 files changed, 107 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > index bb33af8..2122ff2 100644 > --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > > switch (attr->group) { > case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > - ret = -EINVAL; > + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); > break; > case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index c453e6f..0060539 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, > } > } > > +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > +{ > + if (kvm_vgic_global_state.type == VGIC_V2) > + vgic_v2_set_vmcr(vcpu, vmcr); > + else > + vgic_v3_set_vmcr(vcpu, vmcr); > +} > + > +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > +{ > + if (kvm_vgic_global_state.type == VGIC_V2) > + vgic_v2_get_vmcr(vcpu, vmcr); > + else > + vgic_v3_get_vmcr(vcpu, vmcr); > +} > + > +#define GICC_ARCH_VERSION_V2 0x2 > + > +/* These are for userland accesses only, there is no guest-facing emulation. */ > +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len) > +{ > + struct vgic_vmcr vmcr; > + u32 val; > + > + vgic_get_vmcr(vcpu, &vmcr); > + > + switch (addr & 0xff) { > + case GIC_CPU_CTRL: > + val = vmcr.ctlr; > + break; > + case GIC_CPU_PRIMASK: > + val = vmcr.pmr; > + break; > + case GIC_CPU_BINPOINT: > + val = vmcr.bpr; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + val = vmcr.abpr; > + break; > + case GIC_CPU_IDENT: > + val = ((PRODUCT_ID_KVM << 20) | > + (GICC_ARCH_VERSION_V2 << 16) | > + IMPLEMENTER_ARM); > + break; > + default: > + return 0; > + } > + > + return extract_bytes(val, addr & 3, len); I don't think we allow anything than a full 32-bit aligned accesses from userspace - we shouldn't at least. > +} > + > +static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + struct vgic_vmcr vmcr; > + > + vgic_get_vmcr(vcpu, &vmcr); > + > + switch (addr & 0xff) { > + case GIC_CPU_CTRL: > + vmcr.ctlr = val; > + break; > + case GIC_CPU_PRIMASK: > + vmcr.pmr = val; > + break; > + case GIC_CPU_BINPOINT: > + vmcr.bpr = val; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + vmcr.abpr = val; > + break; > + } > + > + vgic_set_vmcr(vcpu, &vmcr); > +} > + > static const struct vgic_register_region vgic_v2_dist_registers[] = { > REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL, > vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12), > @@ -237,6 +315,21 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16), > }; > > +static const struct vgic_register_region vgic_v2_cpu_registers[] = { > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_CTRL, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_PRIMASK, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_BINPOINT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO, > + vgic_mmio_read_raz, vgic_mmio_write_wi, 16), > + REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT, > + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), > +}; > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev) > { > dev->regions = vgic_v2_dist_registers; > @@ -306,6 +399,17 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, > return ret; > } > > +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + int offset, u32 *val) > +{ > + struct vgic_io_device dev = { > + .regions = vgic_v2_cpu_registers, > + .nr_regions = ARRAY_SIZE(vgic_v2_cpu_registers), > + }; > + > + return vgic_uaccess(vcpu, &dev, is_write, offset, val); > +} > + > int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > int offset, u32 *val) > { > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index 5260d23..7a69955 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -39,6 +39,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu); > int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr); > int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, > int offset, u32 *val); > +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, > + int offset, u32 *val); > void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); > void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); > int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address, > -- > 2.7.3 > Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 12/05/16 19:47, Christoffer Dall wrote: > On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: >> Using the VMCR accessors we provide access to GIC CPU interface state >> to userland by wiring it up to the existing userland interface. >> [Marc: move and make VMCR accessors static, streamline MMIO handlers] > > does this mean Marc did this and serves as credit or is it a lost > reminder? It was meant as credit. I thought that is the usual annotation for this? >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> Changelog v2 .. v3: >> - total rework, moving into vgic-mmio-v2.c >> - move vmcr accessor wrapper functions into this file >> - use the register description table for CPU i/f registers as well >> - add RAZ/WI handling for the active priority registers >> - streamline MMIO handler functions >> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic.h | 2 + >> 3 files changed, 107 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c >> index bb33af8..2122ff2 100644 >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c >> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, >> >> switch (attr->group) { >> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: >> - ret = -EINVAL; >> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); >> break; >> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: >> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> index c453e6f..0060539 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, >> } >> } >> >> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >> +{ >> + if (kvm_vgic_global_state.type == VGIC_V2) >> + vgic_v2_set_vmcr(vcpu, vmcr); >> + else >> + vgic_v3_set_vmcr(vcpu, vmcr); >> +} >> + >> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >> +{ >> + if (kvm_vgic_global_state.type == VGIC_V2) >> + vgic_v2_get_vmcr(vcpu, vmcr); >> + else >> + vgic_v3_get_vmcr(vcpu, vmcr); >> +} >> + >> +#define GICC_ARCH_VERSION_V2 0x2 >> + >> +/* These are for userland accesses only, there is no guest-facing emulation. */ >> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len) >> +{ >> + struct vgic_vmcr vmcr; >> + u32 val; >> + >> + vgic_get_vmcr(vcpu, &vmcr); >> + >> + switch (addr & 0xff) { >> + case GIC_CPU_CTRL: >> + val = vmcr.ctlr; >> + break; >> + case GIC_CPU_PRIMASK: >> + val = vmcr.pmr; >> + break; >> + case GIC_CPU_BINPOINT: >> + val = vmcr.bpr; >> + break; >> + case GIC_CPU_ALIAS_BINPOINT: >> + val = vmcr.abpr; >> + break; >> + case GIC_CPU_IDENT: >> + val = ((PRODUCT_ID_KVM << 20) | >> + (GICC_ARCH_VERSION_V2 << 16) | >> + IMPLEMENTER_ARM); >> + break; >> + default: >> + return 0; >> + } >> + >> + return extract_bytes(val, addr & 3, len); > > I don't think we allow anything than a full 32-bit aligned accesses > from userspace - we shouldn't at least. Indeed - I think userland was always 32-bit only. And since last night we even enforce this. So potentially there are more extract_bytes() calls that can go. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 12, 2016 at 07:52:38PM +0100, Andre Przywara wrote: > Hi, > > On 12/05/16 19:47, Christoffer Dall wrote: > > On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: > >> Using the VMCR accessors we provide access to GIC CPU interface state > >> to userland by wiring it up to the existing userland interface. > >> [Marc: move and make VMCR accessors static, streamline MMIO handlers] > > > > does this mean Marc did this and serves as credit or is it a lost > > reminder? > > It was meant as credit. I thought that is the usual annotation for this? > I'm not sure if that's the usual way, I read it as a reminder, but it's not too important. Mostly wanting to make sure we're not forgetting some todo item. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > >> Changelog v2 .. v3: > >> - total rework, moving into vgic-mmio-v2.c > >> - move vmcr accessor wrapper functions into this file > >> - use the register description table for CPU i/f registers as well > >> - add RAZ/WI handling for the active priority registers > >> - streamline MMIO handler functions > >> > >> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- > >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ > >> virt/kvm/arm/vgic/vgic.h | 2 + > >> 3 files changed, 107 insertions(+), 1 deletion(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > >> index bb33af8..2122ff2 100644 > >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > >> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > >> > >> switch (attr->group) { > >> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > >> - ret = -EINVAL; > >> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); > >> break; > >> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > >> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); > >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> index c453e6f..0060539 100644 > >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, > >> } > >> } > >> > >> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >> +{ > >> + if (kvm_vgic_global_state.type == VGIC_V2) > >> + vgic_v2_set_vmcr(vcpu, vmcr); > >> + else > >> + vgic_v3_set_vmcr(vcpu, vmcr); > >> +} > >> + > >> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >> +{ > >> + if (kvm_vgic_global_state.type == VGIC_V2) > >> + vgic_v2_get_vmcr(vcpu, vmcr); > >> + else > >> + vgic_v3_get_vmcr(vcpu, vmcr); > >> +} > >> + > >> +#define GICC_ARCH_VERSION_V2 0x2 > >> + > >> +/* These are for userland accesses only, there is no guest-facing emulation. */ > >> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > >> + gpa_t addr, unsigned int len) > >> +{ > >> + struct vgic_vmcr vmcr; > >> + u32 val; > >> + > >> + vgic_get_vmcr(vcpu, &vmcr); > >> + > >> + switch (addr & 0xff) { > >> + case GIC_CPU_CTRL: > >> + val = vmcr.ctlr; > >> + break; > >> + case GIC_CPU_PRIMASK: > >> + val = vmcr.pmr; > >> + break; > >> + case GIC_CPU_BINPOINT: > >> + val = vmcr.bpr; > >> + break; > >> + case GIC_CPU_ALIAS_BINPOINT: > >> + val = vmcr.abpr; > >> + break; > >> + case GIC_CPU_IDENT: > >> + val = ((PRODUCT_ID_KVM << 20) | > >> + (GICC_ARCH_VERSION_V2 << 16) | > >> + IMPLEMENTER_ARM); > >> + break; > >> + default: > >> + return 0; > >> + } > >> + > >> + return extract_bytes(val, addr & 3, len); > > > > I don't think we allow anything than a full 32-bit aligned accesses > > from userspace - we shouldn't at least. > > Indeed - I think userland was always 32-bit only. And since last night > we even enforce this. So potentially there are more extract_bytes() > calls that can go. > Right. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 13/05/16 08:53, Christoffer Dall wrote: > On Thu, May 12, 2016 at 07:52:38PM +0100, Andre Przywara wrote: >> Hi, >> >> On 12/05/16 19:47, Christoffer Dall wrote: >>> On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: >>>> Using the VMCR accessors we provide access to GIC CPU interface state >>>> to userland by wiring it up to the existing userland interface. >>>> [Marc: move and make VMCR accessors static, streamline MMIO handlers] >>> >>> does this mean Marc did this and serves as credit or is it a lost >>> reminder? >> >> It was meant as credit. I thought that is the usual annotation for this? >> > > I'm not sure if that's the usual way, I read it as a reminder, but it's > not too important. Mostly wanting to make sure we're not forgetting > some todo item. > >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> Changelog v2 .. v3: >>>> - total rework, moving into vgic-mmio-v2.c >>>> - move vmcr accessor wrapper functions into this file >>>> - use the register description table for CPU i/f registers as well >>>> - add RAZ/WI handling for the active priority registers >>>> - streamline MMIO handler functions >>>> >>>> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- >>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ >>>> virt/kvm/arm/vgic/vgic.h | 2 + >>>> 3 files changed, 107 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c >>>> index bb33af8..2122ff2 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c >>>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c >>>> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, >>>> >>>> switch (attr->group) { >>>> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: >>>> - ret = -EINVAL; >>>> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); >>>> break; >>>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: >>>> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> index c453e6f..0060539 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, >>>> } >>>> } >>>> >>>> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >>>> +{ >>>> + if (kvm_vgic_global_state.type == VGIC_V2) >>>> + vgic_v2_set_vmcr(vcpu, vmcr); >>>> + else >>>> + vgic_v3_set_vmcr(vcpu, vmcr); >>>> +} >>>> + >>>> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >>>> +{ >>>> + if (kvm_vgic_global_state.type == VGIC_V2) >>>> + vgic_v2_get_vmcr(vcpu, vmcr); >>>> + else >>>> + vgic_v3_get_vmcr(vcpu, vmcr); >>>> +} >>>> + >>>> +#define GICC_ARCH_VERSION_V2 0x2 >>>> + >>>> +/* These are for userland accesses only, there is no guest-facing emulation. */ >>>> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >>>> + gpa_t addr, unsigned int len) >>>> +{ >>>> + struct vgic_vmcr vmcr; >>>> + u32 val; >>>> + >>>> + vgic_get_vmcr(vcpu, &vmcr); >>>> + >>>> + switch (addr & 0xff) { >>>> + case GIC_CPU_CTRL: >>>> + val = vmcr.ctlr; >>>> + break; >>>> + case GIC_CPU_PRIMASK: >>>> + val = vmcr.pmr; >>>> + break; >>>> + case GIC_CPU_BINPOINT: >>>> + val = vmcr.bpr; >>>> + break; >>>> + case GIC_CPU_ALIAS_BINPOINT: >>>> + val = vmcr.abpr; >>>> + break; >>>> + case GIC_CPU_IDENT: >>>> + val = ((PRODUCT_ID_KVM << 20) | >>>> + (GICC_ARCH_VERSION_V2 << 16) | >>>> + IMPLEMENTER_ARM); >>>> + break; >>>> + default: >>>> + return 0; >>>> + } >>>> + >>>> + return extract_bytes(val, addr & 3, len); >>> >>> I don't think we allow anything than a full 32-bit aligned accesses >>> from userspace - we shouldn't at least. >> >> Indeed - I think userland was always 32-bit only. And since last night >> we even enforce this. So potentially there are more extract_bytes() >> calls that can go. >> > Right. So can I replace every call to extract_bytes() with just a "return val;" for every register that allows 32-bit accesses only? I think that's safe now, just checking ... Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 13, 2016 at 11:44:38AM +0100, Andre Przywara wrote: > Hi, > > On 13/05/16 08:53, Christoffer Dall wrote: > > On Thu, May 12, 2016 at 07:52:38PM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 12/05/16 19:47, Christoffer Dall wrote: > >>> On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: > >>>> Using the VMCR accessors we provide access to GIC CPU interface state > >>>> to userland by wiring it up to the existing userland interface. > >>>> [Marc: move and make VMCR accessors static, streamline MMIO handlers] > >>> > >>> does this mean Marc did this and serves as credit or is it a lost > >>> reminder? > >> > >> It was meant as credit. I thought that is the usual annotation for this? > >> > > > > I'm not sure if that's the usual way, I read it as a reminder, but it's > > not too important. Mostly wanting to make sure we're not forgetting > > some todo item. > > > >>>> > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>> --- > >>>> Changelog v2 .. v3: > >>>> - total rework, moving into vgic-mmio-v2.c > >>>> - move vmcr accessor wrapper functions into this file > >>>> - use the register description table for CPU i/f registers as well > >>>> - add RAZ/WI handling for the active priority registers > >>>> - streamline MMIO handler functions > >>>> > >>>> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- > >>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ > >>>> virt/kvm/arm/vgic/vgic.h | 2 + > >>>> 3 files changed, 107 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>> index bb33af8..2122ff2 100644 > >>>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > >>>> > >>>> switch (attr->group) { > >>>> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > >>>> - ret = -EINVAL; > >>>> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); > >>>> break; > >>>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > >>>> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); > >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> index c453e6f..0060539 100644 > >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, > >>>> } > >>>> } > >>>> > >>>> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >>>> +{ > >>>> + if (kvm_vgic_global_state.type == VGIC_V2) > >>>> + vgic_v2_set_vmcr(vcpu, vmcr); > >>>> + else > >>>> + vgic_v3_set_vmcr(vcpu, vmcr); > >>>> +} > >>>> + > >>>> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >>>> +{ > >>>> + if (kvm_vgic_global_state.type == VGIC_V2) > >>>> + vgic_v2_get_vmcr(vcpu, vmcr); > >>>> + else > >>>> + vgic_v3_get_vmcr(vcpu, vmcr); > >>>> +} > >>>> + > >>>> +#define GICC_ARCH_VERSION_V2 0x2 > >>>> + > >>>> +/* These are for userland accesses only, there is no guest-facing emulation. */ > >>>> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > >>>> + gpa_t addr, unsigned int len) > >>>> +{ > >>>> + struct vgic_vmcr vmcr; > >>>> + u32 val; > >>>> + > >>>> + vgic_get_vmcr(vcpu, &vmcr); > >>>> + > >>>> + switch (addr & 0xff) { > >>>> + case GIC_CPU_CTRL: > >>>> + val = vmcr.ctlr; > >>>> + break; > >>>> + case GIC_CPU_PRIMASK: > >>>> + val = vmcr.pmr; > >>>> + break; > >>>> + case GIC_CPU_BINPOINT: > >>>> + val = vmcr.bpr; > >>>> + break; > >>>> + case GIC_CPU_ALIAS_BINPOINT: > >>>> + val = vmcr.abpr; > >>>> + break; > >>>> + case GIC_CPU_IDENT: > >>>> + val = ((PRODUCT_ID_KVM << 20) | > >>>> + (GICC_ARCH_VERSION_V2 << 16) | > >>>> + IMPLEMENTER_ARM); > >>>> + break; > >>>> + default: > >>>> + return 0; > >>>> + } > >>>> + > >>>> + return extract_bytes(val, addr & 3, len); > >>> > >>> I don't think we allow anything than a full 32-bit aligned accesses > >>> from userspace - we shouldn't at least. > >> > >> Indeed - I think userland was always 32-bit only. And since last night > >> we even enforce this. So potentially there are more extract_bytes() > >> calls that can go. > >> > > Right. > > So can I replace every call to extract_bytes() with just a "return val;" > for every register that allows 32-bit accesses only? > I think that's safe now, just checking ... yes, I think the way we do it now, you simply return val (asuming you build that variable at the right offset, even for byte accesses). The only exception is for 32-bit accesses to 64-bit registers, where you have to return either the upper or lower 32-bits. I think you can still use extract_bytes() there should you be so inclined. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 13/05/16 12:54, Christoffer Dall wrote: > On Fri, May 13, 2016 at 11:44:38AM +0100, Andre Przywara wrote: >> Hi, >> >> On 13/05/16 08:53, Christoffer Dall wrote: >>> On Thu, May 12, 2016 at 07:52:38PM +0100, Andre Przywara wrote: >>>> Hi, >>>> >>>> On 12/05/16 19:47, Christoffer Dall wrote: >>>>> On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: >>>>>> Using the VMCR accessors we provide access to GIC CPU interface state >>>>>> to userland by wiring it up to the existing userland interface. >>>>>> [Marc: move and make VMCR accessors static, streamline MMIO handlers] >>>>> >>>>> does this mean Marc did this and serves as credit or is it a lost >>>>> reminder? >>>> >>>> It was meant as credit. I thought that is the usual annotation for this? >>>> >>> >>> I'm not sure if that's the usual way, I read it as a reminder, but it's >>> not too important. Mostly wanting to make sure we're not forgetting >>> some todo item. >>> >>>>>> >>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>>> --- >>>>>> Changelog v2 .. v3: >>>>>> - total rework, moving into vgic-mmio-v2.c >>>>>> - move vmcr accessor wrapper functions into this file >>>>>> - use the register description table for CPU i/f registers as well >>>>>> - add RAZ/WI handling for the active priority registers >>>>>> - streamline MMIO handler functions >>>>>> >>>>>> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- >>>>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ >>>>>> virt/kvm/arm/vgic/vgic.h | 2 + >>>>>> 3 files changed, 107 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c >>>>>> index bb33af8..2122ff2 100644 >>>>>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c >>>>>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c >>>>>> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, >>>>>> >>>>>> switch (attr->group) { >>>>>> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: >>>>>> - ret = -EINVAL; >>>>>> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); >>>>>> break; >>>>>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: >>>>>> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); >>>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>>>> index c453e6f..0060539 100644 >>>>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >>>>>> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, >>>>>> } >>>>>> } >>>>>> >>>>>> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >>>>>> +{ >>>>>> + if (kvm_vgic_global_state.type == VGIC_V2) >>>>>> + vgic_v2_set_vmcr(vcpu, vmcr); >>>>>> + else >>>>>> + vgic_v3_set_vmcr(vcpu, vmcr); >>>>>> +} >>>>>> + >>>>>> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) >>>>>> +{ >>>>>> + if (kvm_vgic_global_state.type == VGIC_V2) >>>>>> + vgic_v2_get_vmcr(vcpu, vmcr); >>>>>> + else >>>>>> + vgic_v3_get_vmcr(vcpu, vmcr); >>>>>> +} >>>>>> + >>>>>> +#define GICC_ARCH_VERSION_V2 0x2 >>>>>> + >>>>>> +/* These are for userland accesses only, there is no guest-facing emulation. */ >>>>>> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, >>>>>> + gpa_t addr, unsigned int len) >>>>>> +{ >>>>>> + struct vgic_vmcr vmcr; >>>>>> + u32 val; >>>>>> + >>>>>> + vgic_get_vmcr(vcpu, &vmcr); >>>>>> + >>>>>> + switch (addr & 0xff) { >>>>>> + case GIC_CPU_CTRL: >>>>>> + val = vmcr.ctlr; >>>>>> + break; >>>>>> + case GIC_CPU_PRIMASK: >>>>>> + val = vmcr.pmr; >>>>>> + break; >>>>>> + case GIC_CPU_BINPOINT: >>>>>> + val = vmcr.bpr; >>>>>> + break; >>>>>> + case GIC_CPU_ALIAS_BINPOINT: >>>>>> + val = vmcr.abpr; >>>>>> + break; >>>>>> + case GIC_CPU_IDENT: >>>>>> + val = ((PRODUCT_ID_KVM << 20) | >>>>>> + (GICC_ARCH_VERSION_V2 << 16) | >>>>>> + IMPLEMENTER_ARM); >>>>>> + break; >>>>>> + default: >>>>>> + return 0; >>>>>> + } >>>>>> + >>>>>> + return extract_bytes(val, addr & 3, len); >>>>> >>>>> I don't think we allow anything than a full 32-bit aligned accesses >>>>> from userspace - we shouldn't at least. >>>> >>>> Indeed - I think userland was always 32-bit only. And since last night >>>> we even enforce this. So potentially there are more extract_bytes() >>>> calls that can go. >>>> >>> Right. >> >> So can I replace every call to extract_bytes() with just a "return val;" >> for every register that allows 32-bit accesses only? >> I think that's safe now, just checking ... > > yes, I think the way we do it now, you simply return val (asuming you > build that variable at the right offset, even for byte accesses). ??? If we only have word accesses, then we don't need to care about byte accesses? Or do I got something wrong here? > The only exception is for 32-bit accesses to 64-bit registers, where you > have to return either the upper or lower 32-bits. I think you can still > use extract_bytes() there should you be so inclined. Yeah, in fact GICR_TYPER and GICD_IROUTER are the only users remaining afterwards. So I moved the declaration into vgic-mmio-v3.c and renamed it to extract_words() on the way. Will send the patch in a minute ... Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 13, 2016 at 01:23:02PM +0100, Andre Przywara wrote: > Hi, > > On 13/05/16 12:54, Christoffer Dall wrote: > > On Fri, May 13, 2016 at 11:44:38AM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 13/05/16 08:53, Christoffer Dall wrote: > >>> On Thu, May 12, 2016 at 07:52:38PM +0100, Andre Przywara wrote: > >>>> Hi, > >>>> > >>>> On 12/05/16 19:47, Christoffer Dall wrote: > >>>>> On Fri, May 06, 2016 at 11:46:00AM +0100, Andre Przywara wrote: > >>>>>> Using the VMCR accessors we provide access to GIC CPU interface state > >>>>>> to userland by wiring it up to the existing userland interface. > >>>>>> [Marc: move and make VMCR accessors static, streamline MMIO handlers] > >>>>> > >>>>> does this mean Marc did this and serves as credit or is it a lost > >>>>> reminder? > >>>> > >>>> It was meant as credit. I thought that is the usual annotation for this? > >>>> > >>> > >>> I'm not sure if that's the usual way, I read it as a reminder, but it's > >>> not too important. Mostly wanting to make sure we're not forgetting > >>> some todo item. > >>> > >>>>>> > >>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>>>> --- > >>>>>> Changelog v2 .. v3: > >>>>>> - total rework, moving into vgic-mmio-v2.c > >>>>>> - move vmcr accessor wrapper functions into this file > >>>>>> - use the register description table for CPU i/f registers as well > >>>>>> - add RAZ/WI handling for the active priority registers > >>>>>> - streamline MMIO handler functions > >>>>>> > >>>>>> virt/kvm/arm/vgic/vgic-kvm-device.c | 2 +- > >>>>>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 104 ++++++++++++++++++++++++++++++++++++ > >>>>>> virt/kvm/arm/vgic/vgic.h | 2 + > >>>>>> 3 files changed, 107 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>>>> index bb33af8..2122ff2 100644 > >>>>>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>>>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > >>>>>> @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > >>>>>> > >>>>>> switch (attr->group) { > >>>>>> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: > >>>>>> - ret = -EINVAL; > >>>>>> + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); > >>>>>> break; > >>>>>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: > >>>>>> ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); > >>>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>>>> index c453e6f..0060539 100644 > >>>>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > >>>>>> @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, > >>>>>> } > >>>>>> } > >>>>>> > >>>>>> +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >>>>>> +{ > >>>>>> + if (kvm_vgic_global_state.type == VGIC_V2) > >>>>>> + vgic_v2_set_vmcr(vcpu, vmcr); > >>>>>> + else > >>>>>> + vgic_v3_set_vmcr(vcpu, vmcr); > >>>>>> +} > >>>>>> + > >>>>>> +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) > >>>>>> +{ > >>>>>> + if (kvm_vgic_global_state.type == VGIC_V2) > >>>>>> + vgic_v2_get_vmcr(vcpu, vmcr); > >>>>>> + else > >>>>>> + vgic_v3_get_vmcr(vcpu, vmcr); > >>>>>> +} > >>>>>> + > >>>>>> +#define GICC_ARCH_VERSION_V2 0x2 > >>>>>> + > >>>>>> +/* These are for userland accesses only, there is no guest-facing emulation. */ > >>>>>> +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, > >>>>>> + gpa_t addr, unsigned int len) > >>>>>> +{ > >>>>>> + struct vgic_vmcr vmcr; > >>>>>> + u32 val; > >>>>>> + > >>>>>> + vgic_get_vmcr(vcpu, &vmcr); > >>>>>> + > >>>>>> + switch (addr & 0xff) { > >>>>>> + case GIC_CPU_CTRL: > >>>>>> + val = vmcr.ctlr; > >>>>>> + break; > >>>>>> + case GIC_CPU_PRIMASK: > >>>>>> + val = vmcr.pmr; > >>>>>> + break; > >>>>>> + case GIC_CPU_BINPOINT: > >>>>>> + val = vmcr.bpr; > >>>>>> + break; > >>>>>> + case GIC_CPU_ALIAS_BINPOINT: > >>>>>> + val = vmcr.abpr; > >>>>>> + break; > >>>>>> + case GIC_CPU_IDENT: > >>>>>> + val = ((PRODUCT_ID_KVM << 20) | > >>>>>> + (GICC_ARCH_VERSION_V2 << 16) | > >>>>>> + IMPLEMENTER_ARM); > >>>>>> + break; > >>>>>> + default: > >>>>>> + return 0; > >>>>>> + } > >>>>>> + > >>>>>> + return extract_bytes(val, addr & 3, len); > >>>>> > >>>>> I don't think we allow anything than a full 32-bit aligned accesses > >>>>> from userspace - we shouldn't at least. > >>>> > >>>> Indeed - I think userland was always 32-bit only. And since last night > >>>> we even enforce this. So potentially there are more extract_bytes() > >>>> calls that can go. > >>>> > >>> Right. > >> > >> So can I replace every call to extract_bytes() with just a "return val;" > >> for every register that allows 32-bit accesses only? > >> I think that's safe now, just checking ... > > > > yes, I think the way we do it now, you simply return val (asuming you > > build that variable at the right offset, even for byte accesses). > > ??? If we only have word accesses, then we don't need to care about byte > accesses? Or do I got something wrong here? > Ah right, we're talking about the userspace API here, never mind. > > The only exception is for 32-bit accesses to 64-bit registers, where you > > have to return either the upper or lower 32-bits. I think you can still > > use extract_bytes() there should you be so inclined. > > Yeah, in fact GICR_TYPER and GICD_IROUTER are the only users remaining > afterwards. So I moved the declaration into vgic-mmio-v3.c and renamed > it to extract_words() on the way. did you modify it to only work on words and let len specify the length in words then, or? > Will send the patch in a minute ... > I guess I can comment on it there. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c index bb33af8..2122ff2 100644 --- a/virt/kvm/arm/vgic/vgic-kvm-device.c +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c @@ -300,7 +300,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, switch (attr->group) { case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: - ret = -EINVAL; + ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg); break; case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg); diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c index c453e6f..0060539 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c @@ -206,6 +206,84 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu, } } +static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) +{ + if (kvm_vgic_global_state.type == VGIC_V2) + vgic_v2_set_vmcr(vcpu, vmcr); + else + vgic_v3_set_vmcr(vcpu, vmcr); +} + +static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr) +{ + if (kvm_vgic_global_state.type == VGIC_V2) + vgic_v2_get_vmcr(vcpu, vmcr); + else + vgic_v3_get_vmcr(vcpu, vmcr); +} + +#define GICC_ARCH_VERSION_V2 0x2 + +/* These are for userland accesses only, there is no guest-facing emulation. */ +static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len) +{ + struct vgic_vmcr vmcr; + u32 val; + + vgic_get_vmcr(vcpu, &vmcr); + + switch (addr & 0xff) { + case GIC_CPU_CTRL: + val = vmcr.ctlr; + break; + case GIC_CPU_PRIMASK: + val = vmcr.pmr; + break; + case GIC_CPU_BINPOINT: + val = vmcr.bpr; + break; + case GIC_CPU_ALIAS_BINPOINT: + val = vmcr.abpr; + break; + case GIC_CPU_IDENT: + val = ((PRODUCT_ID_KVM << 20) | + (GICC_ARCH_VERSION_V2 << 16) | + IMPLEMENTER_ARM); + break; + default: + return 0; + } + + return extract_bytes(val, addr & 3, len); +} + +static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val) +{ + struct vgic_vmcr vmcr; + + vgic_get_vmcr(vcpu, &vmcr); + + switch (addr & 0xff) { + case GIC_CPU_CTRL: + vmcr.ctlr = val; + break; + case GIC_CPU_PRIMASK: + vmcr.pmr = val; + break; + case GIC_CPU_BINPOINT: + vmcr.bpr = val; + break; + case GIC_CPU_ALIAS_BINPOINT: + vmcr.abpr = val; + break; + } + + vgic_set_vmcr(vcpu, &vmcr); +} + static const struct vgic_register_region vgic_v2_dist_registers[] = { REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL, vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12), @@ -237,6 +315,21 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16), }; +static const struct vgic_register_region vgic_v2_cpu_registers[] = { + REGISTER_DESC_WITH_LENGTH(GIC_CPU_CTRL, + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), + REGISTER_DESC_WITH_LENGTH(GIC_CPU_PRIMASK, + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), + REGISTER_DESC_WITH_LENGTH(GIC_CPU_BINPOINT, + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT, + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), + REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO, + vgic_mmio_read_raz, vgic_mmio_write_wi, 16), + REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT, + vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4), +}; + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev) { dev->regions = vgic_v2_dist_registers; @@ -306,6 +399,17 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, return ret; } +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, + int offset, u32 *val) +{ + struct vgic_io_device dev = { + .regions = vgic_v2_cpu_registers, + .nr_regions = ARRAY_SIZE(vgic_v2_cpu_registers), + }; + + return vgic_uaccess(vcpu, &dev, is_write, offset, val); +} + int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, int offset, u32 *val) { diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index 5260d23..7a69955 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -39,6 +39,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu); int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr); int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, int offset, u32 *val); +int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, + int offset, u32 *val); void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr); int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,