Re: [Qemu-devel] [PATCH] msix: Support specifying offsets, BARs, and capability location

[Jan Kiszka]

On 2012-06-13 13:21, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 12:44:01PM +0200, Jan Kiszka wrote:
>> On 2012-06-12 22:03, Alex Williamson wrote:
>>> msix_init has very little configurability as to how it lays out MSIX
>>> for a device.  It claims to resize BARs, but doesn't actually do this
>>> anymore.  This patch allows MSIX to be fully specified, which is
>>> necessary both for emulated devices trying to match the physical
>>> layout of a hardware device as well as for any kind of device
>>> assignment.
>>>
>>> New functions msix_init_bar & msix_uninit_bar provide wrappers around
>>> the more detailed functions for drivers that just want a simple MSIX
>>> setup.
>>>
>>> Signed-off-by: Alex Williamson <address@hidden>
>>> ---
>>>
>>>  hw/ivshmem.c    |    9 +-
>>>  hw/msix.c       |  299 
>>> +++++++++++++++++++++++++++++++------------------------
>>>  hw/msix.h       |   11 +-
>>>  hw/pci.h        |   12 ++
>>>  hw/virtio-pci.c |   15 +--
>>>  5 files changed, 192 insertions(+), 154 deletions(-)
>>>
>>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>>> index 05559b6..71c84a6 100644
>>> --- a/hw/ivshmem.c
>>> +++ b/hw/ivshmem.c
>>> @@ -563,16 +563,13 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>>>
>>>  static void ivshmem_setup_msi(IVShmemState * s)
>>>  {
>>> -    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
>>> -    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
>>> -        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>> -                         &s->msix_bar);
>>> -        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
>>> -    } else {
>>> +    if (msix_init_bar(&s->dev, s->vectors, &s->msix_bar, 1, 
>>> "ivshmem-msix")) {
>>
>> I don't think the callers of msix_init_bar should have to provide the
>> memory region for that bar. That can be embedded into PCIDevice, just
>> like you did for the table and PBA. That was my idea with msix_init_simple.
>>
>> Back then, I only included a generic memory region name. That can be
>> improved, but without bothering the caller. Just derive it from
>> PCIDevice::name.
>>
>> Jan
> 
> I think callers must initialize the BAR regions.
> This is because BAR can include other stuff besides MSI-X.
> MSI-X adds its own subregion.

That's the non-common case handled by msix_init. I don't see this as
typical for emulated devices.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux