From: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table
Date: Fri, 26 Apr 2024 17:26:49 +0200 [thread overview]
Message-ID: <ZivHw9RUUN1CV4Hi@mail-itl> (raw)
In-Reply-To: <68f99f0a-e27a-449f-8d13-fb5ca9f6069a@suse.com>
[-- Attachment #1: Type: text/plain, Size: 4729 bytes --]
On Thu, Apr 25, 2024 at 01:15:34PM +0200, Jan Beulich wrote:
> On 13.03.2024 16:16, 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 <marmarek@invisiblethingslab.com>
>
> Sadly there are further more or less cosmetic issues. Plus, as indicated
> before, I'm not really happy for us to gain all of this extra code. In
> the end I may eventually give an R-b not including the usually implied
> A-b, to indicate the code (then) looks okay to me but I still want
> someone else to actually ack it to allow it going in.
I understand. Given similar code is committed for vPCI already, I hope
somebody will be comfortable with acking this one too (yes, I do realize
the vPCI one is much less exposed, but still).
> > +static int adjacent_read(
> > + unsigned int fixmap_idx,
> > + paddr_t address, unsigned int len, uint64_t *pval)
> > +{
> > + const void __iomem *hwaddr;
> > +
> > + *pval = ~0UL;
> > +
> > + ASSERT(fixmap_idx != ADJACENT_DISCARD_WRITE);
>
> Why only one of the special values? And before you add the other here:
> Why not simply ASSERT(fixmap_idx <= FIX_MSIX_IO_RESERV_END)? (Could of
> course bound at the other end, too, i.e. against FIX_MSIX_IO_RESERV_BASE.)
That's the most likely bug that could happen, but indeed broader assert
would be better.
> > + 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();
>
> Misra demands "break;" to be here for release builds. In fact I wonder
> why "*pval = ~0UL;" isn't put here, too. Question of course is whether
> in such a case a true error indicator wouldn't be yet better.
I don't think it possible for the msixtbl_read() (that calls
adjacent_read()) to be called with other sizes. The default label is
here exactly to make it obvious for the reader.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2024-04-26 15:27 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-13 15:16 [PATCH v5 0/7] MSI-X support with qemu in stubdomain, and other related changes Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 1/7] x86/msi: passthrough all MSI-X vector ctrl writes to device model Marek Marczykowski-Górecki
2024-04-25 10:45 ` Jan Beulich
2024-04-29 8:40 ` Roger Pau Monné
2024-03-13 15:16 ` [PATCH v5 2/7] x86/msi: Extend per-domain/device warning mechanism Marek Marczykowski-Górecki
2024-04-25 11:25 ` Jan Beulich
2024-03-13 15:16 ` [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table Marek Marczykowski-Górecki
2024-04-25 11:15 ` Jan Beulich
2024-04-26 15:26 ` Marek Marczykowski-Górecki [this message]
2024-04-29 6:37 ` Jan Beulich
2024-03-13 15:16 ` [PATCH v5 4/7] automation: prevent QEMU access to /dev/mem in PCI passthrough tests Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 5/7] automation: switch to a wifi card on ADL system Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 6/7] [DO NOT APPLY] switch to qemu fork Marek Marczykowski-Górecki
2024-03-13 15:16 ` [PATCH v5 7/7] [DO NOT APPLY] switch to alternative artifact repo Marek Marczykowski-Górecki
2024-04-18 14:18 ` [PATCH v5 0/7] MSI-X support with qemu in stubdomain, and other related changes Marek Marczykowski-Górecki
2024-04-18 14:22 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZivHw9RUUN1CV4Hi@mail-itl \
--to=marmarek@invisiblethingslab.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.