Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Aug 24, 2020 at 06:07:28PM +0200, Jan Beulich wrote:
> On 24.08.2020 16:44, Roger Pau Monné wrote:
> > On Mon, Aug 24, 2020 at 04:06:31PM +0200, Jan Beulich wrote:
> >> On 12.08.2020 14:47, Roger Pau Monne wrote:
> >>> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> >>> and instead use the newly introduced EOI callback mechanism in order
> >>> to register a callback for MSI vectors injected from passed through
> >>> devices.
> >>
> >> In patch 2 you merely invoke the callback when the EOI arrived,
> >> but you don't clear the callback (unless I've missed something).
> >> Here you register the callback once per triggering of the IRQ,
> >> without clearing it from the callback itself either.
> > 
> > It gets cleared on the next call to vlapic_set_irq_callback, or set to
> > a new callback value if the passed callback is not NULL.
> > 
> >> Why is it
> >> correct / safe to keep the callback registered?
> > 
> > The next vector injected is going to clear it, so should be safe, as
> > no vector can be injected without calling vlapic_set_irq_callback.
> 
> But what about duplicate EOIs, even if just by bug or erratum?
> I notice, for example, that VMX'es VMEXIT handler directly
> calls vlapic_handle_EOI().

Yes, but that's expected and required, because when using VMX LAPIC
virtualization you get an explicit vmexit (EXIT_REASON_EOI_INDUCED) on
EOI of requested vectors by using the EOI exit bitmap
(vmx_update_eoi_exit_bitmap).

> I'd find it more safe if an
> unexpected EOI didn't get any callback invoked.

OK, the callback can be certainly cleared in vlapic_handle_EOI.

> 
> >> What about
> >> conflicting callbacks for shared vectors?
> > 
> > In this callback model for vlapic only the last injected vector
> > callback would get executed. It's my understanding that lapic
> > vectors cannot be safely shared unless there's a higher level
> > interrupt controller (ie: an IO-APIC) that does the sharing.
> 
> As said on a different, but pretty recent thread: I do think
> sharing is possible if devices and drivers meet certain criteria
> (in particular have a way to determine which of the devices
> actually require service). It's just not something one would
> normally do. But iirc such a model was used in good old DOS to
> overcome the 15 IRQs limit (of which quite a few had fixed
> purposes, so for add-in devices there were only very few left).

So my callback model for the vIO-APIC/vPIC is different from the one
used for the vlapic. In that case callers must use a helper to
register/remove a callback for GSIs, and a single GSI can have
multiple callbacks attached.

Such interface works well with the vIO-APIC/vPIC because interrupts
from devices are statically assigned a GSI, and you only need to
register the callback when the device is instantiated.

For vlapic OTOH this would be more complex, as a guest might decide to
change MSI vectors constantly and thus require a non-trivial amount of
registrations/removals of callbacks.

I was assuming that any sane OS wouldn't share a lapic vector for
multiple devices, and that hence we could get away without having to
implement multiple per-vector callback support.

Would you be fine with clearing the callback in vlapic_handle_EOI and
then vlapic_set_irq_callback complaining if it finds there's a
previous callback different than the one provided already set for the
to be injected vector?

Thanks, Roger.