On Tue, May 07, 2024 at 02:44:02PM +0200, Marek Marczykowski-Górecki wrote:
> Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
> on the same page as MSI-X table. Device model (especially one in
> stubdomain) cannot really handle those, as direct writes to that page is
> refused (page is on the mmio_ro_ranges list). Instead, extend
> msixtbl_mmio_ops to handle such accesses too.
>
> Doing this, requires correlating read/write location with guest
> MSI-X table address. Since QEMU doesn't map MSI-X table to the guest,
> it requires msixtbl_entry->gtable, which is HVM-only. Similar feature
> for PV would need to be done separately.
>
> This will be also used to read Pending Bit Array, if it lives on the same
> page, making QEMU not needing /dev/mem access at all (especially helpful
> with lockdown enabled in dom0). If PBA lives on another page, QEMU will
> map it to the guest directly.
> If PBA lives on the same page, discard writes and log a message.
> Technically, writes outside of PBA could be allowed, but at this moment
> the precise location of PBA isn't saved, and also no known device abuses
> the spec in this way (at least yet).
>
> To access those registers, msixtbl_mmio_ops need the relevant page
> mapped. MSI handling already has infrastructure for that, using fixmap,
> so try to map first/last page of the MSI-X table (if necessary) and save
> their fixmap indexes. Note that msix_get_fixmap() does reference
> counting and reuses existing mapping, so just call it directly, even if
> the page was mapped before. Also, it uses a specific range of fixmap
> indexes which doesn't include 0, so use 0 as default ("not mapped")
> value - which simplifies code a bit.
>
> Based on assumption that all MSI-X page accesses are handled by Xen, do
> not forward adjacent accesses to other hypothetical ioreq servers, even
> if the access wasn't handled for some reason (failure to map pages etc).
> Relevant places log a message about that already.
>
> Signed-off-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
Thanks, just a couple of minor comments, I think the only relevant one
is that you can drop ADJACENT_DONT_HANDLE unless there's something
I'm missing. The rest are mostly cosmetic, but if you have to respin
and agree with them might be worth addressing.
Sorry for giving this feedback so late in the process, I should have
attempted to review earlier versions.
> ---
> Changes in v7:
> - simplify logic based on assumption that all access to MSI-X pages are
> handled by Xen (Roger)
> - move calling adjacent_handle() into adjacent_{read,write}() (Roger)
> - move range check into msixtbl_addr_to_desc() (Roger)
> - fix off-by-one when initializing adj_access_idx[ADJ_IDX_LAST] (Roger)
> - no longer distinguish between unhandled write due to PBA nearby and
> other reasons
> - add missing break after ASSERT_UNREACHABLE (Jan)
> Changes in v6:
> - use MSIX_CHECK_WARN macro
> - extend assert on fixmap_idx
> - add break in default label, after ASSERT_UNREACHABLE(), and move
> setting default there
> - style fixes
> Changes in v5:
> - style fixes
> - include GCC version in the commit message
> - warn only once (per domain, per device) about failed adjacent access
> Changes in v4:
> - drop same_page parameter of msixtbl_find_entry(), distinguish two
> cases in relevant callers
> - rename adj_access_table_idx to adj_access_idx
> - code style fixes
> - drop alignment check in adjacent_{read,write}() - all callers already
> have it earlier
> - delay mapping first/last MSI-X pages until preparing device for a
> passthrough
> v3:
> - merge handling into msixtbl_mmio_ops
> - extend commit message
> v2:
> - adjust commit message
> - pass struct domain to msixtbl_page_handler_get_hwaddr()
> - reduce local variables used only once
> - log a warning if write is forbidden if MSI-X and PBA lives on the same
> page
> - do not passthrough unaligned accesses
> - handle accesses both before and after MSI-X table
> ---
> xen/arch/x86/hvm/vmsi.c | 205 ++++++++++++++++++++++++++++++++--
> xen/arch/x86/include/asm/msi.h | 5 +-
> xen/arch/x86/msi.c | 42 +++++++-
> 3 files changed, 242 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 999917983789..f7b7b4998b5e 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -180,6 +180,10 @@ static bool msixtbl_initialised(const struct domain *d)
> return d->arch.hvm.msixtbl_list.next;
> }
>
> +/*
> + * Lookup an msixtbl_entry on the same page as given addr. It's up to the
> + * caller to check if address is strictly part of the table - if relevant.
> + */
> static struct msixtbl_entry *msixtbl_find_entry(
> struct vcpu *v, unsigned long addr)
> {
> @@ -187,8 +191,8 @@ static struct msixtbl_entry *msixtbl_find_entry(
> struct domain *d = v->domain;
>
> list_for_each_entry( entry, &d->arch.hvm.msixtbl_list, list )
> - if ( addr >= entry->gtable &&
> - addr < entry->gtable + entry->table_len )
> + if ( PFN_DOWN(addr) >= PFN_DOWN(entry->gtable) &&
> + PFN_DOWN(addr) <= PFN_DOWN(entry->gtable + entry->table_len -
> 1) )
> return entry;
>
> return NULL;
> @@ -203,6 +207,10 @@ static struct msi_desc *msixtbl_addr_to_desc(
> if ( !entry || !entry->pdev )
> return NULL;
>
> + if ( addr < entry->gtable ||
> + addr >= entry->gtable + entry->table_len )
> + return NULL;
> +
> nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE;
>
> list_for_each_entry( desc, &entry->pdev->msi_list, list )
> @@ -213,6 +221,152 @@ static struct msi_desc *msixtbl_addr_to_desc(
> return NULL;
> }
>
> +/*
> + * Returns:
> + * - ADJACENT_DONT_HANDLE if no handling should be done
> + * - a fixmap idx to use for handling
> + */
> +#define ADJACENT_DONT_HANDLE UINT_MAX
Isn't it fine to just return 0 to signal that the access is not
handled?
fixmap index 0 is reserved anyway (see FIX_RESERVED), so could be used
for this purpose and then you don't need to introduce
ADJACENT_DONT_HANDLE?
> +static unsigned int adjacent_handle(
> + const struct msixtbl_entry *entry, unsigned long addr, bool write)
Now that it has been quite pruned, get_adjacent_idx() or some such
might be more inline with the function logic.
> +{
> + unsigned int adj_type;
> + struct arch_msix *msix;
> +
> + if ( !entry || !entry->pdev )
> + {
> + ASSERT_UNREACHABLE();
> + return ADJACENT_DONT_HANDLE;
> + }
> +
> + if ( PFN_DOWN(addr) == PFN_DOWN(entry->gtable) && addr < entry->gtable )
> + adj_type = ADJ_IDX_FIRST;
> + else if ( PFN_DOWN(addr) == PFN_DOWN(entry->gtable + entry->table_len -
> 1) &&
> + addr >= entry->gtable + entry->table_len )
> + adj_type = ADJ_IDX_LAST;
> + else
> + {
> + /* All callers should already do equivalent range checking. */
> + ASSERT_UNREACHABLE();
> + return ADJACENT_DONT_HANDLE;
> + }
> +
> + msix = entry->pdev->msix;
> + ASSERT(msix);
Since you already do it above, would be best to do:
if ( !msix )
{
ASSERT_UNREACHABLE();
return 0;
}
> +
> + if ( !msix->adj_access_idx[adj_type] )
> + {
> + if ( MSIX_CHECK_WARN(msix, entry->pdev->domain->domain_id,
> + adjacent_not_initialized) )
> + gprintk(XENLOG_WARNING,
> + "Page for adjacent(%d) MSI-X table access not
> initialized for %pp (addr %#lx, gtable %#lx\n",
Do you really need to log an error here? There's an error already
printed in msix_capability_init() if the adjacent pages can't be
mapped.
> + adj_type, &entry->pdev->sbdf, addr, entry->gtable);
> + return ADJACENT_DONT_HANDLE;
> + }
> +
> + /* If PBA lives on the same page too, discard writes. */
> + if ( write &&
> + ((adj_type == ADJ_IDX_LAST &&
> + msix->table.last == msix->pba.first) ||
> + (adj_type == ADJ_IDX_FIRST &&
> + msix->table.first == msix->pba.last)) )
> + {
> + if ( MSIX_CHECK_WARN(msix, entry->pdev->domain->domain_id,
> + adjacent_pba) )
> + gprintk(XENLOG_WARNING,
> + "MSI-X table and PBA of %pp live on the same page, "
> + "writing to other registers there is not implemented\n",
> + &entry->pdev->sbdf);
I would usually start those errors with the SBDF, as that makes it
easier to identify error originating from the same device:
"%pp: MSI-X table and PBA share a page, discard write to adjacent memory
(%#lx)\n",
&entry->pdev->sbdf, addr
> + return ADJACENT_DONT_HANDLE;
> + }
> +
> + return msix->adj_access_idx[adj_type];
> +}
> +
> +static int adjacent_read(
> + const struct msixtbl_entry *entry,
> + paddr_t address, unsigned int len, uint64_t *pval)
> +{
> + const void __iomem *hwaddr;
> + unsigned int fixmap_idx;
> +
I would add an ASSERT(IS_ALIGNED(address, len)) (and in
adjacent_write()) just in case, as otherwise the access could cross a
page boundary.
> + fixmap_idx = adjacent_handle(entry, address, false);
> +
> + if ( fixmap_idx == ADJACENT_DONT_HANDLE )
> + {
> + *pval = ~0UL;
> + return X86EMUL_OKAY;
> + }
FWIW, I find it safer to unconditionally init *pval = ~0UL at the
start of the function, and then the return here and in the default
switch statement case can avoid setting it. It's less easy to return
without the variable being set.
> + hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> +
> + switch ( len )
> + {
> + case 1:
> + *pval = readb(hwaddr);
> + break;
> +
> + case 2:
> + *pval = readw(hwaddr);
> + break;
> +
> + case 4:
> + *pval = readl(hwaddr);
> + break;
> +
> + case 8:
> + *pval = readq(hwaddr);
> + break;
> +
> + default:
> + ASSERT_UNREACHABLE();
> + *pval = ~0UL;
> + break;
We should likely move this to some kind of helpers, as it's now
exactly the same that's done in adjacent_{read,write}() in
vpci/msix.c (not that you should do it here, just a note).
> + }
> +
> + return X86EMUL_OKAY;
> +}
> +
> +static int adjacent_write(
> + const struct msixtbl_entry *entry,
> + paddr_t address, unsigned int len, uint64_t val)
> +{
> + void __iomem *hwaddr;
> + unsigned int fixmap_idx;
> +
> + fixmap_idx = adjacent_handle(entry, address, true);
> +
> + if ( fixmap_idx == ADJACENT_DONT_HANDLE )
> + return X86EMUL_OKAY;
> +
> + hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> +
> + switch ( len )
> + {
> + case 1:
> + writeb(val, hwaddr);
> + break;
> +
> + case 2:
> + writew(val, hwaddr);
> + break;
> +
> + case 4:
> + writel(val, hwaddr);
> + break;
> +
> + case 8:
> + writeq(val, hwaddr);
> + break;
> +
> + default:
> + ASSERT_UNREACHABLE();
> + break;
> + }
> +
> + return X86EMUL_OKAY;
> +}
> +
> static int cf_check msixtbl_read(
> const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
> uint64_t *pval)
> @@ -222,7 +376,7 @@ static int cf_check msixtbl_read(
> unsigned int nr_entry, index;
> int r = X86EMUL_UNHANDLEABLE;
>
> - if ( (len != 4 && len != 8) || (address & (len - 1)) )
> + if ( !IS_ALIGNED(address, len) )
> return r;
>
> rcu_read_lock(&msixtbl_rcu_lock);
> @@ -230,6 +384,17 @@ static int cf_check msixtbl_read(
> entry = msixtbl_find_entry(current, address);
> if ( !entry )
> goto out;
> +
> + if ( address < entry->gtable ||
> + address >= entry->gtable + entry->table_len )
> + {
> + r = adjacent_read(entry, address, len, pval);
> + goto out;
> + }
> +
> + if ( len != 4 && len != 8 )
> + goto out;
> +
> offset = address & (PCI_MSIX_ENTRY_SIZE - 1);
>
> if ( offset != PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
> @@ -291,6 +456,17 @@ static int msixtbl_write(struct vcpu *v, unsigned long
> address,
> entry = msixtbl_find_entry(v, address);
> if ( !entry )
> goto out;
> +
> + if ( address < entry->gtable ||
> + address >= entry->gtable + entry->table_len )
> + {
> + r = adjacent_write(entry, address, len, val);
> + goto out;
> + }
> +
> + if ( len != 4 && len != 8 )
> + goto out;
> +
> nr_entry = array_index_nospec(((address - entry->gtable) /
> PCI_MSIX_ENTRY_SIZE),
> MAX_MSIX_TABLE_ENTRIES);
> @@ -356,8 +532,8 @@ static int cf_check _msixtbl_write(
> const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
> uint64_t val)
> {
> - /* Ignore invalid length or unaligned writes. */
> - if ( (len != 4 && len != 8) || !IS_ALIGNED(address, len) )
> + /* Ignore unaligned writes. */
> + if ( !IS_ALIGNED(address, len) )
> return X86EMUL_OKAY;
>
> /*
> @@ -374,16 +550,25 @@ static bool cf_check msixtbl_range(
> {
> struct vcpu *curr = current;
> unsigned long addr = r->addr;
> - const struct msi_desc *desc;
> + const struct msixtbl_entry *entry;
> + bool ret = false;
>
> ASSERT(r->type == IOREQ_TYPE_COPY);
>
> rcu_read_lock(&msixtbl_rcu_lock);
> - desc = msixtbl_addr_to_desc(msixtbl_find_entry(curr, addr), addr);
> + entry = msixtbl_find_entry(curr, addr);
> + if ( entry )
> + {
> + if ( addr < entry->gtable || addr >= entry->gtable +
> entry->table_len )
> + /* Possibly handle adjacent access. */
> + ret = true;
> + else
> + ret = msixtbl_addr_to_desc(entry, addr) != NULL;
> + }
You could probably put all this into a single condition:
if ( entry &&
/* Adjacent access. */
(addr < entry->gtable || addr >= entry->gtable + entry->table_len ||
/* Otherwise check if there's a matching msi_desc. */
msixtbl_addr_to_desc(entry, addr)) )
ret = true;
That's IMO easier to read by setting ret once only.
> rcu_read_unlock(&msixtbl_rcu_lock);
>
> - if ( desc )
> - return 1;
> + if ( ret )
> + return ret;
>
> if ( r->state == STATE_IOREQ_READY && r->dir == IOREQ_WRITE )
> {
> @@ -429,7 +614,7 @@ static bool cf_check msixtbl_range(
> }
> }
>
> - return 0;
> + return false;
> }
>
> static const struct hvm_io_ops msixtbl_mmio_ops = {
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index bcfdfd35345d..923b730d48b8 100644
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -224,6 +224,9 @@ struct arch_msix {
> } table, pba;
> int table_refcnt[MAX_MSIX_TABLE_PAGES];
> int table_idx[MAX_MSIX_TABLE_PAGES];
> +#define ADJ_IDX_FIRST 0
> +#define ADJ_IDX_LAST 1
> + unsigned int adj_access_idx[2];
> spinlock_t table_lock;
> bool host_maskall, guest_maskall;
> domid_t warned_domid;
> @@ -231,6 +234,8 @@ struct arch_msix {
> uint8_t all;
> struct {
> bool maskall : 1;
> + bool adjacent_not_initialized : 1;
Not sure we need that, since we already warn of failure to map at
initialization time, not sure it's worth also printing another error
at access time.
> + bool adjacent_pba : 1;
> };
> } warned_kind;
> };
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 42c793426da3..87190a88ed5d 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -913,6 +913,37 @@ static int msix_capability_init(struct pci_dev *dev,
> list_add_tail(&entry->list, &dev->msi_list);
> *desc = entry;
> }
> + else
> + {
> + /*
> + * If the MSI-X table doesn't start at the page boundary, map the
> first page for
> + * passthrough accesses.
> + */
> + if ( PAGE_OFFSET(table_paddr) )
> + {
> + int idx = msix_get_fixmap(msix, table_paddr, table_paddr);
> +
> + if ( idx > 0 )
> + msix->adj_access_idx[ADJ_IDX_FIRST] = idx;
> + else
> + gprintk(XENLOG_ERR, "Failed to map first MSI-X table page:
> %d\n", idx);
> + }
> + /*
> + * If the MSI-X table doesn't end on the page boundary, map the last
> page
> + * for passthrough accesses.
> + */
> + if ( PAGE_OFFSET(table_paddr + msix->nr_entries *
> PCI_MSIX_ENTRY_SIZE) )
> + {
> + uint64_t entry_paddr = table_paddr +
> + (msix->nr_entries - 1) * PCI_MSIX_ENTRY_SIZE;
> + int idx = msix_get_fixmap(msix, table_paddr, entry_paddr);
> +
> + if ( idx > 0 )
> + msix->adj_access_idx[ADJ_IDX_LAST] = idx;
> + else
> + gprintk(XENLOG_ERR, "Failed to map last MSI-X table page:
> %d\n", idx);
Could you prefix the messages with the SBDF of the device please?
It's in the seg, bus, slot, func local variables AFAICT.
Thanks, Roger.
