Re: [PATCH v2] vpci/msix: handle accesses adjacent to the MSI-X table

[Jan Beulich <jbeulich@xxxxxxxx>]

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