Re: [PATCH v2 09/15] KVM: arm64: implement basic ITS register handlers

[Andre Przywara <andre.przywara@arm.com>]

Hi Eric,

....

>> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
>> index 659dd39..b498f06 100644
>> --- a/virt/kvm/arm/its-emul.c
>> +++ b/virt/kvm/arm/its-emul.c
>> @@ -32,10 +32,62 @@
>>  #include "vgic.h"
>>  #include "its-emul.h"
>>
>> +#define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL)
>> +
>> +/* The distributor lock is held by the VGIC MMIO handler. */
>>  static bool handle_mmio_misc_gits(struct kvm_vcpu *vcpu,
>>                                 struct kvm_exit_mmio *mmio,
>>                                 phys_addr_t offset)
>>  {
>> +     struct vgic_its *its = &vcpu->kvm->arch.vgic.its;
>> +     u32 reg;
>> +     bool was_enabled;
>> +
>> +     switch (offset & ~3) {
>> +     case 0x00:              /* GITS_CTLR */
>> +             /* We never defer any command execution. */
>> +             reg = GITS_CTLR_QUIESCENT;
>> +             if (its->enabled)
>> +                     reg |= GITS_CTLR_ENABLE;
>> +             was_enabled = its->enabled;
>> +             vgic_reg_access(mmio, &reg, offset & 3,
>> +                             ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
>> +             its->enabled = !!(reg & GITS_CTLR_ENABLE);
>> +             return !was_enabled && its->enabled;
>> +     case 0x04:              /* GITS_IIDR */
>> +             reg = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
>> +             vgic_reg_access(mmio, &reg, offset & 3,
>> +                             ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
>> +             break;
>> +     case 0x08:              /* GITS_TYPER */
>> +             /*
>> +              * We use linear CPU numbers for redistributor addressing,
>> +              * so GITS_TYPER.PTA is 0.
>> +              * To avoid memory waste on the guest side, we keep the
>> +              * number of IDBits and DevBits low for the time being.
>> +              * This could later be made configurable by userland.
>> +              * Since we have all collections in linked list, we claim
>> +              * that we can hold all of the collection tables in our
>> +              * own memory and that the ITT entry size is 1 byte (the
>> +              * smallest possible one).
>> +              */
>> +             reg = GITS_TYPER_PLPIS;
>> +             reg |= 0xff << GITS_TYPER_HWCOLLCNT_SHIFT;
>> +             reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>> +             reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
>> +             vgic_reg_access(mmio, &reg, offset & 3,
>> +                             ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
>> +             break;
>> +     case 0x0c:
>> +             /* The upper 32bits of TYPER are all 0 for the time being.
>> +              * Should we need more than 256 collections, we can enable
>> +              * some bits in here.
>> +              */
>> +             vgic_reg_access(mmio, NULL, offset & 3,
>> +                             ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
>> +             break;
>> +     }
>> +
>>       return false;
>>  }
>>
>> @@ -43,20 +95,142 @@ static bool handle_mmio_gits_idregs(struct kvm_vcpu *vcpu,
>>                                   struct kvm_exit_mmio *mmio,
>>                                   phys_addr_t offset)
>>  {
>> +     u32 reg = 0;
>> +     int idreg = (offset & ~3) + GITS_IDREGS_BASE;
>> +
>> +     switch (idreg) {
>> +     case GITS_PIDR2:
>> +             reg = GIC_PIDR2_ARCH_GICv3;
>> +             break;
>> +     case GITS_PIDR4:
>> +             /* This is a 64K software visible page */
>> +             reg = 0x40;
>> +             break;
>> +     /* Those are the ID registers for (any) GIC. */
>> +     case GITS_CIDR0:
>> +             reg = 0x0d;
>> +             break;
>> +     case GITS_CIDR1:
>> +             reg = 0xf0;
>> +             break;
>> +     case GITS_CIDR2:
>> +             reg = 0x05;
>> +             break;
>> +     case GITS_CIDR3:
>> +             reg = 0xb1;
>> +             break;
>> +     }
>> +     vgic_reg_access(mmio, &reg, offset & 3,
>> +                     ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
>>       return false;
>>  }
>>
>> +static int vits_handle_command(struct kvm_vcpu *vcpu, u64 *its_cmd)
>> +{
>> +     return -ENODEV;
>> +}
>> +
>>  static bool handle_mmio_gits_cbaser(struct kvm_vcpu *vcpu,
>>                                   struct kvm_exit_mmio *mmio,
>>                                   phys_addr_t offset)
>>  {
>> +     struct vgic_its *its = &vcpu->kvm->arch.vgic.its;
>> +     int mode = ACCESS_READ_VALUE;
>> +
>> +     mode |= its->enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE;
>> +
>> +     vgic_handle_base_register(vcpu, mmio, offset, &its->cbaser, mode);
>> +
>> +     /* Writing CBASER resets the read pointer. */
>> +     if (mmio->is_write)
>> +             its->creadr = 0;
>> +
>>       return false;
>>  }
>>
>> +static int its_cmd_buffer_size(struct kvm *kvm)
>> +{
>> +     struct vgic_its *its = &kvm->arch.vgic.its;
>> +
>> +     return ((its->cbaser & 0xff) + 1) << 12;
>> +}
>> +
>> +static gpa_t its_cmd_buffer_base(struct kvm *kvm)
>> +{
>> +     struct vgic_its *its = &kvm->arch.vgic.its;
>> +
>> +     return BASER_BASE_ADDRESS(its->cbaser);
>> +}
>> +
>> +/*
>> + * By writing to CWRITER the guest announces new commands to be processed.
>> + * Since we cannot read from guest memory inside the ITS spinlock, we
>> + * iterate over the command buffer (with the lock dropped) until the read
>> + * pointer matches the write pointer. Other VCPUs writing this register in the
>> + * meantime will just update the write pointer, leaving the command
>> + * processing to the first instance of the function.
>> + */
> I am lost, isn't the dist lock hold by vgic_handle_mmio_access before
> calling call_range_handler/range->handle_mmio ?

Indeed, I was so focussed on the ITS lock...

> if confirmed we need to move the command enumeration + execution
> somewhere else.

I think we get away with dropping the dist lock after doing the actual
register access. Just before returning we then take it again to make the
caller happy.
I will give this a try. Thanks for spotting this!

> Besides that piece of code may deserve to be
> encaspulated in a function. what do you think?

Makes some sense, yes.

Cheers,
André