[PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Marek Marczykowski-Górecki]

QEMU needs to know whether clearing maskbit of a vector is really
clearing, or was already cleared before. Currently Xen sends only
clearing that bit to the device model, but not setting it, so QEMU
cannot detect it. Because of that, QEMU is working this around by
checking via /dev/mem, but that isn't the proper approach.

Give all necessary information to QEMU by passing all ctrl writes,
including masking a vector.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/arch/x86/hvm/vmsi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 75f92885dc5e..ba4cf46dfe91 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -271,7 +271,8 @@ out:
 }
 
 static int msixtbl_write(struct vcpu *v, unsigned long address,
-                         unsigned int len, unsigned long val)
+                         unsigned int len, unsigned long val,
+                         bool completion)
 {
     unsigned long offset;
     struct msixtbl_entry *entry;
@@ -343,7 +344,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
 
 unlock:
     spin_unlock_irqrestore(&desc->lock, flags);
-    if ( len == 4 )
+    if ( len == 4 && completion )
         r = X86EMUL_OKAY;
 
 out:
@@ -355,7 +356,7 @@ static int cf_check _msixtbl_write(
     const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
     uint64_t val)
 {
-    return msixtbl_write(current, address, len, val);
+    return msixtbl_write(current, address, len, val, false);
 }
 
 static bool cf_check msixtbl_range(
@@ -633,7 +634,7 @@ void msix_write_completion(struct vcpu *v)
         return;
 
     v->arch.hvm.hvm_io.msix_unmask_address = 0;
-    if ( msixtbl_write(v, ctrl_address, 4, 0) != X86EMUL_OKAY )
+    if ( msixtbl_write(v, ctrl_address, 4, 0, true) != X86EMUL_OKAY )
         gdprintk(XENLOG_WARNING, "MSI-X write completion failure\n");
 }
 
-- 
2.37.3

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

[Marek Marczykowski-Górecki]

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
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. Technically, writes outside
of PBA could be allowed, but at this moment the precise location of PBA
isn't saved.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/arch/x86/hvm/vmsi.c        | 135 +++++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/msi.h |   1 +
 xen/arch/x86/msi.c             |  21 +++++
 3 files changed, 157 insertions(+)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index ba4cf46dfe91..57cfcf70741e 100644
--- 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)
+{
+    struct domain *d = v->domain;
+    struct pci_dev *pdev = NULL;
+    struct msixtbl_entry *entry;
+    void __iomem *ret = NULL;
+    uint64_t table_end_addr;
+
+    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 );
+
+    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 );
+
+    /* 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));
+
+    return ret;
+}
+
+static bool cf_check msixtbl_page_accept(
+        const struct hvm_io_handler *handler, const ioreq_t *r)
+{
+    unsigned long addr = r->addr;
+
+    ASSERT( r->type == IOREQ_TYPE_COPY );
+
+    return msixtbl_page_handler_get_hwaddr(
+            current, addr, r->dir == IOREQ_WRITE);
+}
+
+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 ) {
+        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;
+    }
+    return X86EMUL_OKAY;
+}
+
+static int cf_check msixtbl_page_write(
+        const struct hvm_io_handler *handler,
+        uint64_t address, uint32_t len, uint64_t val)
+{
+    void __iomem *hwaddr = msixtbl_page_handler_get_hwaddr(
+            current, address, true);
+
+    if ( !hwaddr )
+        return X86EMUL_UNHANDLEABLE;
+
+    switch ( len ) {
+        case 1:
+            writeb(val, hwaddr);
+            break;
+        case 2:
+            writew(val, hwaddr);
+            break;
+        case 4:
+            writel(val, hwaddr);
+            break;
+        case 8:
+            writeq(val, hwaddr);
+            break;
+        default:
+            return X86EMUL_UNHANDLEABLE;
+    }
+    return X86EMUL_OKAY;
+
+}
+
+static const struct hvm_io_ops msixtbl_mmio_page_ops = {
+    .accept = msixtbl_page_accept,
+    .read = msixtbl_page_read,
+    .write = msixtbl_page_write,
+};
+
 static void add_msixtbl_entry(struct domain *d,
                               struct pci_dev *pdev,
                               uint64_t gtable,
@@ -583,6 +710,14 @@ void msixtbl_init(struct domain *d)
         handler->type = IOREQ_TYPE_COPY;
         handler->ops = &msixtbl_mmio_ops;
     }
+
+    /* passthrough access to other registers on the same page */
+    handler = hvm_next_io_handler(d);
+    if ( handler )
+    {
+        handler->type = IOREQ_TYPE_COPY;
+        handler->ops = &msixtbl_mmio_page_ops;
+    }
 }
 
 void msixtbl_pt_cleanup(struct domain *d)
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index fe670895eed2..d4287140f04c 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -236,6 +236,7 @@ struct arch_msix {
     } table, pba;
     int table_refcnt[MAX_MSIX_TABLE_PAGES];
     int table_idx[MAX_MSIX_TABLE_PAGES];
+    void __iomem *last_table_page;
     spinlock_t table_lock;
     bool host_maskall, guest_maskall;
     domid_t warned;
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index d0bf63df1def..e7fe41f424d8 100644
--- 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);
+        }
     }
     WARN_ON(msix->table.first != (table_paddr >> PAGE_SHIFT));
     ++msix->used_entries;
@@ -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;
+        }
 
         if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
                                    msix->pba.last) )
-- 
2.37.3

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

[Jan Beulich]

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 ...

> 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).

> --- 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?

> +    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.)

> +    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.

> +    /* 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.

> +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?

> +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.

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.

> --- 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.

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?

> @@ -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.

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. Which in turn raises the question: Do you need to handle reads
in the new code in the first place?

Jan

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 10579 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jan Beulich]

On 17.11.2022 18:31, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
>> On 14.11.2022 20:21, Marek Marczykowski-Górecki wrote:
>>> --- 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).

To limit growth of the struct, please at least consider storing the fixmap
indexes instead of full pointers.

>>> @@ -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?

I think so, yes.

> But then, it makes it
> rather different implementation (separate feature), than just having a
> common one for PV and HVM.

It would be different, yes, and if - as you explain above - there are
technical reasons why it cannot be shared, then so be it. Mentioning
this in the description may be worthwhile, or else the same question
may be asked again (even by me, in case I forget part of the discussion
by the time I look at a particular future version).

>> 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.

Just for my own clarity: You mean "not mapped to qemu" here?

Jan

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 5418 bytes --]

On Fri, Nov 18, 2022 at 08:20:14AM +0100, Jan Beulich wrote:
> On 17.11.2022 18:31, Marek Marczykowski-Górecki wrote:
> > On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
> >> On 14.11.2022 20:21, Marek Marczykowski-Górecki wrote:
> >>> --- 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).
> 
> To limit growth of the struct, please at least consider storing the fixmap
> indexes instead of full pointers.

Ok.

> >>> @@ -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?
> 
> I think so, yes.
> 
> > But then, it makes it
> > rather different implementation (separate feature), than just having a
> > common one for PV and HVM.
> 
> It would be different, yes, and if - as you explain above - there are
> technical reasons why it cannot be shared, then so be it. Mentioning
> this in the description may be worthwhile, or else the same question
> may be asked again (even by me, in case I forget part of the discussion
> by the time I look at a particular future version).

Ok, I'll extend the commit message.

> >> 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.
> 
> Just for my own clarity: You mean "not mapped to qemu" here?

No, to the HVM domain (in p2m). Xen (outside of MSI-X specific code for
HVM) doesn't know where those reads should be from.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jan Beulich]

On 18.11.2022 13:19, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 18, 2022 at 08:20:14AM +0100, Jan Beulich wrote:
>> On 17.11.2022 18:31, Marek Marczykowski-Górecki wrote:
>>> On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
>>>> 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.
>>
>> Just for my own clarity: You mean "not mapped to qemu" here?
> 
> No, to the HVM domain (in p2m). Xen (outside of MSI-X specific code for
> HVM) doesn't know where those reads should be from.

Hmm, I was expecting them to be mapped r/o to the guest, but perhaps I'm
misremembering. Clearly both ept_p2m_type_to_flags() and
p2m_type_to_flags() take mmio_ro_ranges into consideration, which is
what I was basing my understanding on (without having looked at other
places in detail).

Jan

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Fri, Nov 18, 2022 at 01:33:39PM +0100, Jan Beulich wrote:
> On 18.11.2022 13:19, Marek Marczykowski-Górecki wrote:
> > On Fri, Nov 18, 2022 at 08:20:14AM +0100, Jan Beulich wrote:
> >> On 17.11.2022 18:31, Marek Marczykowski-Górecki wrote:
> >>> On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
> >>>> 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.
> >>
> >> Just for my own clarity: You mean "not mapped to qemu" here?
> > 
> > No, to the HVM domain (in p2m). Xen (outside of MSI-X specific code for
> > HVM) doesn't know where those reads should be from.
> 
> Hmm, I was expecting them to be mapped r/o to the guest, but perhaps I'm
> misremembering. Clearly both ept_p2m_type_to_flags() and
> p2m_type_to_flags() take mmio_ro_ranges into consideration, which is
> what I was basing my understanding on (without having looked at other
> places in detail).

Qemu doesn't map the page (using xc_domain_memory_mapping()) where MSI-X
table lives. I tried to modify this part, but it resulted in EPT
violation on write attempt (and my emulation code wasn't called at all).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jan Beulich]

On 18.11.2022 14:00, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 18, 2022 at 01:33:39PM +0100, Jan Beulich wrote:
>> On 18.11.2022 13:19, Marek Marczykowski-Górecki wrote:
>>> On Fri, Nov 18, 2022 at 08:20:14AM +0100, Jan Beulich wrote:
>>>> On 17.11.2022 18:31, Marek Marczykowski-Górecki wrote:
>>>>> On Thu, Nov 17, 2022 at 05:34:36PM +0100, Jan Beulich wrote:
>>>>>> 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.
>>>>
>>>> Just for my own clarity: You mean "not mapped to qemu" here?
>>>
>>> No, to the HVM domain (in p2m). Xen (outside of MSI-X specific code for
>>> HVM) doesn't know where those reads should be from.
>>
>> Hmm, I was expecting them to be mapped r/o to the guest, but perhaps I'm
>> misremembering. Clearly both ept_p2m_type_to_flags() and
>> p2m_type_to_flags() take mmio_ro_ranges into consideration, which is
>> what I was basing my understanding on (without having looked at other
>> places in detail).
> 
> Qemu doesn't map the page (using xc_domain_memory_mapping()) where MSI-X
> table lives. I tried to modify this part, but it resulted in EPT
> violation on write attempt (and my emulation code wasn't called at all).

Well, yes - that leads to the other path I pointed you at, which is
used for both HVM and PV (and which, as per what you say, then looks
to be relevant for PVH only right now).

Jan

Re: [PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Jan Beulich]

On 14.11.2022 20:20, Marek Marczykowski-Górecki wrote:
> QEMU needs to know whether clearing maskbit of a vector is really
> clearing, or was already cleared before. Currently Xen sends only
> clearing that bit to the device model, but not setting it, so QEMU
> cannot detect it.

Except for qword writes as it looks. Furthermore even clearing
requests aren't sent if address/data are unchanged. If you agree,
please add this here in some form for having a complete picture.

> Because of that, QEMU is working this around by
> checking via /dev/mem, but that isn't the proper approach.
> 
> Give all necessary information to QEMU by passing all ctrl writes,
> including masking a vector.

Can we perhaps still avoid sending dword writes which don't change
the mask bit?

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -271,7 +271,8 @@ out:
>  }
>  
>  static int msixtbl_write(struct vcpu *v, unsigned long address,
> -                         unsigned int len, unsigned long val)
> +                         unsigned int len, unsigned long val,
> +                         bool completion)
>  {

I'd like to propose an alternative approach without an extra parameter:
Have msix_write_completion() pass 0 for "len" and move the initial
check

    if ( (len != 4 && len != 8) || (address & (len - 1)) )
        return r;

into _msixtbl_write(). Then ...

> @@ -343,7 +344,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>  
>  unlock:
>      spin_unlock_irqrestore(&desc->lock, flags);
> -    if ( len == 4 )
> +    if ( len == 4 && completion )
>          r = X86EMUL_OKAY;

... this could simply be "if ( !len )", seeing that even with your
approach it could simply be "if ( completion )".

Jan

Re: [PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

On Tue, Nov 15, 2022 at 10:36:32AM +0100, Jan Beulich wrote:
> On 14.11.2022 20:20, Marek Marczykowski-Górecki wrote:
> > QEMU needs to know whether clearing maskbit of a vector is really
> > clearing, or was already cleared before. Currently Xen sends only
> > clearing that bit to the device model, but not setting it, so QEMU
> > cannot detect it.
> 
> Except for qword writes as it looks. Furthermore even clearing
> requests aren't sent if address/data are unchanged. If you agree,
> please add this here in some form for having a complete picture.

Ok.

> > Because of that, QEMU is working this around by
> > checking via /dev/mem, but that isn't the proper approach.
> > 
> > Give all necessary information to QEMU by passing all ctrl writes,
> > including masking a vector.
> 
> Can we perhaps still avoid sending dword writes which don't change
> the mask bit?

Is it worth it? I don't think such writes are common (which I confirm
observing debug log - every single write to maskbit Linux did was
changing the value). The old value isn't readily available here.

> > --- a/xen/arch/x86/hvm/vmsi.c
> > +++ b/xen/arch/x86/hvm/vmsi.c
> > @@ -271,7 +271,8 @@ out:
> >  }
> >  
> >  static int msixtbl_write(struct vcpu *v, unsigned long address,
> > -                         unsigned int len, unsigned long val)
> > +                         unsigned int len, unsigned long val,
> > +                         bool completion)
> >  {
> 
> I'd like to propose an alternative approach without an extra parameter:
> Have msix_write_completion() pass 0 for "len" and move the initial
> check
> 
>     if ( (len != 4 && len != 8) || (address & (len - 1)) )
>         return r;
> 
> into _msixtbl_write(). Then ...
> 
> > @@ -343,7 +344,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
> >  
> >  unlock:
> >      spin_unlock_irqrestore(&desc->lock, flags);
> > -    if ( len == 4 )
> > +    if ( len == 4 && completion )
> >          r = X86EMUL_OKAY;
> 
> ... this could simply be "if ( !len )", seeing that even with your
> approach it could simply be "if ( completion )".

I find such usage of magic len=0 confusing. It would change the meaning
of "len" from "write length" to "write length, unless it's 0 - then
write length is 4 and it's called from msix_write_completion.
Is there any real value from avoiding extra parameter?


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Jan Beulich]

On 15.11.2022 12:37, Marek Marczykowski-Górecki wrote:
> On Tue, Nov 15, 2022 at 10:36:32AM +0100, Jan Beulich wrote:
>> On 14.11.2022 20:20, Marek Marczykowski-Górecki wrote:
>>> Give all necessary information to QEMU by passing all ctrl writes,
>>> including masking a vector.
>>
>> Can we perhaps still avoid sending dword writes which don't change
>> the mask bit?
> 
> Is it worth it? I don't think such writes are common (which I confirm
> observing debug log - every single write to maskbit Linux did was
> changing the value). The old value isn't readily available here.

For one a 2nd aspect would be Windows behavior. As you've seen in
the hypervisor code somebody back at the time even thought
accelerating reads was useful. I'm going from that rather than
knowing for sure that such an optimization would help anywhere.

>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -271,7 +271,8 @@ out:
>>>  }
>>>  
>>>  static int msixtbl_write(struct vcpu *v, unsigned long address,
>>> -                         unsigned int len, unsigned long val)
>>> +                         unsigned int len, unsigned long val,
>>> +                         bool completion)
>>>  {
>>
>> I'd like to propose an alternative approach without an extra parameter:
>> Have msix_write_completion() pass 0 for "len" and move the initial
>> check
>>
>>     if ( (len != 4 && len != 8) || (address & (len - 1)) )
>>         return r;
>>
>> into _msixtbl_write(). Then ...
>>
>>> @@ -343,7 +344,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>>>  
>>>  unlock:
>>>      spin_unlock_irqrestore(&desc->lock, flags);
>>> -    if ( len == 4 )
>>> +    if ( len == 4 && completion )
>>>          r = X86EMUL_OKAY;
>>
>> ... this could simply be "if ( !len )", seeing that even with your
>> approach it could simply be "if ( completion )".
> 
> I find such usage of magic len=0 confusing. It would change the meaning
> of "len" from "write length" to "write length, unless it's 0 - then
> write length is 4 and it's called from msix_write_completion.
> Is there any real value from avoiding extra parameter?

Perhaps a matter of taste, but to me redundant parameters are odd
at times as well - often I end up wondering in such cases why an
extra parameter was introduced when things could easily be done
with what was there. In the specific case here there's also the
further aspect of you moving the function across the boundary of
all arguments fitting in registers available for parameter passing
(which of course only matters if the compiler decides to not
inline the function at all call sites).

Jan