From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2] vpci/msix: handle accesses adjacent to the MSI-X table
Date: Tue, 21 Mar 2023 16:39:41 +0100 [thread overview]
Message-ID: <f42e0638-b4bc-394d-25bc-317657b0d581@suse.com> (raw)
In-Reply-To: <ZBnN1VRLBtuoO/b0@Air-de-Roger>
On 21.03.2023 16:31, Roger Pau Monné wrote:
> On Mon, Mar 20, 2023 at 01:08:48PM +0100, Jan Beulich wrote:
>> On 16.03.2023 13:07, Roger Pau Monne wrote:
>>> {
>>> - struct vpci *vpci = msix->pdev->vpci;
>>> - unsigned int idx = addr - vmsix_table_addr(vpci, VPCI_MSIX_PBA);
>>> - const void __iomem *pba = get_pba(vpci);
>>> + unsigned int i;
>>> +
>>> + gprintk(XENLOG_DEBUG, "%pp: unaligned read to MSI-X related page\n",
>>> + &msix->pdev->sbdf);
>>>
>>> /*
>>> - * Access to PBA.
>>> + * Split unaligned accesses into byte sized ones. Shouldn't happen in
>>> + * the first place, but devices shouldn't have registers in the same 4K
>>> + * page as the MSIX tables either.
>>> *
>>> - * TODO: note that this relies on having the PBA identity mapped to the
>>> - * guest address space. If this changes the address will need to be
>>> - * translated.
>>> + * It's unclear whether this could cause issues if a guest expects a
>>> + * registers to be accessed atomically, it better use an aligned access
>>> + * if it has such expectations.
>>> */
>>> - if ( !pba )
>>> - {
>>> - gprintk(XENLOG_WARNING,
>>> - "%pp: unable to map MSI-X PBA, report all pending\n",
>>> - &msix->pdev->sbdf);
>>> - return X86EMUL_OKAY;
>>> - }
>>>
>>> - switch ( len )
>>> + for ( i = 0; i < len; i++ )
>>> {
>>> - case 4:
>>> - *data = readl(pba + idx);
>>> - break;
>>> + unsigned long partial = ~0ul;
>>
>> Pointless initializer (~0ul is written first thing above, i.e. also in
>> the recursive invocation). Then again that setting is also redundant
>> with msix_read()'s. So I guess the initializer wants to stay but the
>> setting at the top of the function can be dropped.
>
> I'm always extra cautious with variables on the stack that contain
> data to be returned to the guest. All your points are valid and
> correct, but I like to explicitly initialize them so that further
> changes in the functions don't end up leaking them. If you don't mind
> that much I would rather leave it as-is.
Well, such extra code always raises the question "Why is this here?"
But no, I won't insist if you prefer to keep the redundancy.
Jan
prev parent reply other threads:[~2023-03-21 15:39 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-16 12:07 [PATCH v2] vpci/msix: handle accesses adjacent to the MSI-X table Roger Pau Monne
2023-03-20 12:08 ` Jan Beulich
2023-03-21 15:31 ` Roger Pau Monné
2023-03-21 15:39 ` Jan Beulich [this message]
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=f42e0638-b4bc-394d-25bc-317657b0d581@suse.com \
--to=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).