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
