Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Jan Kiszka]

On 2012-06-14 18:05, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
>> On 2012-06-14 17:49, Michael S. Tsirkin wrote:
>>> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
>>>> msi_init() takes over a BAR without really specifying or allowing
>>>> specification of how it does so.  Instead, let's split it into
>>>> two interfaces, one fully specified, and one trivially easy.  This
>>>> implements the latter.  msix_init_exclusive_bar() takes over
>>>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
>>>> When used, the matching msi_uninit_exclusive_bar() should be used
>>>> to tear it down.
>>>>
>>>> Signed-off-by: Alex Williamson <address@hidden>
>>>> ---
>>>>
>>>>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/msix.h |    3 +++
>>>>  hw/pci.h  |    2 ++
>>>>  3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>> index b64f109..a4cdfb0 100644
>>>> --- a/hw/msix.c
>>>> +++ b/hw/msix.c
>>>> @@ -299,6 +299,41 @@ err_config:
>>>>      return ret;
>>>>  }
>>>>  
>>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>>> +                            uint8_t bar_nr)
>>>> +{
>>>> +    int ret;
>>>> +    char *name;
>>>> +
>>>> +    /*
>>>> +     * Migration compatibility dictates that this remains a 4k
>>>> +     * BAR with the vector table in the lower half and PBA in
>>>> +     * the upper half.
>>>> +     */
>>>> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
>>>> +
>>>> +    free(name);
>>>> +
>>>> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 
>>>> 4096);
>>>> +    if (ret) {
>>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>> +                     &dev->msix_exclusive_bar);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  static void msix_free_irq_entries(PCIDevice *dev)
>>>>  {
>>>>      int vector;
>>>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>>>>      return 0;
>>>>  }
>>>>  
>>>> +void msix_uninit_exclusive_bar(PCIDevice *dev)
>>>> +{
>>>> +    if (msix_present(dev)) {
>>>> +        msix_uninit(dev, &dev->msix_exclusive_bar);
>>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>>>> +    }
>>>> +}
>>>> +
>>>>  void msix_save(PCIDevice *dev, QEMUFile *f)
>>>>  {
>>>>      unsigned n = dev->msix_entries_nr;
>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>> index e5a488d..bed6bfb 100644
>>>> --- a/hw/msix.h
>>>> +++ b/hw/msix.h
>>>> @@ -7,11 +7,14 @@
>>>>  int msix_init(PCIDevice *pdev, unsigned short nentries,
>>>>                MemoryRegion *bar,
>>>>                unsigned bar_nr, unsigned bar_size);
>>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>>> +                            uint8_t bar_nr);
>>>>  
>>>>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>>>>                         uint32_t val, int len);
>>>>  
>>>>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
>>>> +void msix_uninit_exclusive_bar(PCIDevice *dev);
>>>>  
>>>>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>>>>  
>>>> diff --git a/hw/pci.h b/hw/pci.h
>>>> index 4c96268..d517a54 100644
>>>> --- a/hw/pci.h
>>>> +++ b/hw/pci.h
>>>> @@ -226,6 +226,8 @@ struct PCIDevice {
>>>>  
>>>>      /* Space to store MSIX table */
>>>>      uint8_t *msix_table_page;
>>>> +    /* MemoryRegion container for msix exclusive BAR setup */
>>>> +    MemoryRegion msix_exclusive_bar;
>>>
>>> Wrappers are good but this doesn't belong in PCIDevice IMO.
>>> E.g. when I debug and look at data structure I don't want to dig into
>>> code and go "oh this device uses an exclusive bar to it's here, that one
>>> uses a non exlcusive so the field is invalid, the real bar is over
>>> there.
>>>
>>> Keep the region in devices where it was, pass to msix_init_exclusive_bar
>>> by parameter.
>>
>> I disagree. There are plenty of fields in PCIDevice that are only used
>> under certain circumstances. This is just +1, and it makes the interface
>> usage much easier.
>>
>> Jan
> 
> *Much* easier? It's one parameter less.

...and one field to add to every user of msix_init_exclusive_bar while
those users have no business with it. The container is a pure internal
field, so it belongs to the infrastructure.

Jan

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