[PATCH v4 06/12] KVM: arm64: implement basic ITS register handlers

[Andre Przywara]

Hi,

I addressed some comments of yours in v5, just some rationale for the
others (sorry for the late reply on these!):

...

>>>> +static int vgic_mmio_read_its_typer(struct kvm_vcpu *vcpu,
>>>> +				    struct kvm_io_device *this,
>>>> +				    gpa_t addr, int len, void *val)
>>>> +{
>>>> +	u64 reg = GITS_TYPER_PLPIS;
>>>> +
>>>> +	/*
>>>> +	 * 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).
>>>
>>> All of this is going to bite us when we want to implement migration,
>>> specially the HW collection bit.
>>
>> How so? Clearly we keep the number of VCPUs constant, so everything
>> guest visible stays the same even upon migrating to a much different
>> host? Or are we talking about different things here?
> 
> Let me rephrase this: How are you going to address HW collections from
> userspace in order to dump them? Short of having memory tables, you cannot.

I was just hoping for addressing first things first. Having the
collection table entirely in the kernel makes things easier for now.

How is that save/restore story covered by a hardware ITS? Is it just
ignored since it's not needed/used there?

That being said I would be happy to change this when we get migration
support for the ITS register state and we have agreed on an API for
userland ITS save/restore.

>>>
>>>> +	 */
>>>> +	reg |= 0xff << GITS_TYPER_HWCOLLCNT_SHIFT;
>>>> +	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>>>> +	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
>>>> +
>>>> +	write_mask64(reg, addr & 7, len, val);
>>>> +
>>>> +	return 0;
>>>> +}

.....

>>>> +static int vgic_mmio_read_its_idregs(struct kvm_vcpu *vcpu,
>>>> +				     struct kvm_io_device *this,
>>>> +				     gpa_t addr, int len, void *val)
>>>> +{
>>>> +	struct vgic_io_device *iodev = container_of(this,
>>>> +						    struct vgic_io_device, dev);
>>>> +	u32 reg = 0;
>>>> +	int idreg = (addr & ~3) - iodev->base_addr + GITS_IDREGS_BASE;
>>>> +
>>>> +	switch (idreg) {
>>>> +	case GITS_PIDR2:
>>>> +		reg = GIC_PIDR2_ARCH_GICv3;
>>>
>>> Are we leaving the lowest 4 bits to zero?
>>
>> I couldn't stuff "42" in 4 bits, so: yes ;-)
> 
> The 4 bottom bits are "Implementation Defined", so you could fit
> something in it if you wanted.
> 
>>
>>>> +		break;
>>>> +	case GITS_PIDR4:
>>>> +		/* This is a 64K software visible page */
>>>> +		reg = 0x40;
>>>
>>> Same question.
>>>
>>> Also, how about all the others PIDR registers?
>>
>> I guess I was halfway undecided between going with the pure
>> architectural requirements (which only mandate bits 7:4 in PIDR2) and
>> complying with the recommendation later in the spec.
>> So I will just fill PIDR0, PIDR2 and the rest of PIDR2 as well.
>>
>>>> +		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;
>>>> +	}
>>>
>>> Given that these values are directly taken from the architecture, and
>>> seem common to the whole GICv3 architecture when implemented by ARM, we
>>> could have a common handler for the whole GICv3 implementatuin. Not a
>>> bit deal though.

I tried it, but it turned out to be more involved than anticipated, so I
shelved it for now. I can make a patch on top later.

>>
>> Agreed.
>>
>>>> +
>>>> +	write_mask32(reg, addr & 3, len, val);
>>>> +
>>>> +	return 0;
>>>> +}

....

>>>> +/*
>>>> + * 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.
>>>> + */
>>>> +static int vgic_mmio_write_its_cwriter(struct kvm_vcpu *vcpu,
>>>> +				       struct kvm_io_device *this,
>>>> +				       gpa_t addr, int len, const void *val)
>>>> +{
>>>> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +	struct vgic_its *its = &dist->its;
>>>> +	gpa_t cbaser = its_cmd_buffer_base(vcpu->kvm);
>>>> +	u64 cmd_buf[4];
>>>> +	u32 reg;
>>>> +	bool finished;
>>>> +
>>>> +	reg = mask64(its->cwriter & 0xfffe0, addr & 7, len, val);
>>>> +	reg &= 0xfffe0;
>>>> +	if (reg > its_cmd_buffer_size(vcpu->kvm))
>>>> +		return 0;
>>>> +
>>>> +	spin_lock(&its->lock);
>>>> +
>>>> +	/*
>>>> +	 * If there is still another VCPU handling commands, let this
>>>> +	 * one pick up the new CWRITER and process "our" new commands as well.
>>>> +	 */
>>>
>>> How do you detect that condition? All I see is a massive race here, with
>>> two threads processing the queue in parallel, possibly corrupting each
>>> other's data.
>>>
>>> Please explain why you think this is safe.
>>
>> OK, here you go: Actually we are handling the command queue
>> synchronously: The first writer to CWRITER will end up here and will
>> iterate over all commands below. From the guests point of view it will
>> take some time to do the CWRITER MMIO write, but upon the writel
>> returning the queue is actually already empty again.
>> That means that any _sane_ guest will _never_ trigger the case where two
>> threads/VCPUs are handling the queue, because a concurrent write to
>> CWRITER without a lock in the guest would be broken by design.
> 
> That feels very fragile, and you're relying on a well behaved guest.

No, I don't rely on it, as mentioned in the next sentence.
I just don't optimize for that case, because in reality it should never
happen - apart from a malicious guest. Let me think about the security
implications, though (whether a misbehaved guest could hog a physical CPU).

>> However I am not sure we can rely on this, so I added this precaution
>> scheme:

>> The first writer takes our ITS (emulation) lock and examines cwriter and
>> creadr to see if they are equal. If they are, then the queue is
>> currently empty, which means we are the first one and need to take care
>> of the commands. We update our copy of cwriter (now marking the queue as
>> non-empty) and drop the lock. As we kept the queue-was-empty status in a
>> local variable, we now can start iterating through the queue. We only
>> take the lock briefly when really needed in this process (for instance
>> when one command has been processed and creadr is incremented).
>>
>> If now a second VCPU writes CWRITER, we also take the lock and compare
>> creadr and cwriter. Now they are different, because the first thread is
>> still busy with handling the commands and hasn't finished yet.
>> So we set our local "finished" to true. Also we update cwriter to the
>> new value, then drop the lock. The while loop below will not be entered
>> in this thread and we return to the guest.
>> Now the first thread has handled another command, takes the lock to
>> increase creadr and compares it with cwriter. Without the second writer
>> it may have been finished already, but now cwriter has grown, so it will
>> just carry on with handling the commands.
>>
>> A bit like someone washing the dishes while another person adds some
>> more dirty plates to the pile ;-)
> 
> I'm sorry, this feels incredibly complicated, and will make actual
> debugging/tracing a nightmare. It also penalize the first vcpu that gets
> to submitting a command, leading to scheduling imbalances.

Well, as mentioned that would never happen apart from that
malicious/misbehaved guest case - so I don't care so much about
scheduling imbalances _for the guest_. As said above I have to think
about how this affects the host cores, though.

And frankly I don't see how this is overly complicated (in the context
of SMP locking schemes in general) - apart from me explaining it badly
above. It's just a bit tricky, but guarantees a strict order of command
processing.
On another approach we could just qualify a concurrent access as illegal
and ignore it - on real hardware you would have no guarantee for this to
work either - issuing two CWRITER MMIO writes at the same time would
just go bonkers.

> Why can't you simply turn the ITS lock into a mutex and keep it held,
> processing each batch in the context of the vcpu that is writing to
> GITC_CWRITER?

Frankly I spent quite some time to make this locking more fine grained
after some comments on earlier revisions - and I think it's better now,
since we don't hog all of the ITS data structures during the command
processing. Pessimistically one core could issue a big number of
commands, which would block the whole ITS.
Instead MSI injections happening in parallel now have a good chance of
iterating our lists and tables in between. This should improve scalability.

Thanks,
Andre.