[Qemu-devel] [PATCH] msi/msix: added functions to API to set up message address, and data

[Alexey Kardashevskiy]

Ok, another try. Is it any better now? :)


Normally QEMU expects the guest to initialize MSI/MSIX vectors.
However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
does not write these vectors to device's config space or MSIX BAR.

On the other hand, msi_notify()/msix_notify() write to these vectors to
signal the guest about an interrupt so we have to write correct vectors
to the devices in order not to change every user of MSI/MSIX.

The first aim is to support MSIX for virtio-pci on POWER. There is
another patch for POWER coming which introduces a special memory region
where MSI/MSIX vectors point to.

Signed-off-by: Alexey Kardashevskiy <address@hidden>
---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    8 ++++++++
 hw/msix.h |    3 +++
 4 files changed, 26 insertions(+)

diff --git a/hw/msi.c b/hw/msi.c
index 5233204..c7b3e6a 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -363,3 +363,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     return msi_nr_vectors(flags);
 }
+
+void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
+}
+
diff --git a/hw/msi.h b/hw/msi.h
index 75747ab..353386e 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -39,6 +39,7 @@ void msi_reset(PCIDevice *dev);
 void msi_notify(PCIDevice *dev, unsigned int vector);
 void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
 unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
+void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data);
 
 static inline bool msi_present(const PCIDevice *dev)
 {
diff --git a/hw/msix.c b/hw/msix.c
index ded3c55..08e773d 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -526,3 +526,11 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
     dev->msix_vector_use_notifier = NULL;
     dev->msix_vector_release_notifier = NULL;
 }
+void msix_set_address_data(PCIDevice *dev, int vector,
+                           uint64_t address, uint32_t data)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
diff --git a/hw/msix.h b/hw/msix.h
index 50aee82..901f101 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -35,4 +35,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
                               MSIVectorUseNotifier use_notifier,
                               MSIVectorReleaseNotifier release_notifier);
 void msix_unset_vector_notifiers(PCIDevice *dev);
+void msix_set_address_data(PCIDevice *dev, int vector,
+                           uint64_t address, uint32_t data);
+
 #endif
-- 
1.7.10


On 14/06/12 15:45, Jan Kiszka wrote:
> On 2012-06-14 07:17, Alexey Kardashevskiy wrote:
>> On 14/06/12 14:56, Alex Williamson wrote:
>>> On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
>>>> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
>>>> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
>>>> does not write these vectors to device's config space or MSIX BAR.
>>>>
>>>> On the other hand, msi_notify()/msix_notify() write to these vectors to
>>>> signal the guest about an interrupt so we have to write correct vectors
>>>> to the devices in order not to change every user of MSI/MSIX.
>>>>
>>>> The first aim is to support MSIX for virtio-pci on POWER. There is
>>>> another patch for POWER coming which introduces a special memory region
>>>> where MSI/MSIX vectors point to.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>> ---
>>>>  hw/msi.c  |   14 ++++++++++++++
>>>>  hw/msi.h  |    1 +
>>>>  hw/msix.c |   10 ++++++++++
>>>>  hw/msix.h |    3 +++
>>>>  4 files changed, 28 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>> index 5d6ceb6..124878a 100644
>>>> --- a/hw/msi.c
>>>> +++ b/hw/msi.c
>>>> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice 
>>>> *dev)
>>>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>>>      return msi_nr_vectors(flags);
>>>>  }
>>>> +
>>>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
>>>> +{
>>>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>>>> +
>>>> +    if (msi64bit) {
>>>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
>>>> +    } else {
>>>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
>>>> +    }
>>>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
>>>> +}
>>>
>>> Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
>>> you tossed in a msi_get_message() as well, I think we need it to be able
>>> to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,
>>
>>
>> I am missing the point. What is that MSIMessage?
>> It is just an address and data, making a struct from this is a bit too much 
>> :)
> 
> This is about consistent APIs. In practice (assembly), MSIMessage will
> make no difference from open-coded address/data tuples, but it is
> cleaner to pass around. Please follow the existing patterns.
> 
>> I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger 
>> picture, sorry.
> 
> The kvm_irqchip_* API come into play when you implement some interrupt
> controller models in the kernel for performance reasons. We have this on
> x86, we will see it on Book3E and ARM soon. You are targeting Book3S,
> right? Not sure what the plans are there.
> 
> And, yes, msi_get_message() will be needed soon as well, at latest when
> I push vector notifiers for classic MSI. If you have a need for reading
> the message back, please add this helper already.
> 
> Jan
> 


-- 
Alexey