Re: [Qemu-devel] [RFC][PATCH 42/45] msix: Introduce msix_init_simple

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-10-18 12:52, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 09:21:34PM +0200, Jan Kiszka wrote:
>> On 2011-10-17 16:28, Michael S. Tsirkin wrote:
>>> On Mon, Oct 17, 2011 at 01:27:31PM +0200, Jan Kiszka wrote:
>>>> On 2011-10-17 13:22, Michael S. Tsirkin wrote:
>>>>> On Mon, Oct 17, 2011 at 11:28:16AM +0200, Jan Kiszka wrote:
>>>>>> Devices models are usually not interested in specifying MSI-X
>>>>>> configuration details beyond the number of vectors to provide and the
>>>>>> BAR number to use. Layout of an exclusively used BAR and its
>>>>>> registration can also be handled centrally.
>>>>>>
>>>>>> This is the purpose of msix_init_simple. It provides handy services to
>>>>>> the existing users. Future users like device assignment may require more
>>>>>> detailed setup specification. For them we will (re-)introduce msix_init
>>>>>> with the full list of configuration option (in contrast to the current
>>>>>> code).
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Well, this seems a bit of a code churn then, doesn't it?
>>>>> We are also discussing using memory BAR for virtio-pci for other
>>>>> stuff besides MSI-X, so the last user of the _simple variant
>>>>> will be ivshmem then?
>>>>
>>>> We will surely see more MSI-X users over the time. Not sure if they all
>>>> mix their MSIX-X BARs with other stuff. But e.g. the e1000 variant I
>>>> have here does not. So there should be users in the future.
>>>>
>>>> Jan
>>>
>>> Question is, how hard is to pass in the BAR and the offset?
>>
>> That is trivial. But have a look at the final simple implementation. It
>> also manages the container memory region for table and PBA and
>> registers/unregisters that container as BAR. So there is measurable
>> added-value.
>>
>> Jan
>>
> 
> Yes, I agree. In particular it's not very nice that the user has to know
> the size of the bar to create. But the API is very unfortunate IMO.

I'm open to see the prototypes of a different one.

> 
> I am also more interested in solutions that help all devices
> and not just those that have a dedicated bar for msix + pba.

That's what patch 43 is for.

> 
> We should probably pass in the size of the memory region allocated to
> the msix table, and verify that the table fits there.

Well, if you specify table and PBA offset explicitly, I think it is fair
to expect the caller having done the math.

There are some checks built into the core already, e.g. against table
and PBA overlap. We could add one for checking BAR limits. I'm not sure,
though, if requesting table and PBA sizes solves the issue that the user
may have done the calculation wrong.

> We can also avoid passing in bar number, like this:
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 749e8d8..d0d893e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -903,6 +903,17 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>          : pci_dev->bus->address_space_mem;
>  }
>  
> +int pci_get_bar_nr(PCIDevice *pci_dev, MemoryRegion *bar)
> +{
> +    int region_num;
> +    for (region_num = 0; region_num < PCI_NUM_REGIONS; ++region_num) {
> +        if (pci_dev->io_regions[region_num].memory == bar) {
> +            return region_num;
> +        }
> +    }
> +    return -1;
> +}
> +
>  pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num)
>  {
>      return pci_dev->io_regions[region_num].addr;

That enforces a specific registration order. If you call msix_init
before doing the BAR registration, the function above will not help.

Jan

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