On Tue, Aug 02, 2016 at 03:46:49PM +0100, Marc Zyngier wrote:
> On 02/08/16 15:33, Christoffer Dall wrote:
> > On Tue, Aug 02, 2016 at 11:12:46AM +0100, Marc Zyngier wrote:
> >> On 02/08/16 10:40, Andre Przywara wrote:
> >>> Hi,
> >>>
> >>> On 01/08/16 19:20, Christoffer Dall wrote:
> >>>> On Fri, Jul 22, 2016 at 06:28:50PM +0100, Marc Zyngier wrote:
> >>>>> From: Andre Przywara <andre.przywara@xxxxxxx>
> >>>>>
> >>>>> In the GICv3 redistributor there are the PENDBASER and PROPBASER
> >>>>> registers which we did not emulate so far, as they only make sense
> >>>>> when having an ITS. In preparation for that emulate those MMIO
> >>>>> accesses by storing the 64-bit data written into it into a variable
> >>>>> which we later read in the ITS emulation.
> >>>>> We also sanitise the registers, making sure RES0 regions are respected
> >>>>> and checking for valid memory attributes.
> >>>>>
> >>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >>>>> Reviewed-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >>>>> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx>
> >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >>>>> ---
> >>>>> include/kvm/arm_vgic.h | 13 ++++
> >>>>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 153 ++++++++++++++++++++++++++++++++++++++-
> >>>>> virt/kvm/arm/vgic/vgic-mmio.h | 8 ++
> >>>>> virt/kvm/arm/vgic/vgic-v3.c | 11 ++-
> >>>>> 4 files changed, 181 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >>>>> index 450b4da..df2dec5 100644
> >>>>> --- a/include/kvm/arm_vgic.h
> >>>>> +++ b/include/kvm/arm_vgic.h
> >>>>> @@ -146,6 +146,14 @@ struct vgic_dist {
> >>>>> struct vgic_irq *spis;
> >>>>>
> >>>>> struct vgic_io_device dist_iodev;
> >>>>> +
> >>>>> + /*
> >>>>> + * Contains the attributes and gpa of the LPI configuration table.
> >>>>> + * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
> >>>>> + * one address across all redistributors.
> >>>>> + * GICv3 spec: 6.1.2 "LPI Configuration tables"
> >>>>> + */
> >>>>> + u64 propbaser;
> >>>>> };
> >>>>>
> >>>>> struct vgic_v2_cpu_if {
> >>>>> @@ -200,6 +208,11 @@ struct vgic_cpu {
> >>>>> */
> >>>>> struct vgic_io_device rd_iodev;
> >>>>> struct vgic_io_device sgi_iodev;
> >>>>> +
> >>>>> + /* Contains the attributes and gpa of the LPI pending tables. */
> >>>>> + u64 pendbaser;
> >>>>> +
> >>>>> + bool lpis_enabled;
> >>>>> };
> >>>>>
> >>>>> int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
> >>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>>>> index bfcafbd..278bfbb 100644
> >>>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >>>>> @@ -29,6 +29,19 @@ static unsigned long extract_bytes(unsigned long data, unsigned int offset,
> >>>>> return (data >> (offset * 8)) & GENMASK_ULL(num * 8 - 1, 0);
> >>>>> }
> >>>>>
> >>>>> +/* allows updates of any half of a 64-bit register (or the whole thing) */
> >>>>> +static u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
> >>>>> + unsigned long val)
> >>>>> +{
> >>>>> + int lower = (offset & 4) * 8;
> >>>>> + int upper = lower + 8 * len - 1;
> >>>>> +
> >>>>> + reg &= ~GENMASK_ULL(upper, lower);
> >>>>> + val &= GENMASK_ULL(len * 8 - 1, 0);
> >>>>> +
> >>>>> + return reg | ((u64)val << lower);
> >>>>> +}
> >>>>> +
> >>>>> static unsined long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> >>>>> gpa_t addr, unsigned int len)
> >>>>> {
> >>>>> @@ -152,6 +165,142 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +/* We want to avoid outer shareable. */
> >>>>> +u64 vgic_sanitise_shareability(u64 field)
> >>>>> +{
> >>>>> + switch (field) {
> >>>>> + case GIC_BASER_OuterShareable:
> >>>>> + return GIC_BASER_InnerShareable;
> >>>>> + default:
> >>>>> + return field;
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> +/* Avoid any inner non-cacheable mapping. */
> >>>>> +u64 vgic_sanitise_inner_cacheability(u64 field)
> >>>>> +{
> >>>>> + switch (field) {
> >>>>> + case GIC_BASER_CACHE_nCnB:
> >>>>> + case GIC_BASER_CACHE_nC:
> >>>>> + return GIC_BASER_CACHE_RaWb;
> >>>>> + default:
> >>>>> + return field;
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> +/* Non-cacheable or same-as-inner are OK. */
> >>>>> +u64 vgic_sanitise_outer_cacheability(u64 field)
> >>>>> +{
> >>>>> + switch (field) {
> >>>>> + case GIC_BASER_CACHE_SameAsInner:
> >>>>> + case GIC_BASER_CACHE_nC:
> >>>>> + return field;
> >>>>> + default:
> >>>>> + return GIC_BASER_CACHE_nC;
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> +u64 vgic_sanitise_field(u64 reg, u64 field_mask, int field_shift,
> >>>>> + u64 (*sanitise_fn)(u64))
> >>>>> +{
> >>>>> + u64 field = (reg & field_mask) >> field_shift;
> >>>>> +
> >>>>> + field = sanitise_fn(field) << field_shift;
> >>>>> + return (reg & ~field_mask) | field;
> >>>>> +}
> >>>>> +
> >>>>> +#define PROPBASER_RES0_MASK \
> >>>>> + (GENMASK_ULL(63, 59) | GENMASK_ULL(55, 52) | GENMASK_ULL(6, 5))
> >>>>> +#define PENDBASER_RES0_MASK \
> >>>>> + (BIT_ULL(63) | GENMASK_ULL(61, 59) | GENMASK_ULL(55, 52) | \
> >>>>> + GENMASK_ULL(15, 12) | GENMASK_ULL(6, 0))
> >>>>> +
> >>>>> +static u64 vgic_sanitise_pendbaser(u64 reg)
> >>>>> +{
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_SHAREABILITY_MASK,
> >>>>> + GICR_PENDBASER_SHAREABILITY_SHIFT,
> >>>>> + vgic_sanitise_shareability);
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_INNER_CACHEABILITY_MASK,
> >>>>> + GICR_PENDBASER_INNER_CACHEABILITY_SHIFT,
> >>>>> + vgic_sanitise_inner_cacheability);
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_OUTER_CACHEABILITY_MASK,
> >>>>> + GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT,
> >>>>> + vgic_sanitise_outer_cacheability);
> >>>>> +
> >>>>> + reg &= ~PENDBASER_RES0_MASK;
> >>>>> + reg &= ~GENMASK_ULL(51, 48);
> >>>>> +
> >>>>> + return reg;
> >>>>> +}
> >>>>> +
> >>>>> +static u64 vgic_sanitise_propbaser(u64 reg)
> >>>>> +{
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_SHAREABILITY_MASK,
> >>>>> + GICR_PROPBASER_SHAREABILITY_SHIFT,
> >>>>> + vgic_sanitise_shareability);
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_INNER_CACHEABILITY_MASK,
> >>>>> + GICR_PROPBASER_INNER_CACHEABILITY_SHIFT,
> >>>>> + vgic_sanitise_inner_cacheability);
> >>>>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_OUTER_CACHEABILITY_MASK,
> >>>>> + GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT,
> >>>>> + vgic_sanitise_outer_cacheability);
> >>>>> +
> >>>>> + reg &= ~PROPBASER_RES0_MASK;
> >>>>> + reg &= ~GENMASK_ULL(51, 48);
> >>>>> + return reg;
> >>>>> +}
> >>>>> +
> >>>>> +static unsigned long vgic_mmio_read_propbase(struct kvm_vcpu *vcpu,
> >>>>> + gpa_t addr, unsigned int len)
> >>>>> +{
> >>>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >>>>> +
> >>>>> + return extract_bytes(dist->propbaser, addr & 7, len);
> >>>>> +}
> >>>>> +
> >>>>> +static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
> >>>>> + gpa_t addr, unsigned int len,
> >>>>> + unsigned long val)
> >>>>> +{
> >>>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >>>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >>>>> + u64 propbaser = dist->propbaser;
> >>>>> +
> >>>>> + /* Storing a value with LPIs already enabled is undefined */
> >>>>> + if (vgic_cpu->lpis_enabled)
> >>>>> + return;
> >>>>> +
> >>>>> + propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
> >>>>> + propbaser = vgic_sanitise_propbaser(propbaser);
> >>>>> +
> >>>>> + dist->propbaser = propbaser;
> >>>>
> >>>> Which guarantees do we have that this will always be a single atomic
> >>>> write?
> >>>
> >>> At least on arm64 - which is the only architecture this code is
> >>> compiling for at the moment - I don't see why this shouldn't be a single
> >>> write:
> >>> str x19, [x22,#3544]
> >>> is what my setup here creates.
> >>>
> >>> Do we need something stronger? Do we want to postpone this to the point
> >>> when we get arm(32) support?
> >>
> >> This is a *device*. It shouldn't be affected by whatever drives it.
> >>
> >> Now, and more to the point: the write shouldn't have to be atomic. All
> >> 64bit registers should be able to cope with 32bit writes to it, as
> >> described in the architecture spec (IHI0069C 8.1.3).
> >
> > That's not my concern. My concern is that you have two CPUs updating
> > the propbaser, once after the other, and you end up with a mix of the
> > two updates. If this C-code can ever become two 32-bit writes, for
> > example, and these can happen in parallel you can end up with something
> > like that.
> >
> > If there is not valid expectation from guest software that a real device
> > stores either one or the other value, then it's fine to leave it as is.
> >
> > Thinking about it, any sane guest would probably synchronize multiple
> > writes to this register itself?
>
> Ah, I finally clocked what you mean. Yes, we need to guarantee that the
> update to the register itself is atomic (architectural requirement).
> Which means we need to turn this in to an atomic access (atomic64_set).
Doesn't atomic64_set only work on an atomic size?
Really, I think we should just take a lock around both the read and the
write of dist->propbaser. It's not like this is going to be frequent or
in a critical path anywhere, right?
Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
