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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

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.)

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).

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?

Jan