Hi Marc,
On 22/06/16 15:07, Marc Zyngier wrote:
> On 17/06/16 13:08, Andre Przywara wrote:
>> 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>
>> ---
>> include/kvm/vgic/vgic.h | 13 +++++
>> include/linux/irqchip/arm-gic-v3.h | 1 +
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 112 ++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 124 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/kvm/vgic/vgic.h b/include/kvm/vgic/vgic.h
>> index e488a369..dc7f2fd 100644
>> --- a/include/kvm/vgic/vgic.h
>> +++ b/include/kvm/vgic/vgic.h
>> @@ -146,6 +146,14 @@ struct vgic_dist {
>> struct vgic_irq *spis;
>>
>> struct vgic_io_device dist_iodev;
>> +
>> + /*
>> + * Contains the address 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;
>> +
>> + /* Points to the LPI pending tables for the redistributor */
>> + u64 pendbaser;
>> +
>> + bool lpis_enabled;
>> };
>>
>> int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index bfbd707..64e8c70 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -124,6 +124,7 @@
>> #define GICR_PROPBASER_WaWb (5U << 7)
>> #define GICR_PROPBASER_RaWaWt (6U << 7)
>> #define GICR_PROPBASER_RaWaWb (7U << 7)
>> +#define GICR_PROPBASER_CACHEABILITY_SHIFT (7)
>> #define GICR_PROPBASER_CACHEABILITY_MASK (7U << 7)
>> #define GICR_PROPBASER_IDBITS_MASK (0x1f)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index c38302d..8cd7190 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 unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>> gpa_t addr, unsigned int len)
>> {
>> @@ -146,6 +159,101 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
>> return 0;
>> }
>>
>> +/* We don't have any constraints about the shareability attributes. */
>> +static void vgic_sanitise_shareability(u64 *reg)
>
> Which register is that for?
I was under the assumption that any shareability constraints would apply
to _all_ registers that use that mechanism:
PENDBASER, PROPBASER, BASER(device), BASER(collection), CBASER.
That's why this common function.
>> +{
>> +}
>
> We may not have constraints, but we don't want to let the luser go wild
> either... ;-) The notion of outer sharable is pretty useless on ARMv8,
> and I'd rather not pretend it is supported in any way. Can you please
> make this support all values but OS?
Sure, actually I was hoping for your guidance here, since I couldn't
really wrap my mind around shareability in this virtualisation context.
>> +
>> +#define GIC_CACHE_PROP_ATTR(x) ((x) >> GICR_PROPBASER_CACHEABILITY_SHIFT)
>> +#define GIC_CACHE_PROP_MASK \
>> + ((u64)(GIC_CACHE_PROP_ATTR(GICR_PROPBASER_CACHEABILITY_MASK)))
>> +
>> +static void vgic_sanitise_outer_cacheability(u64 *reg, int reg_shift)
>
> I assume this is for GICR_PROPBASER?
Again this is for all registers as listed above. Do we need separate
constraints for different registers?
>> +{
>> + switch (*reg >> reg_shift) {
>
> What about the upper bits?
Pah ;-)
Will fix it ...
>> + case GIC_CACHE_PROP_ATTR(GICR_PROPBASER_nC):
>> + *reg &= ~(GIC_CACHE_PROP_MASK << reg_shift);
>> + *reg |= GIC_CACHE_PROP_ATTR(GICR_PROPBASER_WaWb) << reg_shift;
>> + break;
>
> Why would you force things to be cacheable? Specially outer cacheable?
To be honest, I couldn't really make out what "outer cacheability"
actually means in the context of a guest having user space memory mapped.
>
> Frankly, I'd rather stick to either 000 (same as inner cacheability) and
> 001 (outer non-cacheable).
Sure.
>
>> + default:
>> + /* We are fine with the other attributes. */
>> + break;
>> + }
>> +}
>> +
>> +static void vgic_sanitise_inner_cacheability(u64 *reg, int reg_shift)
>> +{
>> + switch (*reg >> reg_shift) {
>> + case GIC_CACHE_PROP_ATTR(GICR_PROPBASER_nCnB):
>> + case GIC_CACHE_PROP_ATTR(GICR_PROPBASER_nC):
>> + *reg &= ~(GIC_CACHE_PROP_MASK << reg_shift);
>> + *reg |= GIC_CACHE_PROP_ATTR(GICR_PROPBASER_WaWb) << reg_shift;
>> + break;
>> + default:
>> + /* We are fine with the other attributes. */
>> + break;
>> + }
>
> I fail to see the difference with the previous function,
The meaning of 000 is different: for inner cacheability it means Device,
for outer it means "same as inner". So we need two functions here to
differentiate, don't we? Unless our constraints happen to be the same
for the two (by coincidence)?
Also I think you just requested different constraints for outer and
inner cacheability? Or did I miss something?
> and it has the
> same bugs. As for the cacheability, we definitely want to enforce it, so
> you should make both Device-nGnRnE and Normal-nC unsupported.
Isn't that what the function does? If the value is 000 or 001, it
changes it to WaWb, otherwise it keeps the value. At least that was what
I had in mind. WaWb is what the Linux ITS driver prefers, so this serves
as some sensible value here (I hope). It's a bit weird that these bits
are not a bitfield, so the ITS cannot report back a set of supported
attributes easily at once.
>> +}
>> +
>> +static void vgic_sanitise_redist_baser(u64 *reg)
>
> Which base register? Please name them entierely (gicr_propbaser) so that
> we know what you are messing with.
This is for BASER (both for collections and devices). It's a bit
unfortunate that this one just consists of the stem which the other
registers also share, so it's a bit hard to differentiate without
inventing a new name. I can add a comment, though.
>> +{
>> + vgic_sanitise_shareability(reg);
>> + vgic_sanitise_inner_cacheability(reg,
>> + GICR_PROPBASER_CACHEABILITY_SHIFT);
>> + vgic_sanitise_outer_cacheability(reg, 56);
>
> Care to define this bit field?
Sure.
>
>> +}
>> +
>> +#define PROPBASER_RES0_MASK 0xf8f0000000000060
>> +#define PENDBASER_RES0_MASK 0xb8f000000000f07f
>
> Please build those using GENMASK_ULL. I can't process long hex values,
> but I can read something that matches the spec.
OK.
Thanks for having a look!
Andre.
>> +
>> +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;
>> +
>> + /* Storing a value with LPIs already enabled is undefined */
>> + if (vgic_cpu->lpis_enabled)
>> + return;
>> +
>> + dist->propbaser = update_64bit_reg(dist->propbaser, addr & 4, len, val);
>> + dist->propbaser &= ~PROPBASER_RES0_MASK;
>> + vgic_sanitise_redist_baser(&dist->propbaser);
>> +}
>> +
>> +static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
>> + gpa_t addr, unsigned int len)
>> +{
>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> +
>> + return extract_bytes(vgic_cpu->pendbaser, addr & 7, len);
>> +}
>> +
>> +static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>> + gpa_t addr, unsigned int len,
>> + unsigned long val)
>> +{
>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> +
>> + /* Storing a value with LPIs already enabled is undefined */
>> + if (vgic_cpu->lpis_enabled)
>> + return;
>> +
>> + vgic_cpu->pendbaser = update_64bit_reg(vgic_cpu->pendbaser,
>> + addr & 4, len, val);
>> + vgic_cpu->pendbaser &= ~PENDBASER_RES0_MASK;
>> + vgic_sanitise_redist_baser(&vgic_cpu->pendbaser);
>> +}
>> +
>> /*
>> * The GICv3 per-IRQ registers are split to control PPIs and SGIs in the
>> * redistributors, while SPIs are covered by registers in the distributor
>> @@ -226,10 +334,10 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>> vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
>> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
>> + vgic_mmio_read_propbase, vgic_mmio_write_propbase, 8,
>> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_LENGTH(GICR_PENDBASER,
>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
>> + vgic_mmio_read_pendbase, vgic_mmio_write_pendbase, 8,
>> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>> REGISTER_DESC_WITH_LENGTH(GICR_IDREGS,
>> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
>>
>
> Thanks,
>
> M.
>
--
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
