Re: [PATCH 1/3] x86/msi: consistently handle BAR mapping failures in MSI-X setup

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2022 17:59, David Vrabel wrote:
> When setting up an MSI-X vector in msix_capability_init() the error
> handling after a BAR mapping failure is different depending on whether
> the first page fails or a subsequent page. There's no reason to break
> working vectors so consistently use the later error handling
> behaviour.

"zap_on_error" can only be set when there were no working vectors yet
(msix->used_entries being zero), so I don't see what case this last
sentence describes. In fact it was the intention with "zap_on_error"
to leave previously set up vectors functional.

> The zap_on_error flag was added as part of XSA-337, beb54596cfda
> (x86/MSI-X: restrict reading of table/PBA bases from BARs), but
> appears to be unrelated to XSA-337 and is not useful because:
> 
> 1. table.first and pba.first are not used unless msix->used_vectors > 0.

This isn't true afaics. The condition around their setting up is involving
more than just ->used_vectors:

    if ( !msix->used_entries &&
         (!msi ||
          (is_hardware_domain(current->domain) &&
           (dev->domain == current->domain || dev->domain == dom_io))) )

Hence the associated "else if( !msix->table.first )" can also be taken
if msix->used_entries is zero. And in case of a failure we need to force
the error return there for DomU-s, which is achieved by clearing
msix->table.first on the error handling path you alter.

Furthermore I'd consider it bad practice to leave stale values on record.

> 2. Force disabling MSI-X in this error path is not necessary as the
>    per-vector mask is still still set.

I agree that we might be overly strict there, but to remove that
disabling you'd need to further prove that no other inconsistencies can
(later) result (this being on the safe side is where the connection to
the rest of the XSA-337 changes comes from, along with the desire to
not leave stale values around, as per above). Plus you'd want to justify
why this error path is different from others in the function where we
also disable MSI-X altogether (beyond the path you modify there's exactly
one error path where we don't, and I now wonder why I had done it like
that).

But then I may also be misunderstanding some of your intentions here.
The "consistently" in the title and the associated first sentence of the
description escape me for the moment: You're talking about things in
terms of pages, when the handling really is in terms of entries.

Jan