Hi Eric,
On 06/04/16 10:36, Eric Auger wrote:
> Hi Andre,
> On 03/26/2016 03:14 AM, Andre Przywara wrote:
>> Add emulation for some basic MMIO registers used in the ITS emulation.
...
>> +
>> +/*
>> + * 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.
>> + */
>> + finished = (its->cwriter != its->creadr);
> empty?
Maybe that is indeed less misleading ...
>> + its->cwriter = reg;
>> +
>> + spin_unlock(&its->lock);
> Assuming we have 2 vcpus attempting a cwriter write at the same moment I
> don't understand what does guarantee they are not going to execute the
> following loop concurrently and possibly execute vits_handle_command
> twice on the same cmd from the same its->creadr start?
So does my explanation of the algorithm in that other mail today explain
it? Or do you still see a hole in the locking scheme here?
>> +
>> + while (!finished) {
>> + int ret = kvm_read_guest(vcpu->kvm, cbaser + its->creadr,
>> + cmd_buf, 32);
>> + if (ret) {
>> + /*
>> + * Gah, we are screwed. Reset CWRITER to that command
>> + * that we have finished processing and return.
>> + */
>> + spin_lock(&its->lock);
>> + its->cwriter = its->creadr;
> don't get why do you set the queue empty?
So from a guest's point of view this is something like an ITS internal
error which shouldn't happen. So I just bail out, but leave the queue in
a consistent (read: empty) state to allow possible future commands to be
processed at best effort.
I guess this is the best we can come up with, since there is no feasible
way of communicating errors back(?)
Cheers,
Andre.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
