Re: [PATCH v2 2/3] x86/hvm: Allow writes to registers on the same page as MSI-X table

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Mar 28, 2023 at 02:05:14PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, Mar 28, 2023 at 01:28:44PM +0200, Roger Pau Monné wrote:
> > On Sat, Mar 25, 2023 at 03:49:23AM +0100, Marek Marczykowski-Górecki wrote:
> > > +             address >= entry->gtable + entry->table_len )
> > > +        {
> > > +            adj_type = ADJ_IDX_LAST;
> > > +            pdev = entry->pdev;
> > > +            break;
> > > +        }
> > > +    }
> > > +    rcu_read_unlock(&msixtbl_rcu_lock);
> > > +
> > > +    if ( !pdev )
> > > +        return NULL;
> > > +
> > > +    ASSERT(pdev->msix);
> > > +
> > > +    if ( !pdev->msix->adj_access_table_idx[adj_type] )
> > > +    {
> > > +        gdprintk(XENLOG_WARNING,
> > > +                 "Page for adjacent MSI-X table access not initialized for %pp\n",
> > > +                 pdev);
> > > +
> > > +        return NULL;
> > > +    }
> > > +
> > > +    /* If PBA lives on the same page too, forbid writes. */
> > > +    if ( write && (
> > > +        (adj_type == ADJ_IDX_LAST &&
> > > +         pdev->msix->table.last == pdev->msix->pba.first) ||
> > > +        (adj_type == ADJ_IDX_FIRST &&
> > > +         pdev->msix->table.first == pdev->msix->pba.last)) )
> > > +    {
> > > +        gdprintk(XENLOG_WARNING,
> > > +                 "MSI-X table and PBA of %pp live on the same page, "
> > > +                 "writing to other registers there is not implemented\n",
> > > +                 pdev);
> > 
> > Don't you actually need to check that the passed address falls into the
> > PBA array?  PBA can be sharing the same page as the MSI-X table, but
> > that doesn't imply there aren't holes also not used by either the PBA
> > or the MSI-X table in such page.
> 
> I don't know exact location of PBA, so I'm rejecting writes just in case
> they do hit PBA (see commit message).

Hm, maybe we should cache the address and size of the PBA in
msix_capability_init().

> > > +
> > > +}
> > > +
> > > +static bool cf_check msixtbl_page_accept(
> > > +        const struct hvm_io_handler *handler, const ioreq_t *r)
> > > +{
> > > +    ASSERT(r->type == IOREQ_TYPE_COPY);
> > > +
> > > +    return msixtbl_page_handler_get_hwaddr(
> > > +            current->domain, r->addr, r->dir == IOREQ_WRITE);
> > 
> > I think you want to accept it also if it's a write to the PBA, and
> > just drop it.  You should always pass write=false and then drop it in
> > msixtbl_page_write() if it falls in the PBA region (but still return
> > X86EMUL_OKAY).
> 
> I don't want to interfere with msixtbl_mmio_page_ops, this handler is
> only about accesses not hitting actual MSI-X structures.

But msixtbl_mmio_page_ops doesn't handle PBA array accesses at all, so
you won't interfere by accepting PBA writes here and dropping them in
msixtbl_page_write()?

Maybe there's some piece I'm missing.

> > > +}
> > > +
> > > +static int cf_check msixtbl_page_read(
> > > +        const struct hvm_io_handler *handler,
> > > +        uint64_t address, uint32_t len, uint64_t *pval)
> > 
> > Why use hvm_io_ops and not hvm_mmio_ops?  You only care about MMIO
> > accesses here.
> 
> I followed how msixtbl_mmio_ops are registered. Should that also be
> changed to hvm_mmio_ops?

Maybe, but let's leave that for another series I think.  Using
hvm_mmio_ops and register_mmio_handler() together with the slightly
simplified handlers will allow you to drop some of the code.

> > > +
> > > +    if ( address & (len - 1) )
> > > +        return X86EMUL_UNHANDLEABLE;
> > 
> > You can use IS_ALIGNED for clarity.  In my fix for this for vPCI Jan
> > asked to split unaligned accesses into 1byte ones, but I think for
> > guests it's better to just drop them unless there's a specific case
> > that we need to handle.
> > 
> > Also you should return X86EMUL_OKAY here, as the address is handled
> > here, but the current way to handle it is to drop the access.
> > 
> > Printing a debug message can be helpful in case unaligned accesses
> > need to be implemented in order to support some device.
> > 
> > > +
> > > +    hwaddr = msixtbl_page_handler_get_hwaddr(
> > > +            current->domain, address, false);
> > > +
> > > +    if ( !hwaddr )
> > > +        return X86EMUL_UNHANDLEABLE;
> > 
> > Maybe X86EMUL_RETRY, since the page was there in the accept handler.
> 
> How does X86EMUL_RETRY work? Is it trying to find the handler again?

Hm, I'm not sure it works as I thought it does, so maybe not a good
suggestion after all.

> > > @@ -1090,6 +1118,16 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
> > >              WARN();
> > >          msix->table.first = 0;
> > >          msix->table.last = 0;
> > > +        if ( msix->adj_access_table_idx[ADJ_IDX_FIRST] )
> > > +        {
> > > +            msix_put_fixmap(msix, msix->adj_access_table_idx[ADJ_IDX_FIRST]);
> > > +            msix->adj_access_table_idx[ADJ_IDX_FIRST] = 0;
> > > +        }
> > > +        if ( msix->adj_access_table_idx[ADJ_IDX_LAST] )
> > > +        {
> > > +            msix_put_fixmap(msix, msix->adj_access_table_idx[ADJ_IDX_LAST]);
> > > +            msix->adj_access_table_idx[ADJ_IDX_LAST] = 0;
> > 
> > Isn't 0 a valid idx?  You should check for >= 0 and also set
> > to -1 once the fixmap entry has been put.
> 
> I rely here on msix using specific range of fixmap indexes
> (FIX_MSIX_TO_RESERV_BASE - FIX_MSIX_TO_RESERV_END), which isn't starting
> at 0. For this reason also, I keep unused entries at 0 (no need explicit
> initialization).

Hm, OK, then you should also check that the return of
msix_get_fixmap() is strictly > 0, as right now it's using >= (and
thus would think 0 is a valid fixmap entry).

Thanks, Roger.