On 26.04.2024 19:54, 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
> of 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.
>
> GCC 12.2.1 gets confused about 'desc' variable:
>
> arch/x86/hvm/vmsi.c: In function ‘msixtbl_range’:
> arch/x86/hvm/vmsi.c:553:8: error: ‘desc’ may be used uninitialized
> [-Werror=maybe-uninitialized]
> 553 | if ( desc )
> | ^
> arch/x86/hvm/vmsi.c:537:28: note: ‘desc’ was declared here
> 537 | const struct msi_desc *desc;
> | ^~~~
>
> It's conditional initialization is actually correct (in the case where
> it isn't initialized, function returns early), but to avoid
> build failure initialize it explicitly to NULL anyway.
>
> Signed-off-by: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
_Without_ the usually implied ack (as indicated before) and with two
small tweaks (which can likely be taken care of while committing):
Reviewed-by: Jan Beulich <jbeul...@suse.com>
> +static int adjacent_read(
> + unsigned int fixmap_idx,
> + paddr_t address, unsigned int len, uint64_t *pval)
> +{
> + const void __iomem *hwaddr;
> +
> + ASSERT(fixmap_idx <= FIX_MSIX_IO_RESERV_END);
> +
> + 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;
Nit: Better ~0ULL here (short of there being UINT64_C()).
> + break;
> + }
> +
> + return X86EMUL_OKAY;
> +}
> +
> +static int adjacent_write(
> + unsigned int fixmap_idx,
> + paddr_t address, unsigned int len, uint64_t val)
> +{
> + void __iomem *hwaddr;
> +
> + if ( fixmap_idx == ADJACENT_DISCARD_WRITE )
> + return X86EMUL_OKAY;
> +
> + ASSERT(fixmap_idx <= FIX_MSIX_IO_RESERV_END);
> +
> + 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();
> + }
There's still a "break;" missing here.
Jan
