Re: [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded

[James Morse <james.morse@arm.com>]

Hi Marc,

On 09/04/2020 09:27, Marc Zyngier wrote:
> On Wed, 8 Apr 2020 12:16:01 +0100
> James Morse <james.morse@arm.com> wrote:
>> On 08/04/2020 11:07, Marc Zyngier wrote:
>>> I don't fully agree with the analysis, Remember we are looking at the
>>> state of the physical interrupt associated with a virtual interrupt, so
>>> the vcpu doesn't quite make sense here if it isn't loaded.
>>>
>>> What does it mean to look at the HW timer when we are not in the right
>>> context? For all we know, it is completely random (the only guarantee
>>> we have is that it is disabled, actually).  
>>
>>> My gut feeling is that this is another instance where we should provide
>>> specific userspace accessors that would only deal with the virtual
>>> state, and leave anything that deals with the physical state of the
>>> interrupt to be exercised only by the guest.  
>>
>>> Does it make sense?  
>>
>> Broadly, yes. Specifically ... I'm not familiar enough with this code to work out where
>> such a change should go!
>>
>> ~20 mins of grepping later~
>>
>> Remove REGISTER_DESC_WITH_LENGTH() so that uaccess helpers have to be provided, and forbid
>> NULL for the ur/uw values in REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED()...?
> 
> I'd suggest something like this (untested, of course):

[...]

>> Or if that is too invasive, something like, (totally, untested):
>> ----------------%<----------------
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 97fb2a40e6ba..30ae5f29e429 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -113,10 +113,11 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
>>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>
>>                 raw_spin_lock_irqsave(&irq->irq_lock, flags);
>> -               if (vgic_irq_is_mapped_level(irq)) {
>> +               if (kvm_running_vcpu() && vgic_irq_is_mapped_level(irq)) {
>>                         bool was_high = irq->line_level;
>>
>>                         /*
>> +                        * Unless we are running due to a user-space access,
>>                          * We need to update the state of the interrupt because
>>                          * the guest might have changed the state of the device
>>                          * while the interrupt was disabled at the VGIC level.
>> ----------------%<----------------
> 
> Huh, nice try! ;-) Unfortunately, I think there is more than the enable
> register that suffers from a similar problem (see how the pending
> register write is also accessing the HW state, even if accessed from
> userspace).

Yeah, I'd expect to play wack-a-mole if I actually tested it. It was just the smallest,
er, hack I could get my head round given your explanation.


I've blindly tested your version, it works for me on a gicv2 machine:
Tested-by: James Morse <james.morse@arm.com>

I'll test on the gicv3 espressobin that I originally saw this on with rc1 on Tuesday.

Do you want me to post it back to you as a tested patch? You can judge whether I
understand it from the commit message... (I'd need your Signed-off-by...)

Have a good extended weekend!
Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm