Re: [PATCH v3] vpci/msix: handle accesses adjacent to the MSI-X table

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

On Thu, Mar 23, 2023 at 11:58:26AM +0100, Jan Beulich wrote:
> On 23.03.2023 11:06, Roger Pau Monné wrote:
> > On Thu, Mar 23, 2023 at 09:02:31AM +0100, Jan Beulich wrote:
> >> On 22.03.2023 18:08, Roger Pau Monné wrote:
> >>> On Wed, Mar 22, 2023 at 06:05:55PM +0100, Roger Pau Monné wrote:
> >>>> Not sure whether we should consider an access from when the BAR was
> >>>> actually enabled by a different thread could still continue while on
> >>>> another thread the BAR has been disabled and enabled again (and thus
> >>>> the mapping removed).  It's a theoretical race, so I guess I will look
> >>>> into making sure we cannot hit it.
> >>>
> >>> Hm, maybe it doesn't matter much because such kind of trace could only
> >>> be triggered by the hardware domain anyway, and it has plenty of other
> >>> ways to mess with Xen.
> >>
> >> Preferably we should get things to use proper locking. If that turns out
> >> too hard, properly justified such an exception for Dom0 might be
> >> acceptable.
> > 
> > Right, one idea I have would be to use map_pages_to_xen() in
> > vpci_make_msix_hole() to remap any existing virtual addresses to point
> > to the new physical addresses, so that there's no unmap.  I think this
> > would be fine because map_pages_to_xen() does an atomic write of the
> > PTE, but not sure if it's abusing the interface.  Such remap would
> > avoid taking the vpci lock for the whole duration of the access.
> 
> Hmm, no, I'm afraid that won't suffice for another reason: I think these
> mappings need properly removing, and already at the time when memory
> decoding is turned off (hence a potential new address isn't known yet).
> Otherwise we leave too much of a risk that these mappings may actually
> be used when the address range has already been re-purposed (even if I
> think with how the code is written right now there wouldn't be any use
> in reality, but relying on that going forward seems overly fragile to
> me).

Keep in mind it's only dom0 that requires the mappings to be updated.
Hence dom0 playing tricks to read from such addresses when memory
decoding has been disabled seems like an unnecessary hassle, it likely
has plenty of other ways to do so that don't involve relying on races
between memory decoding and BAR accesses.

For domUs the address of the BARs on the real device can never change
while being used by the domU, and hence the mappings don't need
removing on memory decoding toggle.

> I can be convinced otherwise, so let me also consider that case: One
> prereq would imo be that bar->enabled be cleared before memory decoding
> is turned off and set only after it was turned back on. (Unless again
> we want to be forgiving with Dom0.) (Looking at modify_decoding() I
> also wonder why we invoke pci_check_bar() even when we're about to clear
> {,rom_}enabled.)

Hm, I see what you mean: it doesn't make sense to keep the BAR mapped
into the guest physmap if we then turn off memory decoding (or ROM BAR
enabling).  We need to prevent ROM BAR and/or memory decoding from
being turned off if pci_check_bar() returns false for any BAR of the
device.  Let's keep this separate from the issue at hand.

> 
> As to (ab)using map_pages_to_xen() - I wouldn't be very happy about
> playing with ioremap()-ed regions behind vmap.c's back, but I agree it
> looks like technically it would work. In case of future issues we could
> introduce something like vremap() to fiddle with an already mapped
> region (and then perhaps also only changing addresses, but not
> attributes).

I have such a version, which obviously looks very similar to v3.  Let
me try to finalize the comments and see how it looks overall.

> Another option to avoid holding the lock across the entire access might
> be for get_table() to actually acquire a ref on the mapping, with new
> mappings starting out with a single ref, for vpci_make_msix_hole() to
> drop (in place of explicitly unmapping). But that would require further
> precautions to prevent stale mappings to have new refs obtained on
> them, so might easily be getting too complicated for the purpose. As
> would (likely) any other attempt at serializing this properly without
> holding the lock. Which gets to ask: Would it really that bad to hold
> the lock across the entire access? Or could we introduce a 2nd (per-
> device) lock just for this purpose, which might then be an r/w one?

Adding a refcount or another lock seems like too much for my taste, I
would start by just using the vpci->lock for the whole access.

Thanks, Roger.