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.
