On 22/06/16 15:39, Andre Przywara wrote:
> 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?
I'd rather have something that is per register (after all, there is only
a handful). It will make the code readable (and it is so trivial that
the compiler will inline it anyway).
>
>>> +{
>>> + 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?
Yeah, I missed the first line. Your GIC_CACHE_PROP_ATTR() macros are
really getting in the way here. I'd prefer to see each register accessor
doing its own "sanitisation" in situ, without some fake sharing of code.
>> 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
Careful, that's about to change. And don't mind reporting *any* value,
as long as it is cacheable.
> 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.
Well, you say "redist_baser". To me, that's a redistributor, not an ITS
register. just drop these functions, and do it in each accessor.
Everything will become much more readable.
>
>>> +{
>>> + 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!
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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
