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
