Re: [PATCH v2 0/2] Add support for MSI injection on Arm

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 16 Apr 2025, Julien Grall wrote:
> (+ Rahul)
> 
> On 16/04/2025 17:37, Mykyta Poturai wrote:
> > On 16.04.25 01:40, Stefano Stabellini wrote:
> > > On Mon, 14 Apr 2025, Julien Grall wrote:
> > > > Hi Mykyta,
> > > > 
> > > > On 14/04/2025 18:51, Mykyta Poturai wrote:
> > > > > This series adds the base support for MSI injection on Arm. This is
> > > > > needed to streamline virtio-pci interrupt triggering.
> > > > > 
> > > > > With this patches, MSIs can be triggered in guests by issuing the new
> > > > > DM op, inject_msi2. This op is similar to inject_msi, but it allows
> > > > > to specify the source id of the MSI.
> > > > > 
> > > > > We chose the approach of adding a new DM op instead of using the pad
> > > > > field of inject_msi because we have no clear way of distinguishing
> > > > > between set and unset pad fields. New implementations also adds flags
> > > > > field to clearly specify if the SBDF is set.
> > > > > 
> > > > > Patches were tested on QEMU with
> > > > 
> > > > [...]
> > > > 
> > > > > patches for ITS support for DomUs applied.
> > > > 
> > > > This means this series is unusable without external patches. Given this
> > > > is
> > > > adding a new DM operations, I think it would be more sensible to have
> > > > the vITS
> > > > support merged first. Then we can look at merging this series.
> > > 
> > > Hi Mykyta, just checking explicitly with you whether this series
> > > requires vgic-v3-its.c for DomUs?
> > > 
> > > If yes, how far are you from sending the relevant patches to xen-devel?
> > > How many are they?
> > 
> > Hi Stefano,
> > Yes, I am planning to send them together with VPCI MSI support after
> > SMMU patches are merged.
> > 
> > Although the DomU vits itself is only two patches.
> 
> I am assuming this is work in progress patches rather than the one you plan to
> send, correct?
> 
> Asking, because currently there are a few ASSERT(is_hardware_domain(its->d))
> to indicate where changes are likely for the guests. You seem to remove them
> without explaining why or any associated code.
> 
> While I will not ask to have a security support guest vITS from the start. I
> would like some confidence that we are going in the right direction. IOW, I
> would like to see a design document that would explain how we can achieve it.
> Some of the part to consider:
>   * Command queue
>   * LPIs
>   * 1:1 pITS <-> vITS vs one vITS to many pITS
>   * The page-tables are shared between the device and CPU. Are we ok to keep
> the doorbell writable by the CPU?
> 
> There was some discussion in the past about it (I have added Rahul because
> IIRC he was driving the discussion). So most likely, we would need the design
> to be respinned and agreed first.
> 
> Lastly, I see you seem to go down the route of exposing one vITS only. But I
> don't think your patch is sufficient to support multiple pITS (the guest
> doorbell will be mapped to a different host doorbell depending on the guest).

To add to what Julien wrote, one thing is enabling a component with some
limitations, such as lacking a given feature. That might be OK.

A different thing is lack of isolation between VMs, or introducing a new
vehicle for interference, e.g. one VM using vITS causing troubles to
another VM using vITS.

Lack of isolation and interference are not OK. We need to be very
careful and be very aware if there any constraints or limitations we are
hitting against (e.g. size of the command queue) where one VM could
cause a denial of service to another VM because it is exhausting a given
physical resource.