|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/msi: Allow writes to registers on the same page as MSI-X table
On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
> On 14.11.2022 20:21, Marek Marczykowski-Górecki wrote:
> > Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
> > on the same page as MSI-X table. Device model (especially one in
> > stubdomain) cannot really handle those, as direct writes to that page is
> > refused (page is on mmio_ro_ranges list). Instead, add internal ioreq
> > server that handle those writes.
> >
> > This may be also used to read Pending Bit Array, if it lives on the same
>
> "may" sounds as if this would be future work, yet ...
I was meaning it applies only if it shares the page, but indeed it
should be "will".
> > page, making QEMU not needing /dev/mem access at all (especially helpful
> > with lockdown enabled in dom0). If PBA lives on another page, it can be
> > (and will be) mapped to the guest directly.
> > If PBA lives on the same page, forbid writes.
>
> ... here you say you actually handle the case (because otherwise you
> wouldn't need to distinguish writes from reads). It is only ...
>
> > Technically, writes outside
> > of PBA could be allowed, but at this moment the precise location of PBA
> > isn't saved.
>
> ... this part which right now you don't handle. I have to admit that I'm
> not convinced we should take such a partial implementation, especially
> if there's nothing recorded in the log (making it harder to tell whether
> something not working is because of this implementation restriction or
> some other issue).
For this, I need to save exact offset to PBA. I can extend the patch for
that case too.
> > --- a/xen/arch/x86/hvm/vmsi.c
> > +++ b/xen/arch/x86/hvm/vmsi.c
> > @@ -428,6 +428,133 @@ static const struct hvm_io_ops msixtbl_mmio_ops = {
> > .write = _msixtbl_write,
> > };
> >
> > +static void __iomem *msixtbl_page_handler_get_hwaddr(
> > + const struct vcpu *v,
> > + uint64_t address,
> > + bool write)
>
> These want to be indented just like ...
>
> > +{
> > + struct domain *d = v->domain;
> > + struct pci_dev *pdev = NULL;
> > + struct msixtbl_entry *entry;
> > + void __iomem *ret = NULL;
> > + uint64_t table_end_addr;
>
> ... function scope local variables.
>
> Also: Pointer-to-const for the first three local variables? And maybe
> omit "ret", which is effectively used just once (as you could use
> "return" at the point where you assign to it). Also you don't further
> use v afaics, so maybe have the callers pass in const struct domain *
> right away?
Makes sense indeed.
> > + rcu_read_lock(&msixtbl_rcu_lock);
> > + /*
> > + * Check if it's on the same page as the end of the MSI-X table, but
> > + * outside of the table itself.
> > + */
> > + list_for_each_entry( entry, &d->arch.hvm.msixtbl_list, list )
> > + if ( PFN_DOWN(address) == PFN_DOWN(entry->gtable +
> > entry->table_len) &&
> > + address >= entry->gtable + entry->table_len )
> > + {
> > + pdev = entry->pdev;
> > + break;
> > + }
> > + rcu_read_unlock(&msixtbl_rcu_lock);
> > +
> > + if ( !pdev )
> > + return NULL;
> > +
> > + ASSERT( pdev->msix );
>
> Style: ASSERT is not a (pseudo-)keyword and hence should not have
> blanks immediately inside the parentheses. (More instances further
> down.)
Ok.
> > + table_end_addr = (pdev->msix->table.first << PAGE_SHIFT) +
> > + pdev->msix->nr_entries * PCI_MSIX_ENTRY_SIZE;
> > + ASSERT( PFN_DOWN(table_end_addr) == pdev->msix->table.last );
>
> What are you trying to catch here? I ask because the local variable
> exists just for this checking afaics.
This is a double check if pdev->msix->table.last value is correct,
because I have two ways of calculating the address. Now, that I've
written this sentence, I see the way table_end_addr is calculated is
wrong, as it ignores table table not starting at the page boundary. I'll
remove both above lines.
> > + /* If PBA lives on the same page too, forbid writes. */
> > + if ( write && pdev->msix->table.last == pdev->msix->pba.first )
> > + return NULL;
> > +
> > + if ( pdev->msix->last_table_page )
> > + ret = pdev->msix->last_table_page + (address & (PAGE_SIZE - 1));
> > + else
> > + gdprintk(XENLOG_WARNING,
> > + "MSI-X last_table_page not initialized for
> > %04x:%02x:%02x.%u\n",
> > + pdev->seg,
> > + pdev->bus,
> > + PCI_SLOT(pdev->devfn),
> > + PCI_FUNC(pdev->devfn));
> > +
>
> Please use %pp.
Ok.
> > +static bool cf_check msixtbl_page_accept(
> > + const struct hvm_io_handler *handler, const ioreq_t *r)
> > +{
> > + unsigned long addr = r->addr;
>
> Any particular reason for having this local variable, which is used ...
>
> > + ASSERT( r->type == IOREQ_TYPE_COPY );
> > +
> > + return msixtbl_page_handler_get_hwaddr(
> > + current, addr, r->dir == IOREQ_WRITE);
> > +}
>
> ... exactly once?
Ok, will reduce.
> > +static int cf_check msixtbl_page_read(
> > + const struct hvm_io_handler *handler,
> > + uint64_t address, uint32_t len, uint64_t *pval)
> > +{
> > + void __iomem *hwaddr = msixtbl_page_handler_get_hwaddr(
> > + current, address, false);
> > +
> > + if ( !hwaddr )
> > + return X86EMUL_UNHANDLEABLE;
> > +
> > + switch ( len ) {
>
> Style: Brace on its own line please and ...
>
> > + case 1:
> > + *pval = readb(hwaddr);
> > + break;
> > + case 2:
> > + *pval = readw(hwaddr);
> > + break;
> > + case 4:
> > + *pval = readl(hwaddr);
> > + break;
> > + case 8:
> > + *pval = readq(hwaddr);
> > + break;
> > + default:
> > + return X86EMUL_UNHANDLEABLE;
>
> ... the body un-indented by a level.
Ok.
> As to operation I'm unconvinced that carrying out misaligned accesses
> here is generally safe. If we find devices really needing such, we
> may need to think about ways to deal with them without putting at
> risk everyone else. At the very least you need to make sure you don't
> access beyond the end of the page.
Right.
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -961,6 +961,21 @@ static int msix_capability_init(struct pci_dev *dev,
> > domain_crash(d);
> > /* XXX How to deal with existing mappings? */
> > }
> > +
> > + /*
> > + * If the MSI-X table doesn't span full page(s), map the last page
> > for
> > + * passthrough accesses.
> > + */
> > + if ( (msix->nr_entries * PCI_MSIX_ENTRY_SIZE) & (PAGE_SIZE - 1) )
> > + {
> > + uint64_t entry_paddr = table_paddr + msix->nr_entries *
> > PCI_MSIX_ENTRY_SIZE;
> > + int idx = msix_get_fixmap(msix, table_paddr, entry_paddr);
> > +
> > + if ( idx >= 0 )
> > + msix->last_table_page = fix_to_virt(idx);
> > + else
> > + gprintk(XENLOG_ERR, "Failed to map last MSI-X table page:
> > %d\n", idx);
> > + }
>
> Could we avoid the extra work if there's only less than one page's
> worth of entries for a device? But then again maybe not worth any
> extra code, as the same mapping will be re-used anyway due to the
> refcounting that's being used.
I was considering that, but decided against exactly because of
msix_get_fixmap() reusing existing mappings.
> Makes me think of another aspect though: Don't we also need to
> handle stuff living on the same page as the start of the table, if
> that doesn't start at a page boundary?
I have considered that, but decided against given every single device I
tried have MSI-X table at the page boundary. But if you prefer, I can
add such handling too (will require adding another variable to the
arch_msix structure - to store the fixmap location).
> > @@ -1090,6 +1105,12 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
> > WARN();
> > msix->table.first = 0;
> > msix->table.last = 0;
> > + if ( msix->last_table_page )
> > + {
> > + msix_put_fixmap(msix,
> > + virt_to_fix((unsigned
> > long)msix->last_table_page));
> > + msix->last_table_page = 0;
>
> To set a pointer please use NULL.
Ok.
> Overall it looks like you're dealing with the issue for HVM only.
> You will want to express this in the title, perhaps by using x86/hvm:
> as the prefix. But then the question of course is whether this couldn't
> be dealt with in/from mmio_ro_emulated_write(), which handles both HVM
> and PV.
The issue is correlating BAR mapping location with guest's view.
Writable BAR areas are mapped (by qemu) via xc_domain_memory_mapping(), but
that fails for read-only pages (and indeed, qemu doesn't attempt to do
that for the pages with the MSI-X table). Lacking that, I need to use
msixtbl_entry->gtable, which is HVM-only thing.
In fact there is another corner case I don't handle here: guest
accessing those registers when MSI-X is disabled. In that case, there is
no related msixtbl_entry, so I can't correlate the access, but the
page(s) is still read-only, so direct mapping would fail. In practice,
such access will trap into qemu, which will complain "Should not
read/write BAR through QEMU". I have seen this happening several times
when developing the series (due to bugs in my patches), but I haven't
found any case where it would happen with the final patch version.
In fact, I have considered handling this whole thing via qemu (as it
knows better where BAR live from the guest PoV), but stubdomain still
don't have write access to that pages, so that would need to be trapped
(for the second time) by Xen anyway.
For the PV case, I think this extra translation wouldn't be necessary as
BAR are mapped at their actual location, right? But then, it makes it
rather different implementation (separate feature), than just having a
common one for PV and HVM.
> Which in turn raises the question: Do you need to handle reads
> in the new code in the first place?
The page not being mapped is also the reason why I do need to handle
reads too.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Attachment:
signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |