Re: [PATCH 2/2] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Nov 14, 2022 at 07:39:48PM +0000, Andrew Cooper wrote:
> On 14/11/2022 19:20, Marek Marczykowski-Górecki wrote:
> > The /dev/mem is used for two purposes:
> >  - reading PCI_MSIX_ENTRY_CTRL_MASKBIT
> >  - reading Pending Bit Array (PBA)
> >
> > The first one was originally done because when Xen did not send all
> > vector ctrl writes to the device model, so QEMU might have outdated old
> > register value. This has been changed in Xen, so QEMU can now use its
> > cached value of the register instead.

I should have noted that "has been changed in Xen" isn't fully accurate
(yet). It refers to this patch:
https://lore.kernel.org/xen-devel/20221114192100.1539267-2-marmarek@xxxxxxxxxxxxxxxxxxxxxx/T/#u

> > The Pending Bit Array (PBA) handling is for the case where it lives on
> > the same page as the MSI-X table itself. Xen has been extended to handle
> > this case too (as well as other registers that may live on those pages),
> > so QEMU handling is not necessary anymore.
> >
> > Removing /dev/mem access is useful to work within stubdomain, and
> > necessary when dom0 kernel runs in lockdown mode.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> The commit message ought to go further.  Using /dev/mem like this is
> buggy anyway, because it is trapped and emulated by Xen in whatever
> context Qemu is running.  Qemu cannot get the actual hardware value, and
> even if it could, it would be racy with transient operations needing to
> mask the vector.
> 
> i.e. it's not just nice-to-remote - it's fixing real corner cases.

Good point, I'll extend it.
But for this to work, the Xen patch needs to go in first (which won't
happen for 4.17). And also, upstream QEMU probably needs some way to
detect whether Xen has the change or not - to work with older versions
too.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature