Message-ID: <f46f44cd-2961-7731-d5a2-483c9e5189d1@intel.com>
Date:   Fri, 25 Aug 2023 09:55:52 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Jason Gunthorpe <jgg@...dia.com>
CC:     "yishaih@...dia.com" <yishaih@...dia.com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "tom.zanussi@...ux.intel.com" <tom.zanussi@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/3] vfio/ims: Support emulated interrupts

Hi Kevin,

On 8/24/2023 8:05 PM, Tian, Kevin wrote:
>> From: Chatre, Reinette <reinette.chatre@...el.com>
>> Sent: Friday, August 25, 2023 1:19 AM
>>
>> Hi Jason,
>>
>> On 8/24/2023 9:33 AM, Jason Gunthorpe wrote:
>>> On Thu, Aug 24, 2023 at 09:15:21AM -0700, Reinette Chatre wrote:
>>>> Access from a guest to a virtual device may be either 'direct-path',
>>>> where the guest interacts directly with the underlying hardware,
>>>> or 'intercepted path' where the virtual device emulates operations.
>>>>
>>>> Support emulated interrupts that can be used to handle 'intercepted
>>>> path' operations. For example, a virtual device may use 'intercepted
>>>> path' for configuration. Doing so, configuration requests intercepted
>>>> by the virtual device driver are handled within the virtual device
>>>> driver with completion signaled to the guest without interacting with
>>>> the underlying hardware.
>>>
>>> Why does this have anything to do with IMS? I thought the point here
>>> was that IMS was some back end to the MSI-X emulation - should a
>>> purely emulated interrupt logically be part of the MSI code, not IMS?
>>
>> You are correct, an emulated interrupt is not unique to IMS.
>>
>> The target usage of this library is by pure(?) VFIO devices (struct
>> vfio_device). These are virtual devices that are composed by separate
>> VFIO drivers. For example, a single resource of an accelerator device
>> can be composed into a stand-alone virtual device for use by a guest.
>>
>> Through its API and implementation the current VFIO MSI
>> code expects to work with actual PCI devices (struct
>> vfio_pci_core_device). With the target usage not being an
>> actual PCI device the VFIO MSI code was not found to be a good
>> fit and thus this implementation does not build on current MSI
>> support.
>>
> 
> This might be achieved by creating a structure vfio_pci_intr_ctx
> included by vfio_pci_core_device and other vfio device types. Then
> move vfio_pci_intr.c to operate on vfio_pci_intr_ctx instead of
> vfio_pci_core_device to make MSI frontend code sharable by both
> PCI devices or virtual devices (mdev or SIOV).

Thank you very much Kevin.

For data there is the per-device context related to interrupts as
well as the per-interrupt context. These contexts are different between
the different device types and interrupt types and if I understand
correctly both context types (per-device as well as per-interrupt)
should be made opaque within the new vfio_pci_intr.c. Additionally,
with different mechanisms (for example, the locking required) to
interact with the different device types the code is also device
(PCI or virtual) specific. Considering how both the data and
code would be opaque to this new library it looks to me as
though in some aspects this library may thus appear as a skeleton
with interrupt and device specific logic contained in its users.

It is not obvious to me where the MSI frontend and backend boundary
is. If I understand correctly majority of the code in vfio_pci_intrs.c
would move to callbacks. Potentially leaving little to be shared,
vfio_pci_set_irqs_ioctl() and vfio_msihandler() seems like
candidates. 

To clarify it sounds to me as though this design is motivated by the
requirement to bring emulated interrupt support to the current
INTx/MSI/MSI-X support on PCI devices? It is not clear to me how this
feature will be used since interrupts need to be signaled from the
driver instead of the hardware. I am missing something.

> Then there is only one irq_ctx. Within the ctx we can abstract
> backend ops, e.g. enable/disble_msi(), alloc/free_ctx(), alloc/free_irq(), etc.
> to accommodate pci MSI/MSI-X, IMS, or emulation.
> 
> The unknown risk is whether a clear abstraction can be defined. If
> in the end the common library contains many if-else to handle subtle
> backend differences then it might not be a good choice...

Thank you very much for your guidance. Instead of Jason's expectation that
IMS would be a backend of MSI-X this will change to IMS and MSI-X both being
a backend to a new interface. It is difficult for me to envision the end
result so I will work on an implementation based on my understanding of
your proposal that we can use for further discussion.

Reinette

Powered by blists - more mailing lists