Re: [PATCH for-4.14 6/8] x86/vpt: fix injection to remote vCPU

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2020 19:14, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 05:12:17PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>>      case PTSRC_ioapic:
>>>          pt_vector = hvm_ioapic_assert(v->domain, irq, level);
>>> -        if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>>> -        {
>>> -            pt_vector = -1;
>>> -            if ( level )
>>> +        if ( pt_vector < 0 )
>>> +            return pt_vector;
>>> +
>>> +        break;
>>> +    }
>>> +
>>> +    ASSERT(pt_vector >= 0);
>>> +    if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>>> +    {
>>> +        time_cb *cb = NULL;
>>> +        void *cb_priv;
>>> +
>>> +        /*
>>> +         * Vector has been injected to a different vCPU, call pt_irq_fired 
>>> and
>>> +         * execute the callback, since the destination vCPU(s) won't call
>>> +         * pt_intr_post for it.
>>
>> ... this isn't the only reason to come here. Beyond what the comment
>> says there is the hvm_domain_use_pirq() check in assert_gsi() which
>> would similarly result in the IRR bit not observed set here. At the
>> very least these cases want mentioning; I have to admit that I'm not
>> entirely clear yet whether your handling is correct for both, or
>> whether the information needs to be propagated into here.
> 
> I always forget about that weird pirq stuff (and I'm refraining from
> using other adjectives) that we have for HVM.
> 
> AFAICT vpt is already broken when trying to inject interrupts
> generated from it over an event channel. hvm_ioapic_assert will return
> whatever garbage is in the IO-APIC entry, which will likely not be
> initialized because the GSI is routed over an event channel.
> 
> I really have no idea what hvm_ioapic_assert should return in that
> case, the event channel callback vector maybe?
> 
> Maybe just returning -1 would be fine, a guest using this routing of
> pirqs over event channels shouldn't be using any of the emulated
> timers, and hence vpt is not required to be functional in that case?

I would guess(!) that -1 ought to be fine. But this whole thing
escapes me as well, so let's ask Stefano, who iirc was who
introduced this.

Jan