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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

It exits back to guest context, to restart the insn in question. Then
the full emulation path may be re-triggered. If the region was removed
from the guest, a handler then won't be found, and the access handed
to the device model.

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

Jan