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 <alex.william...@redhat.com>
>> ---
>>
>> 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
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
