Re: [PATCH 33/55] KVM: arm64: vgic: Handle ITS related GICv3 redistributor registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux