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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Mar 28, 2023 at 02:34:23PM +0200, Jan Beulich wrote:
> On 28.03.2023 14:05, 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:
> >>> +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.
> 
> In his functionally similar vPCI change I did ask Roger to handle the
> "extra" space right from the same handlers. Maybe that's going to be
> best here, too.

I have considered this option, but msixtbl_range() is already quite
complex, adding yet another case there won't make it easier to follow.
I mean, technically I can probably merge those two handlers together,
but I don't think it will result in nicer code. Especially since the
general direction is to abandon split of MSI-X table access handling
between Xen and QEMU and go with just QEMU doing it, hopefully at some
point not needing msixtbl_mmio_ops anymore (but still needing the one
for adjacent accesses).

> >>> --- a/xen/arch/x86/msi.c
> >>> +++ b/xen/arch/x86/msi.c
> >>> @@ -961,6 +961,34 @@ 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 start at the page boundary, map 
> >>> the first page for
> >>> +         * passthrough accesses.
> >>> +         */
> >>
> >> I think you should initialize
> >> msix->adj_access_table_idx[ADJ_IDX_{FIRST,LAST}] to -1?
> 
> Or better not use a signed type there and set to UINT_MAX here.

If not using 0 as unused entry (see the other commend I made in response
to Roger), then that's probably the way to go.

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

Attachment: signature.asc
Description: PGP signature